Use raw url parameters in redirect signature validation by rembjo0 · Pull Request #94 · SAML-Toolkits/java-saml

rembjo0 · 2017-02-04T14:55:28Z

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
Added extra parameter to the classes where the raw parameters were needed.
Basically, just a map containing the raw URL parameters. Keyed on the name
and with key=value as the value. This is to preserve all variations
like foo=bar, blank= and empty (ex foo=bar&blank=&empty).

Existing HttpRequest abstraction not modified, as the use of
raw url parameters is unusual in most situations.

Note: The SigAlg parameter is now mandatory, test modified.
There was a default SigAlg added to the url in the verification step
when missing. This conflicted with the raw url parameters solution.

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).

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 Added extra parameter to the classes where the raw parameters were needed. Basically, just a map containing the raw URL parameters. Keyed on the name and with key=value as the value. This is to preserve all variations like foo=bar, blank= and empty (ex foo=bar&blank=&empty). Existing HttpRequest abstraction not modified, as the use of raw url parameters is unusual in most situations. Note: The SigAlg parameter is now mandatory, test modified. There was a default SigAlg added to the url in the verification step when missing. This conflicted with the raw url parameters solution. 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).

coveralls · 2017-02-04T14:57:59Z

Coverage decreased (-0.05%) to 95.898% when pulling 14b1d78 on rembjo0:topic-redirect-signature-verifiy into 560d41d on onelogin:master.

miszobi · 2017-02-05T07:58:16Z

Maybe a nicer way to introduce this would be to add a method to retrieve the un-decoded query string to HttpRequest (similar to HttpServletRequest.getQueryString()). That way we could provide a more consistent api, and handle the need for the undecoded query during validation, rather than being forced to pass it as an extra parameter. I think this would minimize signature changes (which could break existing usage, even if this specific case is not needed) and make for a cleaner api.

Would also be nice to see a test that breaks because of not using the raw query string (for example a case difference when urldecoding/encoding), to avoid a regression here, and explain the requirement cleanly.

miszobi · 2017-02-05T07:59:09Z

-				String relayState = request.getParameter("RelayState");
-				if (relayState != null && !relayState.isEmpty()) {
-					signedQuery += "&RelayState=" + Util.urlEncoder(relayState);
+				String rawSamlResponse = rawRequestParams.get("SAMLResponse");


Maybe extract a helper method that extracts the signed query and share that between LogoutResponse and LogoutRequest

miszobi · 2017-02-05T08:02:42Z

+
 				String signAlg = request.getParameter("SigAlg");
 				if (signAlg == null || signAlg.isEmpty()) {
 					signAlg = Constants.RSA_SHA1;


If this is required maybe make it fail with an explicit error here?

rembjo0 · 2017-02-05T18:50:10Z

Once again I am impressed by the speedy response! Thanks for taking the time to consider and review. I figured a pull request would show that I had taken the effort to understand the problem.

Very good points in the review.

A test case for URL encoding variations makes perfect sense.

I also noted the duplicate code in the validate methods, some refactoring there would be a good thing indeed.

The now mandatory SigAlg parameter was solved in weird way. The algorithm was added to the url string for verification (sha-1 default). Thus, it was part of the calculated signature received, just not passed on the url? This could possibly be related to some real-world scenario I guess (hope not!). If the mandatory variant is ok, then improved error handling makes perfect sense. Per the specs, only the RelayState is optional.

I am still uncertain about the best way to pass the raw url to the validate methods. This requirement only affects them. My original solution modified the HttpRequest object as suggested above. Feels like a natural thing to do until you try. It was messy. If the HttpRequest was immutable, then it would make sense. The fact that you can add/remove parameters caused some friction. Should the raw url string be modified? I don’t know. And again, only two validate methods actually care.

In hindsight, modifying the constructors was probably not the best solution. Maybe it would have been better to add a new validate method with the raw-url parameter. The existing validate method could just create a ‘raw url’ object internally based on urlencode. This would behave just like the original code in regards to encoding. Nobody would have to change anything if they use these objects directly. The Auth class would of course wire it up properly using the raw url.

Tempting! This would certainly reduce the impact of the patch. Changing the constructor was a bit of a pain (affected test wiring mostly).

Let me know what you think. I can change the pull request, but I don’t mind if you scrap it either, as long as the url-encoding-verification issue is fixed. I’ll gladly leave this fix to the proper maintainers.

pitbulk · 2017-02-06T09:22:57Z

This is how I fixed that issue on python-saml:

SAML-Toolkits/python-saml@7fc8370

and at php-saml:

I think @miszobi had good suggestions to solve that issue. are you able to improve that PR?

rembjo0 · 2017-02-06T10:22:45Z

@pitbulk & @miszobi, nice solutions! That is what I want in the Java implementation. Adding the getEncoded method on the HttpRequest with fallback to the 'raw' value looks good. I will give that a try. Will also add a test case for different url encodings & signature validation.

This reverts commit 14b1d78.

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).

coveralls · 2017-02-06T19:54:03Z

Coverage increased (+0.02%) to 95.968% when pulling e1a9d09 on rembjo0:topic-redirect-signature-verifiy into 560d41d on onelogin:master.

rembjo0 · 2017-02-07T09:03:02Z

New version, complete rewrite. Added queryString and getEncodedParameter() To HttpRequest. Code is inspired by Python version. This approach required change in existing tests cases.

Added a new config file with Idb certificate and known Idp private key. Could not find the private key for the existing config files. Generated fixed signatures for the tests, just followed the pattern in the other tests.

The tests include a NaiveUrlEncoder. This encodes all characters to %xx and is used to show that the new code handles different encodings.

miszobi

Nice one. Really prefer this approach. Looks good to me overall.

miszobi · 2017-02-08T21:32:11Z

 */
 public final class HttpRequest {
+
+	public static final Map<String, List<String>> EMPTY_PARAMETERS = Collections.<String, List<String>>emptyMap(); 


nit: minor indentation inconsistency here and some in some other spots. It's tricky as the code is partially formatted using tabs, partially spaces.

Ah, whitespace. That was sloppy of me. I'm used to auto-format-on-save in my normal setup. I can commit a cleanup patch. Just fixing the added stuff, adapting to the formatting used in each file.

miszobi · 2017-02-08T21:39:53Z

+     */
+    public String getEncodedParameter(String name) {    	
+    	Matcher matcher = Pattern.compile(Pattern.quote(name) + "=([^&]+)").matcher(queryString);
+    	if (matcher.find()) {


nit: was gonna point out this should stop matching the parameter value at # to avoid including the fragment value, but probably all the querystrings passed here wouldn't have fragments.

Adding # makes sense. It's not forbidden by the spec, so supporting it sounds like a good idea.

miszobi · 2017-02-08T21:43:34Z

@@ -27,9 +37,19 @@ public final class HttpRequest {
     * @throws NullPointerException if requestURL is null
     */
    public HttpRequest(String requestURL) {


Maybe we should deprecate this constructor, pointing to the other one, and mentioning that not providing the queryString can cause issues with signature verification when using the HTTP Redirect binding? @pitbulk?

👍 I'll add the deprecate.

Constructors without queryString deprecated.

miszobi · 2017-02-08T21:54:11Z


-				signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);
-
+				signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);


This still allows an not specifying the SigAlg parameter. The spec says this MUST be included. I'd prefer checking and requiring that here, rather than falling back to the default. @pitbulk, any potential reason not to?

In the other toolkits, when SigAlg is not provided, I assume that RSA_SHA1 was used (since it was the default alg in all implementations.

The spec is clear:

The signature algorithm identifier MUST be included as an additional query string parameter, named SigAlg. The value of this parameter MUST be a URI that identifies the algorithm used to sign the URL-encoded SAML protocol message, specified according to [XMLSig] or whatever specification governs the algorithm.

I tried to be flexible. At the end, if it wasn't the original SigAlg, the validation gonna fail, but maybe introducing that will prevent us to detect in the future signature validation fails.

I was quite pleased when the new patch preserved this logic. My concern was that this potentially was based on a real-world case. Would it make sense if this was addressed in a separate patch/issue that describes the change and reasoning? And perhaps consolidate the various implementations at the same time?

This patch modifies whitespace in areas touched by the previous commit. Attempts to use the same whitespace as the rest of the file. This minimizes clutter in the diff and should improve formatting.

coveralls · 2017-02-11T13:06:51Z

Coverage decreased (-0.4%) to 95.553% when pulling b93d1df on rembjo0:topic-redirect-signature-verifiy into 560d41d on onelogin:master.

HttpRequest.getEncodedParameter() will now return the corrent value for the last parameter before the #. Example: foo=bar#ignore now returns 'bar' and not 'bar#ignore' for the 'foo' parameter.

coveralls · 2017-02-11T13:34:10Z

Coverage decreased (-0.4%) to 95.553% when pulling bc0b129 on rembjo0:topic-redirect-signature-verifiy into 560d41d on onelogin:master.

HTTP Redirect binding can fail if the queryString is ommited as URL encoding is non-canonical.

coveralls · 2017-02-11T13:46:33Z

Coverage decreased (-0.4%) to 95.553% when pulling ca7b2d7 on rembjo0:topic-redirect-signature-verifiy into 560d41d on onelogin:master.

rembjo0 · 2017-02-15T07:46:08Z

Thanks for the merges @pitbulk & @miszobi 😄

pitbulk · 2017-02-15T08:10:00Z

Thanks you for contribute.

miszobi reviewed Feb 5, 2017

View reviewed changes

merit\rembjo0 added 2 commits February 6, 2017 20:00

Reverts first attempt back to plain vanilla

5ad511c

This reverts commit 14b1d78.

miszobi approved these changes Feb 8, 2017

View reviewed changes

Fix whitespace in source files

b93d1df

This patch modifies whitespace in areas touched by the previous commit. Attempts to use the same whitespace as the rest of the file. This minimizes clutter in the diff and should improve formatting.

Adds # support in HttpRequest query string parsing

bc0b129

HttpRequest.getEncodedParameter() will now return the corrent value for the last parameter before the #. Example: foo=bar#ignore now returns 'bar' and not 'bar#ignore' for the 'foo' parameter.

Deprecate HttpRequest constructors without queryString

ca7b2d7

HTTP Redirect binding can fail if the queryString is ommited as URL encoding is non-canonical.

pitbulk merged commit 37b0ed4 into SAML-Toolkits:master Feb 14, 2017

rembjo0 deleted the topic-redirect-signature-verifiy branch February 15, 2017 07:38


		signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);

		signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);

Conversation

rembjo0 commented Feb 4, 2017

Uh oh!

coveralls commented Feb 4, 2017

Uh oh!

miszobi commented Feb 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rembjo0 commented Feb 5, 2017

Uh oh!

pitbulk commented Feb 6, 2017

Uh oh!

rembjo0 commented Feb 6, 2017

Uh oh!

coveralls commented Feb 6, 2017

Uh oh!

rembjo0 commented Feb 7, 2017

Uh oh!

miszobi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitbulk Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 11, 2017

Uh oh!

coveralls commented Feb 11, 2017

Uh oh!

coveralls commented Feb 11, 2017

Uh oh!

rembjo0 commented Feb 15, 2017

Uh oh!

pitbulk commented Feb 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitbulk Feb 10, 2017 •

edited

Loading