GitHub's SAML implementation is useless. The idea is that you can bring your own account into an enterprise, and that sort of works on the site itself, but it does not prevent apps where you log in with GitHub from reading your organization membership once you have authorized an app <i>on the organization level</i> (and if you didn't, it hides the membership from oauth tokens, so it has this capability!).<p>A SAML session is only required if said app fetches data via a token obtained from that user - and in my glance around, this was almost never the case - SAST tools almost always use app instance tokens and are happy to show anyone with a GitHub account in your organization your code. Tailscale fixed this when I pointed it out, Sonarcloud told me to please don't tell anyone and GitHub took a few weeks to say this is totally expected behavior - when no vendor I told did, and their docs contradicted them.<p>I swear, reporting security bugs is a thankless endeavor, even if you just randomly stumble over them. I couldn't imagine doing this as a job.
I recently had to implement SAML and this headline does not surprise me in the slightest.<p>The SAML spec itself is fairly reasonable, but is built upon XML signatures (and in turn, XML canonicalization) which are truly insane standards, if they can even be called such.<p>Only a committee could produce such a twisted and depraved specification, no single mind would be capable of holding and combining such contradictory ideas.<p>It would be <i>so</i> simple to just transmit signatures out-of-band and SAML would be a pleasure to implement.
SAML (more broadly XML-DSIG) is literally the worst security protocol in common use. I think you should generally be taking whatever hits you need to take to transition from it to OAuth. Certainly, I would refuse to bring a new product to market that relied on it. It's incredibly dangerous. Unless there's some breakthrough in practical formal verification, I can't imagine that this will be the last or the worst DSIG vulnerability.
Ugh. No one should use REXML unless they have no other choice. It will happily parse invalid xml, which causes an infinite number of problems downstream.<p>It’s quite literally parsing xml using regular expressions. It’s an excellent case study for why you shouldn’t do that.<p>Projects didn’t start using Nokogiri for performance. They used it because it’s correct.
Saml is insecure by design. Others have said it better before me, such as <a href="https://joonas.fi/2021/08/saml-is-insecure-by-design/" rel="nofollow">https://joonas.fi/2021/08/saml-is-insecure-by-design/</a>, but the quote I got from an old thread here was "Sign Bytes, not meanings".<p>Parser differentials are expected and even necessary. What you intend to get from a signed response is very meaningful. A dilemma in modern TLS is that sometimes you want to trust one internal CA; That's the easy path. Sometimes you want to accept a certificate from a partner's CA, and you've got multiple partners - and you can no longer examine just the end certificate, but the root of that chain is equally important in your decisions.<p>This is also why I recommend whenever possible against AWS Sig algorithms; V4 is theoretically secure, but they screwed it up twice - SigV1 and SigV3 were insecure by design, and yet somehow made it past design review and into the public.
This is a great write-up.<p>He's mentioned in the article, but a major shout-out is warranted for ahacker1. He's doing really sophisticated and valuable work to secure SAML implementations. We at SSOReady are really appreciative of his work.<p>Earlier this week, WorkOS put together a nice write-up on their own collaboration with ahacker1: <a href="https://workos.com/blog/samlstorm" rel="nofollow">https://workos.com/blog/samlstorm</a>
>We discovered an exploitable instance of this vulnerability in GitLab, and have notified their security team<p>GitLab has released a fix on their end for anyone else wondering<p><a href="https://about.gitlab.com/releases/2025/03/12/patch-release-gitlab-17-9-2-released/" rel="nofollow">https://about.gitlab.com/releases/2025/03/12/patch-release-g...</a>
Related: Latacora's (2019) article, How (not) to sign a JSON object[1].<p>In short, nesting trees and signing them is difficult and prone to pitfalls. It's easier if the envelope holds the message as a raw string, and the signing is performed on the raw string.<p>[1]: <a href="https://www.latacora.com/blog/2019/07/24/how-not-to/" rel="nofollow">https://www.latacora.com/blog/2019/07/24/how-not-to/</a>
Isn't the simpler conclusion here that one should look for the signature where it is supposed to be? Instead of using an excessively general XPath like "//ds:Signature" that might find any signature in any unexpected location...
Its kind of annoying to explain the vulnerability in a blog post and then omit the parser differential in question.<p>It is like writing the introduction to a story and omitting the climax.
Don't use SAML, mostly because it uses XMLDSig. Don't use XMLDSig because it's hard to get usefully right and easy to get dangerously wrong.
BlueSky post with a video showing the vulnerability:<a href="https://bsky.app/profile/ulldma.bsky.social/post/3lkbi6rasl22a" rel="nofollow">https://bsky.app/profile/ulldma.bsky.social/post/3lkbi6rasl2...</a>
Interesting vulnerability! It's a classic example of how seemingly small differences in implementation (REXML vs Nokogiri) can lead to significant security holes. Kudos to Peter Stöckli and ahacker1 for finding it!<p>I wonder how many other libraries are vulnerable to similar parser differential attacks. It's a good reminder to be extremely careful when dealing with XML and SAML, which are complex beasts at the best of times. As asmor pointed out, Github's SAML implementation has other issues too. It seems like SAML is just inherently difficult to get right.<p>Also, to the person who suggested not mixing personal and professional stuff in the same Github account: wise words! I've seen that cause headaches more than once.
I’m aware of the reputation of XML signatures, but it’s the first time I read about technical details, and they make my head spin.<p>Q: Is there any non-legacy reason to use SAML instead of libsodium’s public key authenticated encryption (crypto_box)?<p>Another Q: Is there any non-theoretical risk of parser differential when using libsodium’s cyrpto_box on one end and Golang’s x/crypto/nacl/box on the other end?
This is an example of a parser mismatch vulnerability.<p>Related submission a year ago: <a href="https://news.ycombinator.com/item?id=38743029">https://news.ycombinator.com/item?id=38743029</a>
Maybe asking stupid question, but would older versions of puppet be affected (like 6?). Also is there a site to check deps down to what maybe affected?
I'm working on refactoring RubySaml right now so that it uses pure Nokogiri XML parser, which would have avoided at least one of these CVEs. It's really a mess because the current way things work RubySaml is subclassing REXML::Document, which you can't do in Nokogiri, and in the process I have found 15 year old bugs in JRuby Nokogiri, which the maintainer @flavorjones was very responsive and merged my patch. Anyway, fun times.