Replies: 1 comment 1 reply
-
I've created #195 to address your observation. I put the multiple root check after the check for proper encoding as that allows the error messages to be related to the problem more directly. |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
About this approach c1f275c
Is there any scenario where
node-saml
should continue to process incoming message if it has more than one root?If
node-saml
is configured withwantAuthnResponseSigned = false
(note:3.x
series ofpassport-saml
is effetively having this valuefalse
) following code block would send potentially harmful XML message "downstream":node-saml/src/saml.ts
Lines 692 to 704 in c1f275c
I.e. would it be better to move "multiple roots check" from its current location to this line:
node-saml/src/saml.ts
Line 676 in c1f275c
and stop processing message immediately if it has more than one root (because it is not well-formed XML).
Related comment from owasp pages:
https://cheatsheetseries.owasp.org/cheatsheets/SAML_Security_Cheat_Sheet.html#validate-signatures
Unfortunately javascript ecosystem doesn't seem to have pure-javascript validating XML parser (or parser which would check whether XML document is well-formed see e.g. jindw/xmldom#150 which applies also to @xmldom/xmldom).
sidenote:
samlify
project has taken pluggable validator approach:https://github.com/tngan/samlify/blob/0950407d57bb1c5a93985a1d9b1bd2fd9182d1d5/README.md#installation
Beta Was this translation helpful? Give feedback.
All reactions