Add additional HTTP Response splitting prevention

- Adding multiple test.
- HTTP response splitting should be validated too on cookie attributes and
header name.

Issue gh-3910

web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java

@@ -15,18 +15,22 @@
  */
 package org.springframework.security.web.firewall;
 
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpServletResponseWrapper;
 import java.io.IOException;
 import java.util.regex.Pattern;
 
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpServletResponseWrapper;
+
 /**
  * @author Luke Taylor
  * @author Eddú Meléndez
+ * @author Gabriel Lavoie
  */
 class FirewalledResponse extends HttpServletResponseWrapper {
-
 	private static final Pattern CR_OR_LF = Pattern.compile("\\r|\\n");
+	private static final String LOCATION_HEADER = "Location";
+	private static final String SET_COOKIE_HEADER = "Set-Cookie";
 
 	public FirewalledResponse(HttpServletResponse response) {
 		super(response);
@@ -36,7 +40,7 @@ class FirewalledResponse extends HttpServletResponseWrapper {
 	public void sendRedirect(String location) throws IOException {
 		// TODO: implement pluggable validation, instead of simple blacklisting.
 		// SEC-1790. Prevent redirects containing CRLF
-		validateCRLF("Location", location);
+		validateCRLF(LOCATION_HEADER, location);
 		super.sendRedirect(location);
 	}
 
@@ -52,11 +56,20 @@ class FirewalledResponse extends HttpServletResponseWrapper {
 		super.addHeader(name, value);
 	}
 
-	private void validateCRLF(String name, String value) {
+	@Override
-		if (CR_OR_LF.matcher(value).find()) {
+	public void addCookie(Cookie cookie) {
+		validateCRLF(SET_COOKIE_HEADER, cookie.getValue());
+		validateCRLF(SET_COOKIE_HEADER, cookie.getPath());
+		validateCRLF(SET_COOKIE_HEADER, cookie.getDomain());
+		validateCRLF(SET_COOKIE_HEADER, cookie.getComment());
+		super.addCookie(cookie);
+	}
+
+	void validateCRLF(String name, String value) {
+		if (name != null && CR_OR_LF.matcher(name).find()
+				|| value != null && CR_OR_LF.matcher(value).find()) {
 			throw new IllegalArgumentException(
 					"Invalid characters (CR/LF) in header " + name);
 		}
 	}
-
 }

web/src/test/java/org/springframework/security/web/firewall/FirewalledResponseTests.java

@@ -15,65 +15,158 @@
  */
 package org.springframework.security.web.firewall;
 
-import org.junit.Test;
-
-import org.springframework.mock.web.MockHttpServletResponse;
-
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.fail;
 
+import javax.servlet.http.Cookie;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.springframework.mock.web.MockHttpServletResponse;
+
 /**
  * @author Luke Taylor
  * @author Eddú Meléndez
+ * @author Gabriel Lavoie
  */
 public class FirewalledResponseTests {
+	private MockHttpServletResponse response = new MockHttpServletResponse();
+	private FirewalledResponse fwResponse = new FirewalledResponse(response);
+
+	@Rule
+	public ExpectedException expectedException = ExpectedException.none();
+
+	@Test
+	public void acceptRedirectLocationWithoutCRLF() throws Exception {
+		fwResponse.sendRedirect("/theURL");
+		assertThat(response.getRedirectedUrl()).isEqualTo("/theURL");
+	}
+
+	@Test
+	public void validateNullSafetyForRedirectLocation() throws Exception {
+		// Exception from MockHttpServletResponse, exception not described in servlet spec.
+		expectedException.expect(IllegalArgumentException.class);
+		expectedException.expectMessage("Redirect URL must not be null");
+
+		fwResponse.sendRedirect(null);
+	}
 
 	@Test
 	public void rejectsRedirectLocationContainingCRLF() throws Exception {
-		MockHttpServletResponse response = new MockHttpServletResponse();
+		expectedException.expect(IllegalArgumentException.class);
-		FirewalledResponse fwResponse = new FirewalledResponse(response);
+		expectedException.expectMessage("Invalid characters (CR/LF)");
 
-		fwResponse.sendRedirect("/theURL");
-		assertThat(response.getRedirectedUrl()).isEqualTo("/theURL");
-
-		try {
 		fwResponse.sendRedirect("/theURL\r\nsomething");
-			fail("IllegalArgumentException should have thrown");
-		}
-		catch (IllegalArgumentException expected) {
-		}
-		try {
-			fwResponse.sendRedirect("/theURL\rsomething");
-			fail("IllegalArgumentException should have thrown");
-		}
-		catch (IllegalArgumentException expected) {
-		}
-
-		try {
-			fwResponse.sendRedirect("/theURL\nsomething");
-			fail("IllegalArgumentException should have thrown");
-		}
-		catch (IllegalArgumentException expected) {
-		}
 	}
 
 	@Test
-	public void rejectHeaderContainingCRLF() {
+	public void acceptHeaderValueWithoutCRLF() throws Exception {
-		MockHttpServletResponse response = new MockHttpServletResponse();
+		fwResponse.addHeader("foo", "bar");
-		FirewalledResponse fwResponse = new FirewalledResponse(response);
+		assertThat(response.getHeader("foo")).isEqualTo("bar");
+	}
+
+	@Test
+	public void validateNullSafetyForHeaderValue() throws Exception {
+		// Exception from MockHttpServletResponse, exception not described in servlet spec.
+		expectedException.expect(IllegalArgumentException.class);
+		expectedException.expectMessage("Header value must not be null");
+
+		fwResponse.addHeader("foo", null);
+	}
+
+	@Test
+	public void rejectHeaderValueContainingCRLF() {
+		expectCRLFValidationException();
 
-		try {
 		fwResponse.addHeader("foo", "abc\r\nContent-Length:100");
-			fail("IllegalArgumentException should have thrown");
-		}
-		catch (IllegalArgumentException expected) {
-		}
-		try {
-			fwResponse.setHeader("foo", "abc\r\nContent-Length:100");
-			fail("IllegalArgumentException should have thrown");
-		}
-		catch (IllegalArgumentException expected) {
-		}
 	}
 
+	@Test
+	public void rejectHeaderNameContainingCRLF() {
+		expectCRLFValidationException();
+
+		fwResponse.addHeader("abc\r\nContent-Length:100", "bar");
+	}
+
+	@Test
+	public void acceptCookieWithoutCRLF() {
+		Cookie cookie = new Cookie("foo", "bar");
+		cookie.setPath("/foobar");
+		cookie.setDomain("foobar");
+		cookie.setComment("foobar");
+
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void rejectCookieNameContainingCRLF() {
+		// This one is thrown by the Cookie class constructor from javax.servlet-api,
+		// no need to cover in FirewalledResponse.
+		expectedException.expect(IllegalArgumentException.class);
+		Cookie cookie = new Cookie("foo\r\nbar", "bar");
+	}
+
+	@Test
+	public void rejectCookieValueContainingCRLF() {
+		expectCRLFValidationException();
+
+		Cookie cookie = new Cookie("foo", "foo\r\nbar");
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void rejectCookiePathContainingCRLF() {
+		expectCRLFValidationException();
+
+		Cookie cookie = new Cookie("foo", "bar");
+		cookie.setPath("/foo\r\nbar");
+
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void rejectCookieDomainContainingCRLF() {
+		expectCRLFValidationException();
+
+		Cookie cookie = new Cookie("foo", "bar");
+		cookie.setDomain("foo\r\nbar");
+
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void rejectCookieCommentContainingCRLF() {
+		expectCRLFValidationException();
+
+		Cookie cookie = new Cookie("foo", "bar");
+		cookie.setComment("foo\r\nbar");
+
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void rejectAnyLineEndingInNameAndValue() {
+		validateLineEnding("foo", "foo\rbar");
+		validateLineEnding("foo", "foo\r\nbar");
+		validateLineEnding("foo", "foo\nbar");
+
+		validateLineEnding("foo\rbar", "bar");
+		validateLineEnding("foo\r\nbar", "bar");
+		validateLineEnding("foo\nbar", "bar");
+	}
+
+	private void expectCRLFValidationException() {
+		expectedException.expect(IllegalArgumentException.class);
+		expectedException.expectMessage("Invalid characters (CR/LF)");
+	}
+
+	private void validateLineEnding(String name, String value) {
+		try {
+			fwResponse.validateCRLF(name, value);
+			fail("IllegalArgumentException should have thrown");
+		}
+		catch (IllegalArgumentException expected) {
+		}
+	}
 }