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 028acc0556..640172012d 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 @@ -22,8 +22,10 @@ import java.util.regex.Pattern; /** * @author Luke Taylor + * @author Eddú Meléndez */ class FirewalledResponse extends HttpServletResponseWrapper { + private static final Pattern CR_OR_LF = Pattern.compile("\\r|\\n"); public FirewalledResponse(HttpServletResponse response) { @@ -34,10 +36,27 @@ 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 - if (CR_OR_LF.matcher(location).find()) { - throw new IllegalArgumentException( - "Invalid characters (CR/LF) in redirect location"); - } + validateCRLF("Location", location); super.sendRedirect(location); } + + @Override + public void setHeader(String name, String value) { + validateCRLF(name, value); + super.setHeader(name, value); + } + + @Override + public void addHeader(String name, String value) { + validateCRLF(name, value); + super.addHeader(name, value); + } + + private void validateCRLF(String name, String value) { + if (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 7181e839c1..c1af03ddc9 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,19 +15,21 @@ */ 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 org.junit.*; -import org.springframework.mock.web.MockHttpServletResponse; - /** * @author Luke Taylor + * @author Eddú Meléndez */ public class FirewalledResponseTests { @Test - public void rejectsRedirectLocationContaingCRLF() throws Exception { + public void rejectsRedirectLocationContainingCRLF() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); FirewalledResponse fwResponse = new FirewalledResponse(response); @@ -54,4 +56,24 @@ public class FirewalledResponseTests { catch (IllegalArgumentException expected) { } } + + @Test + public void rejectHeaderContainingCRLF() { + MockHttpServletResponse response = new MockHttpServletResponse(); + FirewalledResponse fwResponse = new FirewalledResponse(response); + + 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) { + } + } + }