TheIdentitySelector / thiss-js

The identity selector software source
Other
14 stars 23 forks source link

Potential open redirect vulnerability #276

Closed riney closed 1 month ago

riney commented 1 month ago

Hello - I'm a software engineer with NERSC, the National Energy Research Scientific Computing Center operated by Lawrence Berkeley National Laboratory. We use thiss-js as part of our Federated Identity stack to allow scientific users to access center resources with credentials from their home institutions.

Recently, our lab-wide security organization contracted with a penetration testing company to identify potential vulnerabilities in our infrastructure. They identified an open redirect in the /ds endpoint - URLs of the form https://login-selector-dev.nersc.gov/ds/index.html?entityID=injected+title&return=https://www.google.com can send the user to any desired URL, along with a fake title.

This is seen as a roadblock for our continued FedID rollout - are there any possible remediations, including whitelisting, to limit where the identity selector is allowed to redirect the user?

Thanks in advance!

leifj commented 1 month ago

This is how the SAML discovery protocol works. The whitelist you are talking about is the SAML metadata that is used to limit who can actually talk to the endpoints. It is possible to use the SAML metadata to also limit what the ds will display but this is almost never implemented. If you can control the SAML metadata in use in your deployment this would be an option for a more strict implementation. The thiss-js code is implemented to support the SeamlessAccess where there is no chance to control all the metadata and therefore the UX would be pretty bad if we only limited the metadata to those entities that are able to provide discovery information in metadata. Iirc the dev version of thiss-ds will notify the user if the entityID isn't in the whitelist. I will ask @sunetzacharias to add some info here.

leifj commented 1 month ago

of course it is always possible to add options to thiss-js to allow a deployment to turn on more strict processing of SAML metadata... I'm just not sure it would result in a service that would work for you. This is really a fundamental problem with current SAML deployments and would affect any implementation of the SAML discovery service protocol equally. In a closed ecosystem where you control all SAML metadata this is relatively easy to solve and perhaps thiss-js should allow for that option...

leifj commented 1 month ago

For reference: https://docs.oasis-open.org/security/saml/Post2.0/sstc-saml-idp-discovery.pdf and in section 2.5 the metadata extension is described that would have to be present for this problem to be solved. This extension is almost never present but could of course be easily included in a closed ecosystem.

sunetzacharias commented 1 month ago

Hello @riney! Thanks for reaching out, and nice to hear about your use case. What you're pointing to is known to us. We have been made aware of this from our community, and have looked into it.

The background we have seen:

With this is mind, we have looked at how SeamlessAccess can help the users access while still being made aware of the lack of specified return url. Our best course of action looks a little something like this

enriquepablo commented 1 month ago

Done in version 2.0.1