A second root element: SAML signature bypass in passport-saml

If you've never had to implement SAML, the shape of it is this. Your application doesn't check the user's password. It bounces the browser to an identity provider, and the IdP checks the password over there. Then it sends the browser back to you with a form POST containing one base64 field called SAMLResponse. Decode that and you get an XML document. Buried in it is an assertion: this is Alice, she authenticated at 09:14, here's her email address. Everything you believe about who is logging in comes from that document. The only reason to believe any of it is one XML digital signature made with the IdP's private key.

XML signatures differ from signing a file with GPG in one way that matters here. An XML signature doesn't sign "the document". It signs specific elements, named by URI inside the <Signature> block itself. So the question "is this response signed?" has no answer. Two other questions do. Is this element signed by a key I trust? Is this element the one I'm about to act on? Keeping those two elements the same is the entire job. Every SAML library bug of the past fifteen years is some version of them coming apart, and the family has a name: signature wrapping.

Felix Wilhelm of Google Project Zero found a clean one in passport-saml. Node applications reach for that library when somebody in sales says the word SSO. The advisory went out on 11 October 2022 as GHSA-m974-647v-whv7, CVE-2022-39299, rated High, CVSS 8.1.

The two selectors#

This is from validatePostResponseAsync in version 3.2.1, the last release before the fix. The response has been base64-decoded into xml and parsed into doc.

const certs = await this.certsToCheck();
// Check if this document has a valid top-level signature
let validSignature = false;
if (this.validateSignature(xml, doc.documentElement, certs)) {
  validSignature = true;
}

const assertions = xpath.selectElements(
  doc,
  "/*[local-name()='Response']/*[local-name()='Assertion']"
);

Then the code that decides whether to trust the assertion it just selected:

if (assertions.length == 1) {
  if (
    (this.options.wantAssertionsSigned || !validSignature) &&
    !this.validateSignature(xml, assertions[0], certs)
  ) {
    throw new Error("Invalid signature");
  }
  return await this.processValidlySignedAssertionAsync(
    assertions[0].toString(),
    xml,
    inResponseTo!
  );
}

Read the two selectors. The signature is checked against doc.documentElement. The assertion is fetched with the XPath /*[local-name()='Response']/.... That means "any element at the top level of the document whose local name is Response, then its Assertion child". In a well-formed XML document those two expressions can only ever land in the same place, because a well-formed XML document has exactly one root element. That is the grammar, not a convention.

passport-saml parsed with @xmldom/xmldom, which doesn't enforce it. Here is what that library's Document does when a child is attached:

insertBefore: function(newChild, refChild){
    ...
    if(this.documentElement == null && newChild.nodeType == ELEMENT_NODE){
        this.documentElement = newChild;
    }
    return _insertBefore(this,newChild,refChild),(newChild.ownerDocument = this),newChild;
}

Set once, on the first element, and never revisited. Feed the parser a string with a second root element after the first. You get a document object with two element children, and documentElement still points at number one. XPath's /* has no such opinion. It matches both.

So the attack is a text file:

<SomethingTheIdpSigned>...IdP signature over this element...</SomethingTheIdpSigned>
<samlp:Response>
  <saml:Assertion>
    <saml:Subject><saml:NameID>admin@example.com</saml:NameID></saml:Subject>
  </saml:Assertion>
</samlp:Response>

validateSignature runs over root number one, which really is signed by the IdP's key, and sets validSignature = true. The XPath then walks past it and finds the Response in root number two. Out comes the attacker's unsigned assertion. Because validSignature is true, the guard (wantAssertionsSigned || !validSignature) evaluates to false, and the assertion is never checked against anything. It goes straight to processValidlySignedAssertionAsync, which in this one case is the wrong name for what just happened.

The wantAssertionsSigned option sits right there in that condition. Turn it on and the assertion gets its own signature check, and the attack dies at that line. The constructor in 3.2.1 sets it with ctorOptions.wantAssertionsSigned ?? false. Unless you asked for it, you didn't have it.

Preconditions#

The advisory states one precondition:

A successful attack requires that the attacker is in possession of an arbitrary IDP signed XML element. Depending on the IDP used, fully unauthenticated attacks (e.g without access to a valid user) might also be feasible if generation of a signed message can be triggered.

It doesn't have to be an assertion, and it doesn't have to be for your service. It has to be a lump of XML carrying a valid signature from a key your service trusts. All it does is get validSignature flipped to true.

The obvious supply is the attacker's own login. Any account at all on that IdP will do. He logs in legitimately, keeps the signed response the browser handed him, staples his own Response after it and sends that. His signed element says he's himself. The element the library actually reads says whatever he likes. That is the whole of the privilege escalation. At an IdP where anyone can trigger a signed message without an account, you don't even need the account.

The fix#

Four lines, in the same place:

// Check if this document has a valid top-level signature which applies to the entire XML document
let validSignature = false;
if (
  this.validateSignature(xml, doc.documentElement, certs) &&
  Array.from(doc.childNodes as NodeListOf<Element>).filter(
    (n) => n.tagName != null && n.childNodes != null
  ).length === 1
) {
  validSignature = true;
}

Count the element children of the document. If there's more than one, a valid signature over documentElement no longer earns validSignature. It can't be shown to cover what the rest of the function is going to read. The updated comment says the same thing: the signature has to apply to the entire XML document.

The odd thing is what it doesn't do. It doesn't reject a document with two roots as malformed, which it plainly is. It only stops that document from being treated as globally signed. The flow falls through to per-assertion signature checking, which then fails, because the attacker's assertion isn't signed. That is a smaller change than "refuse to parse invalid XML". It is also much easier to be confident about in a library that has spent years absorbing whatever real identity providers emit.

Fixed in passport-saml 3.2.2, and in the packages the project was in the middle of splitting into: node-saml and @node-saml/node-saml at 4.0.0-beta.5, @node-saml/passport-saml at 4.0.0-beta.3. The advisory lists one workaround: "Disable SAML authentication."