From 4a1f00b90f540429a924a14c5d64dd2b006e8d2a Mon Sep 17 00:00:00 2001 From: Gabriel Lavoie Date: Wed, 21 Sep 2016 10:01:50 -0500 Subject: [PATCH] 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/firewall/FirewalledResponse.java | 27 ++- .../web/firewall/FirewalledResponseTests.java | 171 ++++++++++++++---- 2 files changed, 152 insertions(+), 46 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java b/web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java index 640172012d..8d0349ec4a 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java +++ b/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) { - if (CR_OR_LF.matcher(value).find()) { + @Override + 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); } } - } diff --git a/web/src/test/java/org/springframework/security/web/firewall/FirewalledResponseTests.java b/web/src/test/java/org/springframework/security/web/firewall/FirewalledResponseTests.java index c1af03ddc9..2af6cff1ba 100644 --- a/web/src/test/java/org/springframework/security/web/firewall/FirewalledResponseTests.java +++ b/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(); - FirewalledResponse fwResponse = new FirewalledResponse(response); + expectedException.expect(IllegalArgumentException.class); + 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) { - } + fwResponse.sendRedirect("/theURL\r\nsomething"); } @Test - public void rejectHeaderContainingCRLF() { - MockHttpServletResponse response = new MockHttpServletResponse(); - FirewalledResponse fwResponse = new FirewalledResponse(response); + public void acceptHeaderValueWithoutCRLF() throws Exception { + fwResponse.addHeader("foo", "bar"); + 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(); + + fwResponse.addHeader("foo", "abc\r\nContent-Length:100"); + } + + @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.addHeader("foo", "abc\r\nContent-Length:100"); - fail("IllegalArgumentException should have thrown"); - } - catch (IllegalArgumentException expected) { - } - try { - fwResponse.setHeader("foo", "abc\r\nContent-Length:100"); + fwResponse.validateCRLF(name, value); fail("IllegalArgumentException should have thrown"); } catch (IllegalArgumentException expected) { } } - }