SEC-1790: Reject redirect locations containing CR or LF.

.gitignore

@@ -6,6 +6,7 @@ out/
 *.ipr
 *.iml
 *.iws
+*.log
 intellij/
 .settings
 .classpath

core/src/main/java/org/springframework/security/firewall/DefaultHttpFirewall.java

@@ -33,7 +33,7 @@ public class DefaultHttpFirewall implements HttpFirewall {
     }
 
     public HttpServletResponse getFirewalledResponse(HttpServletResponse response) {
-        return response;
+        return new FirewalledResponse(response);
     }
 
     /**

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

@@ -0,0 +1,26 @@
+package org.springframework.security.firewall;
+
+import javax.servlet.http.HttpServletResponseWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.regex.Pattern;
+
+/**
+ * @author Luke Taylor
+ */
+class FirewalledResponse extends HttpServletResponseWrapper {
+    Pattern CR_OR_LF = Pattern.compile("\\r|\\n");
+
+    public FirewalledResponse(HttpServletResponse response) {
+        super(response);
+    }
+
+    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");
+        }
+        super.sendRedirect(location);
+    }
+}

core/src/main/java/org/springframework/security/ui/TargetUrlResolverImpl.java

@@ -28,38 +28,37 @@ import org.springframework.util.StringUtils;
 
 /**
  * Default implementation for {@link TargetUrlResolver}
- * <p>
+ * <p/>
- * Returns a target URL based from the contents of the configured <tt>targetUrlParameter</tt> if present on 
+ * Returns a target URL based from the contents of the configured <tt>targetUrlParameter</tt> if present on
- * the current request. Failing that, the SavedRequest in the session will be used. 
+ * the current request. Failing that, the SavedRequest in the session will be used.
- * 
+ *
  * @author Martino Piccinato
  * @author Luke Taylor
  * @version $Id$
  * @since 2.0
- *
  */
 public class TargetUrlResolverImpl implements TargetUrlResolver {
     public static String DEFAULT_TARGET_PARAMETER = "spring-security-redirect";
-    
+
     /* SEC-213 */
     private String targetUrlParameter = DEFAULT_TARGET_PARAMETER;
-	
+
-	/**
+    /**
-	 * If <code>true</code>, will only use <code>SavedRequest</code> to determine the target URL on successful
+     * If <code>true</code>, will only use <code>SavedRequest</code> to determine the target URL on successful
      * authentication if the request that caused the authentication request was a GET.
      * It will then return null for a POST/PUT request.
      * Defaults to false.
-	 */
+     */
-	private boolean justUseSavedRequestOnGet = false;
+    private boolean justUseSavedRequestOnGet = false;
 
     /* (non-Javadoc)
-	 * @see org.acegisecurity.ui.TargetUrlResolver#determineTargetUrl(org.acegisecurity.ui.savedrequest.SavedRequest, javax.servlet.http.HttpServletRequest, org.acegisecurity.Authentication)
+     * @see org.acegisecurity.ui.TargetUrlResolver#determineTargetUrl(org.acegisecurity.ui.savedrequest.SavedRequest, javax.servlet.http.HttpServletRequest, org.acegisecurity.Authentication)
-	 */
+     */
-	public String determineTargetUrl(SavedRequest savedRequest, HttpServletRequest currentRequest,
+    public String determineTargetUrl(SavedRequest savedRequest, HttpServletRequest currentRequest,
-            Authentication auth) {
+                                     Authentication auth) {
 
         String targetUrl = currentRequest.getParameter(targetUrlParameter);
-        
+
         if (StringUtils.hasText(targetUrl)) {
             try {
                 return URLDecoder.decode(targetUrl, "UTF-8");
@@ -75,35 +74,34 @@ public class TargetUrlResolverImpl implements TargetUrlResolver {
         }
 
         return targetUrl;
-	}
+    }
-
+
-	/**
+    /**
-	 * @return <code>true</code> if just GET request will be used
+     * @return <code>true</code> if just GET request will be used
-	 * to determine target URLs, <code>false</code> otherwise.
+     *         to determine target URLs, <code>false</code> otherwise.
-	 */
+     */
-	protected boolean isJustUseSavedRequestOnGet() {
+    protected boolean isJustUseSavedRequestOnGet() {
-		return justUseSavedRequestOnGet;
+        return justUseSavedRequestOnGet;
-	}
+    }
-
+
-	/**
+    /**
-	 * @param justUseSavedRequestOnGet set to <code>true</code> if 
+     * @param justUseSavedRequestOnGet set to <code>true</code> if
-	 * just GET request will be used to determine target URLs, 
+     *                                 just GET request will be used to determine target URLs,
-	 * <code>false</code> otherwise.
+     *                                 <code>false</code> otherwise.
-	 */
+     */
-	public void setJustUseSavedRequestOnGet(boolean justUseSavedRequestOnGet) {
+    public void setJustUseSavedRequestOnGet(boolean justUseSavedRequestOnGet) {
-		this.justUseSavedRequestOnGet = justUseSavedRequestOnGet;
+        this.justUseSavedRequestOnGet = justUseSavedRequestOnGet;
-	}
+    }
-
+
-    
+    /**
-	/**
+     * Before checking the SavedRequest, the current request will be checked for this parameter
-	 * Before checking the SavedRequest, the current request will be checked for this parameter
+     * and the value used as the target URL if resent.
-	 * and the value used as the target URL if resent.
+     *
-	 * 
+     * @param targetUrlParameter the name of the parameter containing the encoded target URL. Defaults
-	 *  @param targetUrlParameter the name of the parameter containing the encoded target URL. Defaults
+     *                           to "redirect".
-	 *  to "redirect".
+     */
-	 */
+    public void setTargetUrlParameter(String targetUrlParameter) {
-	public void setTargetUrlParameter(String targetUrlParameter) {
+        Assert.hasText("targetUrlParameter cannot be null or empty");
-	    Assert.hasText("targetUrlParamete canot be null or empty");
         this.targetUrlParameter = targetUrlParameter;
     }
 }