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

Remove unnecessary SP's request state check #975

Closed
lhaemmerle opened this Issue Oct 31, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@lhaemmerle

lhaemmerle commented Oct 31, 2018

Is your feature request related to a problem? Please describe.
We have the case that a user wants to access an SSP SP, which first lets the user pick an IdP A in the Discovery Service. The user then is sent to IdP A to authenticate but in our case the IdP A can also act as IdP B (with a different entityID). If the user is then returned with an authentication response from IdP B (instead of A), this response currently is rejected by the SSP SP even though it would have accepted the response if the user had initially selected IdP B in the Discovery Service and even though the SSP SP would have also accepted it if it was an unsolicited authentication response.

Describe the solution you'd like
Given that the check only provided a benefit if unsolicited authentication responses were completely blocked and given that unsolicited authentication responses are already in use by some SSP SPs, the proposal is to remove the (unncessary) state check and accept the assertion from IdP B even though the user was sent to IdP A by the SP initially. The check of the entityID before and after authentication provides no benefit but creates issues for IdPs that act as different (virtual) IdPs with several entityIDs.

Describe alternatives you've considered
Alternatively to remove this check, SSP could change its behaviour to block unsolicited authentication responses. However, this would most likely cause forward-compatibility/upgrade issues as currently some SSP SPs expect unsolicited authentication responses to work. Also, this would not solve our initial problem but would make things worse for some existing SSP deployments.

Additional context
The Shibboleth SP as well as Microsoft ADFS allow unsolicited authentication responses and they don't block authentication responses that are received from a different IdP than what the user selected on the Discovery Service. So, changing the behaviour of SSP in this regard would make it more consistent with these two other popular SAML SP implementations.
More details on why this check does not provide an extra benefit is also available in this thread where Scott Cantor (one of the SAML authors) explains when and how such a check would make sense: https://marc.info/?l=shibboleth-dev&m=154091453202576&w=2

@tvdijen

This comment has been minimized.

Member

tvdijen commented Oct 31, 2018

Would you be able to provide a SAML-trace that covers your scenario?
I'm wondering if your IdP A is answering as IdP B as an 'InReponseTo' the message ID from the authentication request.. If this is the case, then I'm not surprised SSP is detecting this as a fishy situation..

@lhaemmerle

This comment has been minimized.

lhaemmerle commented Oct 31, 2018

The InResponseTo is containing the correct value but the issuer is IdP B instead of A. The authentication does not contain the information at which IdP the user should authenticate.
As long as the SP is not blocking all unsolicited authentication responses (which SSP does not), the Issuer check is not adding any additional security. The SP would have accepted the authentication response totally fine if the response was IdP-initiated or if the user had chosen IdP B in the first place.

Find below the request/response:

<samlp:AuthnRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                    AssertionConsumerServiceURL="https://attribute-viewer.aai.switch.ch/Shibboleth.sso/SAML2/POST"
                    Destination="https://login.eduid.ch/idp/profile/SAML2/Redirect/SSO"
                    ID="_b3268cc54acd73395e4657143be9f65a"
                    IssueInstant="2018-10-31T11:46:09Z"
                    ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
                    Version="2.0"
                    > <saml:Issuer xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">https://attribute-viewer.aai.switch.ch/shibboleth</saml:Issuer> <samlp:NameIDPolicy AllowCreate="1" /> </samlp:AuthnRequest> 

and

<saml2p:Response Destination="https://attribute-viewer.aai.switch.ch/Shibboleth.sso/SAML2/POST"
                 ID="_fd1ff1b85db33066a15da6af09ca61a3"
                 InResponseTo="_b3268cc54acd73395e4657143be9f65a"
                 IssueInstant="2018-10-31T11:46:14.350Z"
                 Version="2.0"
                 xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol"
                 >
                  <saml2:Issuer xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">https://aai-logon.switch.ch/idp/shibboleth</saml2:Issuer>
                 <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
[...]
@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Oct 31, 2018

Just for completeness, Lukas and I have discussed this off the issue tracker. While it makes sense to have the check in place (the user asked to authenticate with IdP A, and a response arrived from IdP B instead, which can be confusing and troublesome), Scott has a pretty good point when he argues that if we're not blocking unsolicited responses (a.k.a. IdP-first flow), the check is rather useless: receiving a SAML response from IdP B that doesn't have an InResponseTo field or that points to a state that cannot be found will be treated as an unsolicited response for convenience, so an attacker can always send a response like that and log in anyway.

It would be different if we block all unsolicited responses, because then, as Scott points out, there are actual security benefits. However, Shibboleth doesn't do it, and this looks like it's not precisely a corner case, as some IdPs behave like that. That given, blocking IdP-first flows would hinder interoperability, and make also the experience much worse for our users (imagine we store the sessions in a database and the database goes down, or it has a hiccup and the session is lost: today, a response pointing to a missing state would still allow you to log in, while if we block IdP-first, the user would get an error).

That said, I'm inclined to take a look at this, try to find out if there were other (good) reasons to reject responses from unmatching IdPs, and if not, remove the check or downgrade it to a warning message, for example.

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 13, 2018

FWIW it seems reasonable to me to downgrade the check to a warning.

@thijskh thijskh closed this in e543fe5 Nov 21, 2018

@thijskh thijskh added this to the 1.17 milestone Nov 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment