Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth.processSLO() should fast-fail on non-GET requests #299

Open
veselov opened this issue Feb 12, 2021 · 9 comments
Open

Auth.processSLO() should fast-fail on non-GET requests #299

veselov opened this issue Feb 12, 2021 · 9 comments

Comments

@veselov
Copy link

veselov commented Feb 12, 2021

Signatures generated for logout response in opensaml (2.6.6) can not be validated by java-saml.

I'm still in the process of investigating this, and not really sure who to blame.

The reason for signature mismatch is that Opensaml puts the original message contents into the signing buffer, and java-saml uses base64 encoded version of it. It actually seems to use percent-encoded base64 encoded message.

However, I also could not validate the message using https://www.samltool.com/validate_logout_res.php. Interestingly, the latter doesn't accept base64 logout response contents.

I also can't find where is this described in SAML documentation. That documentation only talks about XML signatures, which whatever is being used here is not.

I'm loaded with questions, though:

  • Where is this method of signing documented (as part of SAML documentation suite)?
  • Why not just use the XML signature of the logout response XML?
  • Verifying signature using percent-encoded string sounds suspicious. At least from the perspective that online validator takes only XML/Deflated String (whatever it is), and recreating percent-encoding is not possible, as too many variations (simplest is the letter case of the hex characters) are allowed in it (unless, of course, the spec mandates that percent encoding is done in a specific way. On the other hand, that encoding is done by a browser, as it's submitting an HTML form, so it can encode it arbitrarily).
@pitbulk
Copy link
Contributor

pitbulk commented Feb 22, 2021

samltool.com uses the php-saml toolkit as base, not java-saml, and the validate_logout_res.php endpoint accepts the XML but not the base64encoded form, you can just decode it with https://www.samltool.com/decode.php.

At java-saml, the method to validate the Signature of HTTP-Redirect bindings is validateBinarySignature

The parameters are retrieved using request.getEncodedParameter unless the signature which is retrieved first with request.getParameter("Signature"); and later decoded with Util.base64decoder that executes

Base64.decodeBase64(input)

Based on its documentation, the Base64 class provides Base64 encoding and decoding as defined by RFC 2045.

The getEncodedParameter uses Utils.urlEncoder uses
java.net.URLEncoder.encode

The SAML standard specifies how sign on the HTTP-Redirect binding protocol at
https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf

3.4.4.1 DEFLATE Encoding
...
The signature value MUST be encoded using the base64 encoding (see RFC 2045 [RFC2045]) with
any whitespace removed.

....

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.

@veselov
Copy link
Author

veselov commented Feb 22, 2021

@pitbulk Thank you for your patience, Sixto.

I was apparently using HTTP Post bindings, where I should have been using HTTP Redirect bindings. Auth.processSLO() just gets request parameter, and can't know whether those are post parameters or URL parameters, so problems only become apparent during signature verification.

@veselov veselov closed this as completed Feb 22, 2021
@veselov
Copy link
Author

veselov commented Feb 22, 2021

On the other hand, would it be an improvement if bindings type was checked before the rest of the validation started, so this is easier to figure out?

@veselov veselov reopened this Feb 22, 2021
@pitbulk
Copy link
Contributor

pitbulk commented Feb 22, 2021

java-saml generates and receives all SAML messages by the HTTP-Redirect binding with the exception of the SAMLResponse received via HTTP-POST.

@veselov
Copy link
Author

veselov commented Feb 22, 2021

But how can the SP side control which bindings it wants?

In my case I'm the SP, and after sending a logout request, the logout response was received as an HTTP Post binding, simply because IDP decided so. But then it's just a mock IDP, so it's not following a whole lot of rules.

I've changed it to use HTTP Redirect and that fixed things, but it would have been a lot easier to realize what's wrong if the response processing code checked the binding and threw an error before going into signature check.

@pitbulk
Copy link
Contributor

pitbulk commented Feb 22, 2021

SP exposes SAML metadata where are defined the endpoints and its associated binding.
If you registered the SP metadata properly at the IdP, the IdP should not send an HTTP-POST Logout request to an HTTP-Redirect endpoint.

I wonder why you have not experienced the error defined here:
https://github.com/onelogin/java-saml/blob/master/toolkit/src/main/java/com/onelogin/saml2/Auth.java#L954

String errorMsg = "SAML LogoutRequest/LogoutResponse not found. Only supported HTTP_REDIRECT Binding";

which is exactly the error that you wanted to receive instead the signature validation error.

@veselov
Copy link
Author

veselov commented Feb 23, 2021

SP exposes SAML metadata where are defined the endpoints and its associated binding.

That's fair. But yes, I was dealing with Mujina, which I then hacked myself, no SP metadata, so no wonder I stepped into this.

I wonder why you have not experienced the error defined here

That's what I was talking about. Using HTTPRequest.getParameter() there is no way to differentiate between form parameters and query parameters. Since there was a (validish) SAML response returned from httpRequest.getParameter("SAMLResponse") (provided by the HTTP post request into the SLS), the code just happily marched forward.

@pitbulk
Copy link
Contributor

pitbulk commented Feb 23, 2021

Oh I see, I think in the past this kind of error was raised...but when the httpRequest class was added, we missed it.
I agree could be cool to have some kind of check on processSLO to verify it is a GET and not a POST.
Do you want to provide a PR?

@veselov veselov changed the title Signature verification of logout response is not compatible with opensaml (2.6.6) Auth.processSLO() should fast-fail on non-GET requests Feb 23, 2021
@veselov veselov changed the title Auth.processSLO() should fast-fail on non-GET requests Auth.processSLO() should fast-fail on non-GET requests Feb 23, 2021
@veselov
Copy link
Author

veselov commented Feb 23, 2021

Yes, let me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants