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

Add InResponseTo validation support #9174

Closed
fast-reflexes opened this issue Nov 2, 2020 · 10 comments
Closed

Add InResponseTo validation support #9174

fast-reflexes opened this issue Nov 2, 2020 · 10 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@fast-reflexes
Copy link
Contributor

Describe the bug
The InResponseTo field is not validated and the repsonse is not rejected when this does not correspond to a sent request.

To Reproduce

  1. Set up SAML2 login and run your server locally which redirects to an IDP which will then redirect to an actual deployed SAML2 authentication endpoint.
  2. Set the host of your deployed app to 127.0.0.1 in /etc/hosts
  3. Ping the host and verify that localhost responds
  4. Start the local app and try to log in, you will be directed to your IDP which will redirect back to the authentication endpoint which will not exist on your local machine, thereby resulting in a not found error.
  5. Copy the unresponded request as cURL from the console and paste it in a document.
  6. Add flags .L and -i and remove cookies headers to be on the safe side.
  7. Remove the entry in /etc/hosts and verify that the actual deployed application responds on ping.
  8. Run the command with cURL and verify that the deployed application authentication endpoint accepts the cURL command which is a response to a request NOT initiated by the deployed application but by the local application.

Expected behavior
If the request contains a InResponseTo which is incorrect, it should be rejected.

Sample

A link to a GitHub repository with a minimal, reproducible sample.

Reports that include a sample will take priority over reports that do not.
At times, we may require a sample, so it is good to try and include a sample up front.

@jzheaux
Copy link
Contributor

jzheaux commented Nov 3, 2020

@fast-reflexes, you are correct - this is because the AuthNRequest is not saved anywhere after the relying party generates and sends it to the asserting party.

I've created a ticket to add that support - #9185 - where your contribution would be most welcome.

Once that is complete, we can take a closer look at this ticket to see how to best validate InResponseTo. In the meantime, would you mind if I changed this ticket to an enhancement and reworded it to Add InResponseTo validation support?

@fast-reflexes
Copy link
Contributor Author

@jzheaux Thanks for answering! No, go ahead and reword it!

@jzheaux jzheaux changed the title SAML2 InResponseTo is not properly validated Add InResponseTo validation support Nov 12, 2020
@jzheaux jzheaux added type: enhancement A general enhancement and removed type: bug A general bug labels Nov 12, 2020
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 10, 2022
@jzheaux jzheaux added this to the 5.7.x milestone Feb 10, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Feb 10, 2022

@fast-reflexes Now that Saml2AuthenticationRequestRepository is in place, I think we have what's needed to proceed with this ticket.

As I see it, there are two things needed:

  • Add an id attribute to AbstractSaml2AuthenticationRequest and populate it in OpenSamlAuthenticationRequestResolver
  • Add a validation step in OpenSaml4AuthenticationProvider that compares Response#inResponseTo to Saml2AuthenticationToken#getAuthenticationRequest#getId if both are set

Are you able to provide a PR along those lines? If not, I believe I have some time to do it.

@jzheaux jzheaux modified the milestones: 5.7.x, 5.7.0-M2 Feb 10, 2022
@bitrecycling
Copy link
Contributor

@jzheaux I have the same problem currently. I have 2 thoughts on that topic currently:

  1. I think validation is only really relevant for Responses to authn Request and logout request or am I mistaken? Also replay attacks with "inResponseTo"-Responses can be mitigated if "inResponseTo" is validated.

  2. However I was wondering, if an additional validation in the ID itself makes sense, to prevent using the same response i.e. if no "inResponseTo" is present. If any Request ID is stored for the time the request is valid and any new request's ID is checked against the list of the already consumed (and hence ID stored) IDs, then everything should be fine.

What do you think?

@fast-reflexes
Copy link
Contributor Author

@jzheaux Yes, I would really like to start contributing so could you give me a week to do it and I will either do it in that time or let someone else ahead?

@jzheaux
Copy link
Contributor

jzheaux commented Feb 15, 2022

@bitrecycling, good questions:

I think validation is only really relevant for Responses to authn Request and logout request

I think that it's important to confirm that the InResponseTo value in a SAML 2.0 Response actually matches the AuthnRequest that initiated the request. Same for logout. Yes, I believe it's a way to mitigate replay attacks.

However I was wondering, if an additional validation in the ID itself makes sense, to prevent using the same response i.e. if no "inResponseTo" is present.

A SAML Response has an expiry. Given that, you should be able to create a cache of IDs that you can check against for replay attacks. For the time being, I think Spring Security's default validation should remain minimal so that it's easy to extend and replace, but I do think a component like that could be interesting for you to share, should you create one.

@bitrecycling
Copy link
Contributor

@jzheaux after thinking it through, I currently would argue against ID validation at least for login where it's most critical. Reasoning:

  1. Spring security saml2 currently only supports Web Browser SSO profile, that means all requests / responses are handed over through the user's browser and are (at least the logins) user "initiated". Even true for SSO logins to another SP, because an AuthnRequest is issued still if "my" SP sees an anonymous user.
  2. That in turn means any reponse from an IDP will be aligned into the same session as the originating request. Hence the request must reside already in the Saml2AuthenticationRequestRepository that can be session-specific.
  3. If 1 and 2 are true, any response to a login must have a valid inResponseTo that can be checked against the stored AuthnRequest.
  4. this renders an ID validation using a storage of already "seen" and still valid request IDs somehow unnecessary. It might be useful to prevent unwanted SLO logouts that are not initiated on "my" SP. But I doubt it's value.

So given the high value of and (relatively) easy and lightweight validation of "inResponseTo" I would go that path to prevent false or doubly issued login responses. And rather not implement the difficult to handle and maintain IDs Storage to check against especially given that it might not even provide real value. I am sorry this got lengthy, but I would really appreciate your opinion. If you still think it could be useful, I might come up with a solution.

I definitely need a solution for the inReponseTo validation though, probably even earlier than @fast-reflexes can provide, so I will likely come up with my own implementation.

/Robert

@fast-reflexes
Copy link
Contributor Author

fast-reflexes commented Feb 18, 2022

@jzheaux I've started working on it. So we should always validate if it is present in the response, no user flag to indicate whether the validation functionality is desired or not?

@jzheaux
Copy link
Contributor

jzheaux commented Feb 18, 2022

Good question, @fast-reflexes. I think you'd validate it if both the request and response are present and the response has an InResponseTo. Applications may choose to not store the AuthnRequest.

To turn off that validation, applications can always either:

  • Not store the AuthnRequest, or
  • Wire their own response validator. It could either be completely custom or could delegate to the default and remove the unwanted error

@fast-reflexes
Copy link
Contributor Author

A question, if the login is unsolicited, there won't be a request stored, but if there IS a request stored but the response does NOT have InResponseTo attribute set, then we should reject as well right?

@marcusdacoregio marcusdacoregio modified the milestones: 5.7.0-M2, 5.7.0-M3 Feb 21, 2022
fast-reflexes added a commit to fast-reflexes/spring-security that referenced this issue Mar 12, 2022
Whenever an InResponseTo is present in the SAML2 response and / or any of its assertions, it will be validated against the stored SAML2 request. If the request is missing or the ID of the request does not match the InResponseTo, validation fails. If there is no InResponseTo, no validation of it is done (as opposed to checking whether there is a saved request or not and then failing based on that).

Closes spring-projectsgh-9174
jzheaux added a commit that referenced this issue Mar 15, 2022
- Moved methods so methods are listed before the methods they call
- Adjusted exception handling so no exceptions are eaten
- Adjusted so that malformed_request_data is returned with request data is malformed
- Refactored methods to have only immutable method parameters
- Removed usage of Stream API
- Moved AuthnRequestUnmarshaller into static block so that only looked
up once

Issue gh-9174
jzheaux pushed a commit that referenced this issue Mar 15, 2022
Whenever an InResponseTo is present in the SAML2 response and / or any of its assertions, it will be validated against the stored SAML2 request. If the request is missing or the ID of the request does not match the InResponseTo, validation fails. If there is no InResponseTo, no validation of it is done (as opposed to checking whether there is a saved request or not and then failing based on that).

Closes gh-9174
jzheaux added a commit that referenced this issue Mar 15, 2022
- Moved methods so methods are listed before the methods they call
- Adjusted exception handling so no exceptions are eaten
- Adjusted so that malformed_request_data is returned with request data is malformed
- Refactored methods to have only immutable method parameters
- Removed usage of Stream API
- Moved AuthnRequestUnmarshaller into static block so that only looked
up once

Issue gh-9174
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants