Use raw url parameters in redirect signature validation

This patch implements the 'use original URL-encoded values it receives
on the query string' to verify the signature when http redirect is used.

We encountered this problem when integrating with Microsoft ADFS 2.0.
The server uses UPPERCASE in url encodings, this conflicts with the
lowercase url encoding used here (ex %2B vs %2b).

Solution
Modified the HttpRequest class to contain the original query string.
Extracts the raw url paramters using the getEncodedParamter().
This solution is inspired by the python implementation.

Added test cases to demonstate handling of different signatures based
on the url encoding used.

From saml-bindings-2.0 (http://docs.oasis-open.org/security/saml/v2.0/)

3.4.4.1 DEFLATE Encoding
Identification: urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE

To construct the signature, a string consisting of the concatenation
of the RelayState (if present), SigAlg, and SAMLRequest (or SAMLResponse)
query string parameters (each one URLencoded) is constructed in one of the
following ways (ordered as below):
SAMLRequest=value&RelayState=value&SigAlg=value
SAMLResponse=value&RelayState=value&SigAlg=value

Further, note that URL-encoding is not canonical; that is, there are
multiple legal encodings for a given value. The relying party MUST
therefore perform the verification step using the original URL-encoded
values it received on the query string. It is not sufficient to
re-encode the parameters after they have been processed by software
because the resulting encoding may not match the signer's encoding

Finally, note that if there is no RelayState value, the entire parameter
should be omitted from the signature computation (and not included as
an empty parameter name).

Author: merit\rembjo0

core/src/main/java/com/onelogin/saml2/http/HttpRequest.java

@@ -10,15 +10,25 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.apache.commons.lang3.StringUtils;
+
+import com.onelogin.saml2.util.Util;
 
 /**
  * Framework-agnostic representation of an HTTP request.
  *
  * @since 2.0.0
  */
 public final class HttpRequest {
+	
+	public static final Map<String, List<String>> EMPTY_PARAMETERS = Collections.<String, List<String>>emptyMap(); 
+	
     private final String requestURL;
     private final Map<String, List<String>> parameters;
+    private final String queryString;
 
     /**
      * Creates a new HttpRequest.
@@ -27,9 +37,19 @@ public final class HttpRequest {
      * @throws NullPointerException if requestURL is null
      */
     public HttpRequest(String requestURL) {
-        this(requestURL, Collections.<String, List<String>>emptyMap());
+        this(requestURL, EMPTY_PARAMETERS);
     }
 
+    /**
+     * Creates a new HttpRequest.
+     *
+     * @param requestURL  the request URL (up to but not including query parameters)
+     * @param query string that is contained in the request URL after the path
+     */
+    public HttpRequest(String requestURL, String queryString) {
+    	this(requestURL, EMPTY_PARAMETERS, queryString);
+    }
+    
     /**
      * Creates a new HttpRequest.
      *
@@ -38,10 +58,24 @@ public HttpRequest(String requestURL) {
      * @throws NullPointerException if any of the parameters is null
      */
     public HttpRequest(String requestURL, Map<String, List<String>> parameters) {
+    	this(requestURL, parameters, null);
+    }  
+      
+    /**
+     * Creates a new HttpRequest.
+     *
+     * @param requestURL  the request URL (up to but not including query parameters)
+     * @param parameters the request query parameters
+     * @param query string that is contained in the request URL after the path
+     * @throws NullPointerException if any of the parameters is null
+     */
+    public HttpRequest(String requestURL, Map<String, List<String>> parameters, String queryString) {
         this.requestURL = checkNotNull(requestURL, "requestURL");
         this.parameters = unmodifiableCopyOf(checkNotNull(parameters, "queryParams"));
+        this.queryString = StringUtils.trimToEmpty(queryString);
     }
 
+    
     /**
      * @param name  the query parameter name
      * @param value the query parameter value
@@ -58,7 +92,7 @@ public HttpRequest addParameter(String name, String value) {
         final Map<String, List<String>> params = new HashMap<>(parameters);
         params.put(name, newValues);
 
-        return new HttpRequest(requestURL, params);
+        return new HttpRequest(requestURL, params, queryString);
     }
 
     /**
@@ -72,7 +106,7 @@ public HttpRequest removeParameter(String name) {
         final Map<String, List<String>> params = new HashMap<>(parameters);
         params.remove(name);
 
-        return new HttpRequest(requestURL, params);
+        return new HttpRequest(requestURL, params, queryString);
     }
 
     /**
@@ -110,6 +144,38 @@ public Map<String, List<String>> getParameters() {
         return parameters;
     }
 
+    /**
+     * Return an url encoded get parameter value
+     * Prefer to extract the original encoded value directly from queryString since url
+     * encoding is not canonical.
+     * 
+     * @param name
+     * @return the first value for the parameter, or null
+     */
+    public String getEncodedParameter(String name) {    	
+    	Matcher matcher = Pattern.compile(Pattern.quote(name) + "=([^&]+)").matcher(queryString);
+    	if (matcher.find()) {
+    		return matcher.group(1);
+    	} else {
+    		return Util.urlEncoder(getParameter(name));
+    	}
+    }
+    
+    /**
+     * Return an url encoded get parameter value
+     * Prefer to extract the original encoded value directly from queryString since url
+     * encoding is not canonical.
+     * 
+     * @param name
+     * @param defaultValue 
+     * @return the first value for the parameter, or url encoded default value 
+     */
+	public String getEncodedParameter(String name, String defaultValue) {
+		String value = getEncodedParameter(name);
+		return (value != null ? value : Util.urlEncoder(defaultValue));
+	}
+
+    
     @Override
     public boolean equals(Object o) {
         if (this == o) {
@@ -122,19 +188,21 @@ public boolean equals(Object o) {
 
         HttpRequest that = (HttpRequest) o;
         return Objects.equals(requestURL, that.requestURL) &&
-                Objects.equals(parameters, that.parameters);
+                Objects.equals(parameters, that.parameters) &&
+                Objects.equals(queryString, this.queryString);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(requestURL, parameters);
+        return Objects.hash(requestURL, parameters, queryString);
     }
 
     @Override
     public String toString() {
         return "HttpRequest{" +
                 "requestURL='" + requestURL + '\'' +
                 ", parameters=" + parameters +
+                ", queryString=" + queryString + 
                 '}';
     }
 
@@ -146,4 +214,6 @@ private static Map<String, List<String>> unmodifiableCopyOf(Map<String, List<Str
 
         return unmodifiableMap(copy);
     }
+
+
 }

core/src/main/java/com/onelogin/saml2/logout/LogoutRequest.java

@@ -334,16 +334,16 @@ public Boolean isValid() throws Exception {
 				if (signAlg == null || signAlg.isEmpty()) {
 					signAlg = Constants.RSA_SHA1;
 				}
-				String relayState = request.getParameter("RelayState");
+				String relayState = request.getEncodedParameter("RelayState");
 
-				String signedQuery = "SAMLRequest=" + Util.urlEncoder(request.getParameter("SAMLRequest"));
+				String signedQuery = "SAMLRequest=" + request.getEncodedParameter("SAMLRequest");
 
 				if (relayState != null && !relayState.isEmpty()) {
-					signedQuery += "&RelayState=" + Util.urlEncoder(relayState);
+					signedQuery += "&RelayState=" + relayState;
 				}
 
-				signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);
-
+				signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);
+				
 				if (!Util.validateBinarySignature(signedQuery, Util.base64decoder(signature), cert, signAlg)) {
 					throw new ValidationError("Signature validation failed. Logout Request rejected", ValidationError.INVALID_SIGNATURE);
 				}

core/src/main/java/com/onelogin/saml2/logout/LogoutResponse.java

@@ -220,15 +220,15 @@ public Boolean isValid(String requestId) {
 					signAlg = Constants.RSA_SHA1;
 				}
 
-				String signedQuery = "SAMLResponse=" + Util.urlEncoder(request.getParameter("SAMLResponse"));
+				String signedQuery = "SAMLResponse=" + request.getEncodedParameter("SAMLResponse");
 
-				String relayState = request.getParameter("RelayState");
+				String relayState = request.getEncodedParameter("RelayState");
 				if (relayState != null && !relayState.isEmpty()) {
-					signedQuery += "&RelayState=" + Util.urlEncoder(relayState);
+					signedQuery += "&RelayState=" + relayState;
 				}
 
-				signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);
-
+				signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);
+				
 				if (!Util.validateBinarySignature(signedQuery, Util.base64decoder(signature), cert, signAlg)) {
 					throw new ValidationError("Signature validation failed. Logout Response rejected", ValidationError.INVALID_SIGNATURE);
 				}

core/src/test/java/com/onelogin/saml2/http/HttpRequestTest.java

@@ -10,11 +10,15 @@
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
 import org.junit.Test;
 
+import com.onelogin.saml2.test.NaiveUrlEncoder;
+import com.onelogin.saml2.util.Util;
+
 public class HttpRequestTest {
     @Test
     public void testConstructorWithNoQueryParams() throws Exception {
@@ -43,7 +47,7 @@ public void testConstructorWithQueryParams() throws Exception {
         assertThat(request.getParameters(name), equalTo(values));
         assertThat(request.getParameter(name), equalTo(value1));
     }
-
+    
     @Test
     public void testAddParameter() throws Exception {
         final String url = "some_url";
@@ -55,6 +59,9 @@ public void testAddParameter() throws Exception {
         assertThat(request.getParameters(), equalTo(singletonMap(name, singletonList(value))));
         assertThat(request.getParameters(name), equalTo(singletonList(value)));
         assertThat(request.getParameter(name), equalTo(value));
+        
+        final HttpRequest request2 = request.addParameter(name, value);
+        assertThat(request2.getParameters(name), equalTo(Arrays.asList(value, value)));
     }
 
     @Test
@@ -75,4 +82,107 @@ public void testRemoveParameter() throws Exception {
         assertTrue(request.getParameters(name).isEmpty());
         assertNull(request.getParameter(name));
     }
+    
+    @Test
+    public void testGetEncodedParameter_encodesParametersNotOnQueryString() throws Exception {
+        final String url = "url";
+        final String name = "name";
+        final String value1 = "val/1!";
+        final String addedName = "added";
+        final String addedValue = "added#value!";
+             
+        final List<String> values = Arrays.asList(value1);
+        final Map<String, List<String>> parametersMap = singletonMap(name, values);
+
+		final HttpRequest request = new HttpRequest(url, parametersMap).addParameter(addedName, addedValue);
+        
+        assertThat(request.getEncodedParameter(name), equalTo(Util.urlEncoder(value1)));
+        assertThat(request.getEncodedParameter(addedName), equalTo(Util.urlEncoder(addedValue)));        
+    }    
+
+    @Test
+    public void testGetEncodedParameter_prefersValueFromQueryString() throws Exception {
+        final String url = "url";
+        final String name = "name";
+        final String value1 = "value1";
+        final String urlValue1 = "onUrl1";
+		final String queryString = name + "=" + urlValue1;
+ 
+        final List<String> values = Arrays.asList(value1);
+        final Map<String, List<String>> parametersMap = singletonMap(name, values);
+
+		final HttpRequest request = new HttpRequest(url, parametersMap, queryString);
+        		
+        assertThat(request.getEncodedParameter(name), equalTo(urlValue1));
+        assertThat(request.getParameter(name), equalTo(value1));
+    }    
+
+    @Test
+    public void testGetEncodedParameter_returnsExactAsGivenInQueryString() throws Exception {
+        final String url = "url";
+        final String name = "name";
+        String encodedValue1 = NaiveUrlEncoder.encode("do not alter!");
+		final String queryString = name + "=" + encodedValue1;
+ 
+		final HttpRequest request = new HttpRequest(url, queryString);
+        		
+        assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
+    }     
+    
+    @Test
+    public void testGetEncodedParameter_handlesMultipleValuesOnQueryString() throws Exception {
+		final String url = "url";
+    	final String queryString = "k1=v1&k2=v2&k3=v3";
+ 
+        final Map<String, List<String>> parametersMap = new HashMap<>();
+		final HttpRequest request = new HttpRequest(url, parametersMap, queryString);
+        		
+        assertThat(request.getEncodedParameter("k1"), equalTo("v1"));
+        assertThat(request.getEncodedParameter("k2"), equalTo("v2"));
+        assertThat(request.getEncodedParameter("k3"), equalTo("v3"));
+    }     
+    
+    
+    @Test
+    public void testGetEncodedParameter_withDefault_usesDefaultWhenParameterMissing() throws Exception {
+		final String url = "url";
+		final String foobar = "foo/bar!";
+		
+		final HttpRequest request = new HttpRequest(url);        		
+        assertThat(request.getEncodedParameter("missing", foobar), equalTo(Util.urlEncoder(foobar)));
+    }     
+    
+    
+    @Test
+    public void testAddParameter_preservesQueryString() throws Exception {
+        final String url = "url";
+        final String name = "name";
+        final String value1 = "val/1!";
+        String encodedValue1 = NaiveUrlEncoder.encode(value1);
+		final String queryString = name + "=" + encodedValue1;
+ 
+        final Map<String, List<String>> parametersMap = new HashMap<>();
+		final HttpRequest request = new HttpRequest(url, parametersMap, queryString).addParameter(name, value1);
+        		
+        assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
+    }  
+
+    @Test
+    public void testRemoveParameter_preservesQueryString() throws Exception {
+        final String url = "url";
+        final String name = "name";
+        final String value1 = "val/1!";
+        String encodedValue1 = NaiveUrlEncoder.encode(value1);
+		final String queryString = name + "=" + encodedValue1;
+ 
+        final List<String> values = Arrays.asList(value1);
+        final Map<String, List<String>> parametersMap = singletonMap(name, values);
+
+        final HttpRequest request = new HttpRequest(url, parametersMap, queryString).removeParameter(name);
+        		
+        assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
+    }  
+ 
+
+    
 }

core/src/test/java/com/onelogin/saml2/test/NaiveUrlEncodeTest.java

@@ -0,0 +1,24 @@
+package com.onelogin.saml2.test;
+
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.onelogin.saml2.util.Util;
+
+public class NaiveUrlEncodeTest {
+	
+	@Test
+	public void testDemonstratingUrlEncodingNotCanonical () throws UnsupportedEncodingException {
+		String theString = "Hello World!";
+		
+		String naiveEncoded = NaiveUrlEncoder.encode(theString);
+		String propperEncoded = Util.urlEncoder(theString);
+		
+		Assert.assertNotEquals("Encoded versions should differ", naiveEncoded, propperEncoded);
+		Assert.assertEquals("Decoded versions equal", URLDecoder.decode(naiveEncoded, "UTF-8"), URLDecoder.decode(propperEncoded, "UTF-8"));
+	}
+	
+}

core/src/test/java/com/onelogin/saml2/test/NaiveUrlEncoder.java

@@ -0,0 +1,24 @@
+package com.onelogin.saml2.test;
+
+import java.io.UnsupportedEncodingException;
+
+public final  class NaiveUrlEncoder {
+
+	private NaiveUrlEncoder () {
+		//static class only
+	}
+	
+	/**
+	 * Encodes ALL characters in URL using %xx encoding.<br>
+	 * This differs from 'normal' url encoding.  
+	 */
+	public static String encode (String s) throws UnsupportedEncodingException {
+		StringBuilder buf = new StringBuilder();
+		for  (byte b : s.getBytes("UTF-8")) {
+			buf.append("%");
+			buf.append(String.format("%02x", b));
+		}
+		return buf.toString();
+	}
+	
+}