Skip to content

Commit

Permalink
Merge pull request from GHSA-m974-647v-whv7
Browse files Browse the repository at this point in the history
  • Loading branch information
@cjbarth
cjbarth committed Oct 11, 2022
1 parent 6ba76ba commit 8b7e3f5
Show file tree
Hide file tree
Showing 3 changed files with 1,353 additions and 45 deletions.

6 comments on commit 8b7e3f5

@frumioj
Copy link

@frumioj frumioj commented on 8b7e3f5 Oct 13, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.from(doc.childNodes as NodeListOf<Element>).filter( (n) => n.tagName != null && n.childNodes != null ).length === 1

Why does multiple root elements not get caught by the XML parser (it's an XML parsing error?) prior to signature validation?

@cjbarth
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An XML parser will gladly parse an XML document with multiple roots. Then, a signature can apply to only one root node. However, XPath can traverse multiple root nodes to find authn and authz information. So, one root node may be signed and then another, unsigned node, could contain authn and authz information, which is obviously a problem.

The solution was chosen because there should never be more than one root node with children in a valid SAML XML document. So, limiting things this way makes all further processing much simpler and more secure.

@frumioj
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jindw/xmldom#150 (see the discussion about the fact that xmldom does not follow the standard, and instead allows multiple root nodes without either ignoring the malformed content after the first root node, or erroring). I see why you have chosen the solution you've chosen, but it's still broken at the XML parser level.

@frumioj
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some better solutions:

  • Error on authentication response when an error status code is read (need to stop the parser at the point the status code is read, and then don't bother reading further and cancel the authentication)
  • Parse the first root element only, and error when the second root element is received (don't read the second element at all), returning only the first valid root element, before validating the signature.
  • Fail the assertion received since it cannot be signed by the attacker, if the SP has a key for the signer in metadata, and the response was signed.
  • SAML assertion should never be a root element in an AuthnResponse (should only ever be inside of a samlp:Response).

@cjbarth
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frumioj , I'd love to see a PR with some of these solutions. You can either make the PR against the 3.x branch here, or over at node-saml, which will be what eventually powers a 4.x release here at passsport-saml.

@cjbarth
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frumioj , I've started a PR with some comments from @srd90 here. Feel free to comment there, or even make a PR against that branch with some suggestions.

Please sign in to comment.