Describe the bug
After upgrading the Spring Security to version 5.8.0 signature validation of the SAML Logout Response won't work.
According to the SAML specification order of query params for signature should be:
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
But in the Spring Security 5.8.0/6.0.0, content is just query parameters without the Signature param and without care about mentioned above order of params.
The issue was caused by this change.
To Reproduce
IdP redirects to the URL where the query param RelayState is not in the middle e.g.
SAMLResponse=someContentHere&SigAlg=http://www.w3.org/2000/09/xmldsig#rsa-sha1&RelayState=a7d72218-c3e6-44b1-b3ba-e0dbca8ea8a9
Example IdP: OneLogin
Expected behavior Content for signature validation should be ordered as described in the SAML 2.0 specification. Also, encoded query param values (SigAlg, RelayState, SAMLRequest, etc) should be unchanged as described here.
Comment From: jzheaux
Hi, @lukaszmigdalek. There are some details I'm not quite following just yet.
IdP redirects to the URL where the query param RelayState is not in the middle
This sounds like a problem with the IdP. Are you saying that Spring Security should fail if the parameters from the IdP are in the wrong order? (Perhaps you are right, I just first want to make sure I understand the problem you are running into.)
But in the Spring Security 5.8.0/6.0.0, content is just query parameters without the Signature param and without care about mentioned above order of params
I'm not able to reproduce this. When I run OpenSaml4LogoutRequestResolver, the result of Saml2LogoutRequest#getParametersQuery is in the order you describe and does contain the Signature parameter.
Comment From: lukaszmigdalek
@jzheaux I can reproduce this issue by changing the order of params in the saml2LogoutRequestWhenLowercaseEncodingThenLogsOutAndSendsLogoutResponse test from the Spring Security like in this example: https://github.com/lukaszmigdalek/spring-security-issue-12346/commit/1f79f7beed3c0c6dbba762898058322f31f03c74
Order of HTTP params should not fail this test. But in this case, the mentioned test failed because the content for signature verification is prepared incorrectly:
[invalid_signature] Invalid signature for object [LR3e632a15-8eb6-4049-b72b-c38bea4adaad].
In the previous implementation, content for signature verification is prepared correctly.
Comment From: jzheaux
This seems to be a different observation you are making than what you posted in the issue description. What I'm hearing you say in your last comment is that if a request like this:
SAMLResponse=${samlRespnose}&SigAlg=${sigAlg}&RelayState=${relayState}&Signature=${signature}
is presented as:
SAMLResponse=${samlRespnose}&RelayState=${relayState}&SigAlg=${sigAlg}&Signature=${signature}
then signature verification should pass.
I disagree (though you might not be saying this). A signature is there to guarantee that the request has not been altered; if the order of the parameters changes, then the request has been altered, so it's reasonable for signature verification to fail.
In case I'm still misunderstanding, I'll further clarify that the reason the unit test fails when you change it is that the apLogoutRequestSignature matches a request that lists the parameters RelayState and then SigAlg. Once you change the order, the request has been tampered with and, as such, the signature verification fails.
Comment From: lukaszmigdalek
@jzheaux you are wrong about the order of query parameters. According to the specification of SAML 2.0 HTTP Redirect Binding:
Note that when verifying signatures, the order of the query string parameters on the resulting URL to be
verified is not prescribed by this binding. The parameters may appear in any order. Before verifying a
signature, if any, the relying party MUST ensure that the parameter values to be verified are ordered as
required by the signing rules above.
(page 18, SAML 2.0 Binding Spec)
Comment From: lukaszmigdalek
According to my previous comment, I have prepared an example implementation to handle query parameters as described in the SAML 2.0 HTTP Redirect Binding specification.
https://github.com/spring-projects/spring-security/pull/12963
Comment From: jzheaux
Okay, I think I understand now.
What you are saying is that the order needs to be SAMLRequest -> RelayState -> SigAlg (which I agree with), but that Spring Security is not requiring this order during verification as it was lost in https://github.com/spring-projects/spring-security/commit/5cbc1a47da4dea6c3bba02236a1f149959ef9ff0#diff-a622c52bae8f625138d6cced9a4a822114dbc24bdcec1fc46511f9f97920e7c6L206.
I'll review the PR, thanks.