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.

[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.

[ahacker1-securesaml]

@DemiMarie yes your comments are correct.

We need to do two things to make dex secure:

• Fix the XML Signature library (goxmldsig) so that it returns only the signed bytes in it's verification function.

goxmldsig

I refactored https://github.com/russellhaering/goxmldsig so that it's verification function returns the verified element. This verified element is parsed from the exact byte string that was signed.

You can see the changes here: russellhaering/goxmldsig@e1c8a5b
This has been reflected in v1.5.0, which dex is up to date with.

Dex's responsibility

As you mentioned earlier, dex needs to use only the signed contents from the xml signature library i.e. use only the contents from the goxmldsig verification function.

To load a SAML Response, dex uses the verifyResponseSig function:

dex/connector/saml/saml.go

Lines 309 to 318 in 7c97449

	rawResp, rootElementSigned, err = verifyResponseSig(p.validator, rawResp)
	if err != nil {
	return ident, fmt.Errorf("verify signature: %v", err)
	}
	}
	var resp response
	if err := xml.Unmarshal(rawResp, &resp); err != nil {
	return ident, fmt.Errorf("unmarshal response: %v", err)
	}

Now we need to analyze whether the return value of this function depends solely on return value of the goxmldsig.

func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed []byte, rootVerified bool, err error) {
	doc := etree.NewDocument()
	if err = doc.ReadFromBytes(data); err != nil {
		return nil, false, fmt.Errorf("parse document: %v", err)
	}
       // doc is the originalDocument. XML Signatures don't attest anything about this original doc

	response := doc.Root()
	if response == nil {
		return nil, false, fmt.Errorf("parse document: empty root")
	}
        // A: transformedResponse is authenticated NOT the original doc
	transformedResponse, err := validator.Validate(response)
	if err == nil {
		// Root element is verified, return it.
		doc.SetRoot(transformedResponse)
		signed, err = doc.WriteToBytes()
                // A: INCORRECT, the transformedResponse element is indeed verified, but this code implicitly trusts the original doc
                // Because the original doc is contaminated with user input's when serializing it might override the contents i.e. roundtripping
                // we need to instead just return the signed element i.e. either create a NEW isolated etree or just return as is.
		return signed, true, err
	}


	assertion, err := etreeutils.NSSelectOne(response, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
	if err != nil || assertion == nil {
		return nil, false, fmt.Errorf("response does not contain an Assertion element")
	}
        // B: transformedAssertion is authenticated **NOT anything else**
	transformedAssertion, err := validator.Validate(assertion)
	if err != nil {
		return nil, false, fmt.Errorf("response does not contain a valid signature element: %v", err)
	}

        // INCORRECT, this indirectly trusts the response element. Again the response element contains untrusted user input. Removing the childElements doesn't remove directives which interferes with serialization.
       // Most secure way is to create a NEW isolated etree with only `transformedAssertion`
	for _, el := range response.ChildElements() {
		response.RemoveChild(el)
	}


        // C: INCORRECT, comment below is not true. We can return a SAML assertions and be fine

	// We still return the full <Response> element, even though it's unverified
	// because the <Assertion> element is not a valid XML document on its own.
	// It still requires the root element to define things like namespaces.
	response.AddChild(transformedAssertion)
	signed, err = doc.WriteToBytes()
	return signed, false, err
}

There are two signatures that dexidp verifies. I commented on the analysis of both: (A: and B: )

Solution:

• We just need the SAML assertion. We don't even need the SAML Response.

Return (depending on code flow)

• transformedResponse.selectElement("Assertion") // psuedocode you may need to update it
• transformAssertion

Breaking changes since we only use assertion now

Obtain the InResponseTo from assertion.subjectConfirmationData["InResponseTo"] https://learn.microsoft.com/en-us/dotnet/api/microsoft.identitymodel.tokens.saml2.saml2subjectconfirmationdata.inresponseto?view=msal-web-dotnet-latest

Namespaces: The transformedAssertion already contains all the namespaces that are used. We can just use the etree element as
is?

[ahacker1-securesaml]

PR is here:
https://github.com/dexidp/dex/pull/4165/files

Hopefully we can close this discussion as resolved

[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).

[berhauz]

The XML digital signature standards are extremely (and I dare even say "stupidly") complex because of the number of options they have created or leave open, instead of having created a very strict subset of enforced conventions: that was definitely paving the way to overly complex implementations with plenty of weaknesses, bugs when not functional errors (like the signature verification vulnerability in go-lang libs), and has definitely fueled a waste of countless hours at solving compatibility issues, debugging, and so forth. Organizations like ETSI have indeed created official standards like "ETSI EN 319 132 XML Advanced Electronic Signatures (XAdES)" to streamline proper use and practices which are widely used, notably regarding the EU eIDAS qualified signature directives. So I understand that this field generates a lot of defiance (I share it), but I would advocate of not deprecating a still very important SAML DEX connector for many organizations on the basis that:

1. most of us have not studied XML signature mechanisms and/or do not see any interest in investing into the domain: incompetence is not an argument I can buy, but defiance is definitely to be taken into account
2. the vulnerabilities at stake (and risks of more) are real, but in any proper operational context, there shall be a 2-way secured TLS connection between DEX (as SP / ServiceProvider) and the IDP with which SAML assertions are exchanged, and then between the App and DEX. That means these TLS connections (in particular the one between SP and IDP) shall first be compromised by any hacker to possibly exploit the weakness of the xmldsig module in go (that has been fixed since then...). That means the hacker is possibly in the place with far more capabilities - and easier paths - to compromise systems that tentatively tweaking XML digital signatures in SAML assertions
3. OIDC starts with client ID's and secrets that are not signed and rely on the security of TLS/SSL connections
4. Most (if not all) DEX usage scenarii about its SAML connector is to support INTRAnet use (i.e. SSO for App's within the Intranet), hence with systems and networking security capabilities beyond what is feasible over the public Internet. I would not use SAML if the App, DEX as SP, and the IDP, are not altogether within the Intranet.
5. I undertand the shortage of resources with sufficient skills or with any willigness to devote the needed time to acquire the relevant skills

So I would recommend not to throw away the connector, and to maintain the SAML connector working as is in future DEX releases with definitely a proper accompanying note or state marking the defiance, about the risks and exposure to vulnerabilities found in the implementation stack at stake. Organization are indeed invited to consider security settings globally, and notably the strength of network security in place to ensure the integrity of the information exchanged between their systems.

[ahacker1-securesaml]

The XML digital signature standards are extremely (and I dare even say "stupidly") complex because of the number of options they have created or leave open, i

This is correct. Flaws of XML Signatures it there are too many options need to support (hence processing vulnerabilities), unclear documentation, and it's just wrong cryptographically.

the vulnerabilities at stake (and risks of more) are real, but in any proper operational context, there shall be a 2-way secured TLS connection between DEX (as SP / ServiceProvider) and the IDP with which SAML assertions are exchanged, and then between the App and DEX. That means these TLS connections (in particular the one between SP and IDP) shall first be compromised by any hacker to possibly exploit the weakness of the xmldsig module in go (that has been fixed since then...). That means the hacker is possibly in the place with far more capabilities - and easier paths - to compromise systems that tentatively tweaking XML digital signatures in SAML assertions

Incorrect. In SAML that IdP sends the SAMLResponse to the user's browser via an HTML form. This form sends a POST request to dex (SP) An attacker is free to tamper with it i.e. via burp suite, SAML Raider et al.

There is a flow called SAML artificat binding which relies on a TLS connection between IdP and SP. However, no one uses it, and I doubt users will migrate to it.

I undertand the shortage of resources with sufficient skills or with any willigness to devote the needed time to acquire the relevant skills

I am devoting time in researching SAML. I hope to release a fix for DexIdP to eliminate the class of signature verification vulnerabilities. By actually using what was signed instead of trusting the original document.

[DemiMarie]

Other mitigations that have been used include:

• Forbidding all XML comments and directives in the response.
• Verify the SAML response against its XML schema before doing any other processing.
• Use a parser, such as libxml2, that does not have bugs like encoding/xml: does not check namespace constraints that do not require keeping extra state golang/go#68296 and encoding/xml: normalize \t, \r and \n to space in attributes golang/go#69198.

However, none of those are as strong as what you have done here.

[ajdurr]

Hi is any update? We would like to use the Dex SAML connector with Argo.

[TreeKat71]

Yeah, I also want to know that. I just configured it in ArgoCD and then found this. If it is a seriou secuirty issue, then I will find other soultions.

[ahacker1-securesaml]

I wrote the PR: https://github.com/dexidp/dex/pull/4165/files
You can ask maintainers to merge it