Fix saml signature verification

[ahacker1-securesaml]

Overview

• Update verifyResponseSig, so that it returns only signed assertion contents
• Update saml.go to parse identity from solely the verified assertion i.e. return value in verifyResponseSig

What this PR does / why we need it

Resolves security concerns around: Closes #1884
Previously, the verifyResponseSig returned contents from the original SAML Response document. We must instead return contents from only the signed parts of the document i.e. parts verified with the XML Signature library.
This PR does that.

Special notes for your reviewer

This would break cases where p.validator is nil. i.e. users don't want to verify SAML Response, since we cannot obtain any signed assertion from it.
This would be an insecure and rare use case, as it would allow an attacker to create their own SAML Responses unsigned.

However, I can update the PR if Dex wants to support this use case.

[ahacker1-securesaml]

@joonas-fi @DemiMarie see: #1884

Can you confirm that this PR meets your expectation of parsing from solely signed contents. Let me know if there are any security improvements.

Thanks!

[DemiMarie]

Is it possible to create a response that contains only the assertion, data derived from the assertion, and data not under attacker control?

[ahacker1-securesaml]

In a case where only the assertion is signed like:

<Response InResponseTo="...">
<Assertion><Signature/>
Subject ...
</Assertion>
</Response>

What we can tell is signed is the canonical assertion string:

<Assertion>
subject
</Assertion>

In that case I just reparse the Assertino node and return it.

I think you're interested in also returning a Response.
In this case, I actually return nil for the Response since we can't tell it's signed. (And further on we don't actually use the Response element, we just process the assertion element)

Previously dexidp, kept the original response ,but then removed all the child elements and replaced them with our signed assertion node.

This is unsafe because you end up with something like

etree {
   "rootElement": {
         "name": "Response",
         "attrs": ["USER CONTROLLED"], 
        "childElements": [SignedAssertionNode]
    }, 
   "directives": ["USER_CONTROLLED"], 
   "other xml parts": "USERCONTROLLED"
}

When serializing our etree element, the USER_CONTROLLED could round trip and actually become another Assertion node. When reparsing it sees our USER_CONTROLLED data.

So now I don't even bother to return the response if only assertion is signed

[DemiMarie]

What I meant is to return something like

<Response>
<Assertion>
stuff
</Assertion>
</Response>

where no components are attacker controlled.

[ahacker1-securesaml]

Yea, that could work.
However, I think it's simpler if we return (assertion, response) as a pair. It's simpler since the we only need the assertion for obtaining the user identity.

The response element is quite different, and sometimes unsigned. So it's better to separate the two.

[ahacker1-securesaml]

Hey @jupenur , @ericchiang
could you also review the PR, and decide next steps for release.
Since you previously worked on saml connector/found vulnerabilities in it.

Also what should we do if no validator is specified. It's quite dangerous behavior to not verify a SAML Response, so for my PR I didn't return any identity.

Thanks!

[ericchiang]

I haven't worked on dex in a long time, so you may want to reach out to one of the existing maintainers for review:

https://github.com/dexidp/dex/blob/master/MAINTAINERS

[ahacker1-securesaml]

cc @sagikazarmark this resolves a security concern