Proposal: deprecate the SAML connector

[ericchiang]

@justaugustus @srenatus @sagikazarmark

Due to the nature of complex security issues in SAML, and lack of technical domain experts, I'd like to propose dex deprecate the SAML connector.

Is your feature request related to a problem?

This issue is to formalize internal discussion from GHSA-m9hp-7r99-94h5

Dex is inherently in a privileged position within an environment, and being able to bypass authentication is usually enough to access any resources that trust dex. SAML is unique from other connectors because of the complexity in the underlying technologies (https://www.cs.auckland.ac.nz/~pgut001/pubs/xmlsec.txt). In addition modern IdPs also always support some form of OAuth2 or OIDC, which aren't perfect, but there's wide agreement in the Security community that these are significantly more robust than SAML.

Despite @jupenur awesome explanations, even I don't feel I fully understand exactly how dex was impacted, or how dex could have resolve the issue without checking the specific round-trip issues in the CVEs (57640cc). As discussed in golang/go#43168 Go's XML parser doesn't guarantee round-trip stability, so there are likely more vulnerabilities due to this, particularly since dex depends on both encoding/xml and github.com/beevik/etree to interpret XML. Broadly, it's extremely hard to reason about the security of this code without strong expertise.

Dex currently doesn't have a maintainer that's actively worked on the SAML connector. During the response to the CVEs GHSA-m9hp-7r99-94h5 it was discovered that dex was vulnerable to a different issue that had been public since September GHSA-q547-gmf8-8jr7.

UPDATE:

It's likely that dex has more issues with round trip serialization that aren't covered by the current report #1884 (comment). We should assume it's vulnerable to more authentication bypasses that aren't resolved by GHSA-m9hp-7r99-94h5.

In addition, the upstream maintainer of the XML signature validation library doesn't have confidence in the protocol as a whole https://news.ycombinator.com/item?id=25424267. That means the author of the connector (me), the maintainer of the sig validation library (Russell), and the maintainer of encoding/xml golang/go#43168 don't think this code meets any strong security guarantees.

Describe the solution you'd like to see

I'd propose deprecating the SAML connector to communicate the risks of using it, which I believe are different enough from other connectors that it's worth calling out. If a user has a choice between OAuth2, OIDC, or SAML, they should always use OAuth2 or OIDC.

Describe alternatives you've considered

If there are companies invested in this code, they might consider providing resources for a subject matter expert that can give this connector the attention it needs (maybe from https://github.com/dexidp/dex/blob/master/ADOPTERS.md?).

Additional context

[justaugustus]

(Converted this to a GitHub Discussion.)

[sagikazarmark]

Generally I agree, but I think SAML is still a popular connector. Maybe we could gather some information from the community (eg. with a quick survey?).

[primeroz]

but I think SAML is still a popular connector

Most Users of dex i know uses it with generic SAML IDP providers.

We are one of those

[ericchiang]

Agreed that we should get the word out 😃 Would also be good to understand what IdP software users are connecting to. If this means more custom OAuth2 providers, or implementing a plugin model, that might be a reasonable alternative.

[ericchiang]

@justaugustus would you mind sending this discussion out on the @dexidp twitter handle?

[heidemn]

Not sure if you started a survey anywhere - Some of our customers are definitely still using SAML (maybe without knowing how bad it is).
So SAML is still important for us.

[justaugustus]

(Echoing my comments from the private advisory discussion.)

I'm broadly in favor of deprecating the SAML connector, with some notes...

Support policy

The SAML connector is one of our Stable connectors.

As a consumer, I'd have an expectation of a reasonable deprecation period and guidance on what/how I should migrate. AFAIK, we have no official deprecation policy and we should work on one before putting anything else into motion (I can take this item).

Maintainer overhead/burden

As a small set of maintainers, with several of us not having Dex as a primary focus at $dayjob, it's important that we recognize when certain things may not be working and move towards patterns that ease the maintenance burden.

Even more so as a security-focused project, it's our responsibility to move quickly (but safely) on areas that are attack surfaces, especially if we are not equipped to effectively maintain them.

Adopters

To the points mentioned by @ericchiang, I'd be in favor of considering keeping the SAML connector around if we had SME adopters that are interested in becoming maintainers.

Breaking up the "monolith"

Corollary to ^^, something that we could do is split the connectors out into their own repos?
Something like that would make me immeasurably more comfortable in onboarding new maintainers.
And maybe we don't need to create repos for every connector, but we could instead ACL the connectors along:

• stable
• beta
• alpha

[ericchiang]

While we're waiting on the exact deprecation process, I've sent PRs to add warnings for users:
#1887
dexidp/website#72

[sagikazarmark]

Corollary to ^^, something that we could do is split the connectors out into their own repos?

What would be the advantage of doing that, apart from better access control?

Generally, I'm strongly against "breaking up the monolith" without an obvious advantage, especially if the costs outweigh the benefits (which I believe is the case now, at least on the technical level). So there should be a pretty strong case presented in order for me to support this. Let's open a separate discussion.

As for better access control: maybe we could do something with OWNER files?

[justaugustus]

As for better access control: maybe we could do something with OWNER files?

Yep, this is roughly what my ramble might distill down to. I've got some ideas and will open a separate discussion on that.
Definitely agree that there should be clear advantages before considering any code reorganization.

[sagikazarmark]

I have some random thoughts, like general refactorings should definitely come first if we decide to go down this path (there are quire a few things to set in order) and we should agree on a long-term roadmap as well.

(BTW I really like this thread reply feature)

[justaugustus]

(BTW I really like this thread reply feature)

Yes! I'm a big fan so far! :)

[ericchiang]

One thing I'm worried about from the SAML side: despite fixes to github.com/russellhaering/goxmldsig dex was still vulnerable. As stated in the internal thread, I tried to structure the connector code so that if github.com/russellhaering/goxmldsig behaved correctly dex couldn't be vulnerable to these kind of issues. The fact that that assumption didn't hold worries me and should probably prompt an audit, if not rewrite. Combined with golang/go#43168, I think we should assume there're more vulnerabilities in this connector.

[DemiMarie]

Can we avoid relying on round-trip stability? After we have verified the signature, we should be able to throw the original XML out, and then re-parse the byte string we just checked the signature of.

In pseudo-Go:

func parseSignedXML(untrustedXml []byte) (XML, error) {
    if sig, err := getSignature(untrustedXml); nil != err {
        return nil, err
    } else if signedElement, err := getSignedElement(untrustedXml); nil != err {
        return nil, err
    } else if serialized, err := serializeTokens(signedElement); nil != err {
        return nil, err
    } else if err = checkSignature(serialized, sig); nil != err {
        return nil, err
    } else if reparsed, err := parseXML(serialized); nil != err {
        return nil, err
    } else {
        return reparsed, nil
    }
}

This doesn’t rely on round-trip stability at all. Once the signature is validated, we know that the input is trustworthy.

[DemiMarie]

Here is another approach:

func parseSignedXML(untrustedXml []byte) (XML, error) {
    if sig, err := getSignature(untrustedXml); nil != err {
        return nil, err
    } else if signedElement, err := getSignedElement(untrustedXml); nil != err {
        return nil, err
    } else if serialized, err := serializeTokens(signedElement); nil != err {
        return nil, err
    } else if err = checkSignature(serialized, sig); nil != err {
        return nil, err
    } else if reparsed, err := parseXML(serialized); nil != err {
        return nil, err
    } else if reserialized, err := serializeTokens(reparsed); nil != err {
        return nil, err
    } else if serialized != reserialized || !deepEqual(singedElement, reparsed) {
        // XML does not round trip ― possible attack
        return nil, errors.New("XML doesn’t round trip, aborting!")
    } else {
        return reparsed, nil
    }
}

[ericchiang]

Can we avoid relying on round-trip stability? After we have verified the signature, we should be able to throw the original XML out, and then re-parse the byte string we just checked the signature of.

I thought that's how the code worked when I wrote it the first time :)

@jupenur wrote some extensive analysis on our internal thread. Some of these issues stem from SAML's support for signing different parts of the message (which is kinda insane in the first place). Pasting the message here (apologies in advanced Juho if you didn't mean to make this public, but going to ask for forgiveness in that case):

@ericchiang I hate to have to say this, but your analysis is largely incorrect.

> it appears that you can force elements, attributes, and directives into the local default namespace instead of the global one

This is incorrect. The vulnerabilities don't make elements magically change their namespace and directives don't even have the concept of a namespace. Here's what happens with elements:

1. Application code invokes Go's decoder on this XML markup: <::SomeElement>
2. Go parses the tag's name as ::SomeElement, which it then splits into the namespace prefix and the local part; everything before the first colon is the namespace prefix -- so that's the empty string -- and everything after it is the local part, i.e. :SomeElement
3. Application code invokes Go's encoder on the output from the previous step, writing the XML back into a string
4. Go's encoder looks at the empty namespace prefix and determines that the tag is not prefixed. Because there's no prefix, a colon between the prefix and the local part is not needed. It continues to encode just the local part of the name, resulting in <:SomeElement>
5. Application code invokes the decoder again, but now the tag is parsed as having a local name of SomeElement, i.e. without a colon
This means that code that looks for, say, Assertion elements, does not initially interpret <::Assertion> as one. But after a round trip through the Go stdlib, the element suddenly does become an Assertion.

A similar thing happens with attributes, but because some attributes like xmlns have a special meaning, this has a whole range of implications of its own.

The case with directives is even worse: a round-tripped directive can split into parts, with some parts remaining directives, while others turning into arbitrary XML structures: start tags, end tags, comments, processing instructions, anything.

> If goxmldsig works correctly, Dex never actually handles unsigned input

This is also incorrect. Dex parses the incoming XML using etree and passes the already parsed structure to goxmldsig. The etree format cannot be easily validated using the security library we've implemented, because it's an entirely custom representation of XML.

In addition to that, the verifyResponseSig method is capable of verifying independently signed Assertions in a case where the root element is not signed. But the original root element is always preserved: unsigned child elements are removed one by one, but the root element itself can contain maliciously crafted attributes. If the root element is not signed, values from its attributes are of course never read by Dex, but "special" attributes like xmlns can still affect how the element's children are parsed.

Finally, the verifyResponseSig method only removes unsigned elements. Directives are not elements and thus are not removed. But because of the vulnerability affecting directive parsing, they can become elements later!

And because verifyResponseSig returns the returns the verified XML as a byte slice, it marshals the etree representation before returning, already constituting a full round-trip!

I stepped away from dex a couple years ago and have basically given up trying to understand SAML. I've written too many vulns at this point to have any confidence in my ability to know the security nuances. This is part of the issue. Dex doesn't have a subject matter expert to actually maintain this code.

I'll defer to @russellhaering on if it's possible to make this logic secure at all. But someone else will have to setup up to endorse the dex functionality.

[DemiMarie]

Does verifyResponseSig return anything other than the exact byte sequence that was signed? If so, that’s the problem.

[DemiMarie]

Also, I see no reason not to just reject directives entirely.

[joonas-fi]

Just a FYI: I wrote a blog post about SAML: https://joonas.fi/2021/08/saml-is-insecure-by-design/

[cweagans]

I came across this discussion from the link on https://dexidp.io/docs/connectors/saml/

I really hope that SAML support can remain in Dex somehow. I'm in the unfortunate position of needing to use the AWS SSO service (which is regrettably SAML-only right now) for a number of things. Dex is the only simple OIDC IdP that I've found that can authenticate against a SAML IdP. I fully agree that OIDC is the future and that SAML is broken and horrible, but Dex is currently serving a really important purpose as a stepping stone to help people get from SAML to OIDC while the rest of the world catches up. It seems way better to have one application (Dex) that authenticates against a SAML IdP than the alternative.

Separately from my interest as a user of Dex, I also maintain a SAML authentication module for the Drupal CMS, so I have some domain knowledge here. The PHP community has run into similar problems with xmlseclibs (roughly equivalent to goxmldsig). There are other places where xmlseclibs is used, but SAML libraries are by far the biggest consumer. This is by no means a problem that's specific to goxmldsig or even Go itself: XML signatures are just dumb and difficult to get right.

Because of the inherent complexity, most of the environments that I've worked in have used something like SAML Raider and focused a lot of time and energy into figuring out ways to break their implementation (As an aside, there is a lot of good SAML security-related literature linked in the readme of that repo). I haven't yet seen a better way to improve SAML security other than by repeatedly breaking things and fixing them. I'd really love to see a comprehensive blackbox testing tool for SAML SPs that codifies all of the existing knowledge about how to break a particular SP into a test suite of some kind, but that seems like a pipe dream.

In any case: if there's something I can do to help here, I'd be glad to spend some time on it. Just let me know what would be helpful!

[DemiMarie]

This is by no means a problem that's specific to goxmldsig or even Go itself: XML signatures are just dumb and difficult to get right.

It is possible to implement xml-dsig safely IF one does not rely on the XML parser to guarantee round-trip stability. That is, one needs to figure out what was signed, serialize it to a byte string, verify the signature on the byte string, reparse the signed byte string, and discard the original unsigned XML.

[LM1LC3N7]

Hi @cweagans ,

I currently have a situation close to yours: I need to use a SAML v2 SSO system with an app only accepting OIDC for SSO.

So I have few questions :

1. Do you still use Dex for "SSO translation"? Can you provide details on how you setup this translation?
2. Is there another way now for SSO translation between SAML and OIDC?

Thanks a lot for your time.

[deepakprabhakara]

Would love to help where we can, we have a well-maintained SAML service where we have wrapped the flow as an OAuth 2.0 flow and OIDC wrapper is coming soon. So integrating it with Dex should be straightforward. Our code is all in TypeScript though so the integration will have to be via a separate service rather than being inside of Dex.

I agree with @cweagans, SAML security is ultimately continuous testing. We have fixed a lot of the issues mentioned in the thread earlier and continue to stay on top of it. We are also looking to plug in a schema validator which should take away a significant surface area of attack.

OIDC is definitely the future but we are not quite there yet, SAML is still very popular and I do not see it going away immediately. Happy to discuss and collaborate further.

[DemiMarie]

I agree with @cweagans, SAML security is ultimately continuous testing. We have fixed a lot of the issues mentioned in the thread earlier and continue to stay on top of it. We are also looking to plug in a schema validator which should take away a significant surface area of attack.

See my comment above regarding reparsing the signed XML. If one only uses the byte string that was actually signed, and forgets about the rest of the code, then the security problems go away.

[deepakprabhakara]

That would work, ids can be compared as well for additional safety. Bigger question is if the saml library is being actively maintained?

[DemiMarie]

And if it uses the approach I mentioned (and which others have suggested long before).