Limit transforms for signed nodes

[pp-ps]

As far as I know a WebSSO profile shouldn't have more than 2 Transform, at most an "enveloped-signature" and an exclusive canonicalization transform. A SAMLResponse can force xml-crypto to needlessly spend time, bound only by POST size constraints.

All tests pass.

[cjbarth]

Can you please reference where in the SAML spec this is recommended? Or, if you have a security paper reference that would be good too.

[pp-ps]

I think this one might be good:
https://www.w3.org/TR/xmldsig-bestpractices/Overview_diff.html#too-many-tranforms

I have not looked much at how other implementations have dealt with this specific concern, but I've found that Spring Security SAML is using OpenSAML, which in turn is putting a limit of two:
https://github.com/spring-projects/spring-security-saml/blob/08a236e1c65c4fb559282a7f16115e6f122d77b0/core/src/main/java/org/springframework/security/saml/websso/AbstractProfileBase.java#L263
https://git.shibboleth.net/view/?p=java-opensaml.git;a=blob;f=opensaml-saml-impl/src/main/java/org/opensaml/saml/security/impl/SAMLSignatureProfileValidator.java;h=d38caf044cfa8fc4bb98e79e45088424b64eee80;hb=HEAD#l218

[markstos]

I agree with the concept. I have not looked closely at the Xpath to confirm it's correct.

@pp-ps Thanks for helping to improve the project security.

[pp-ps]

@markstos I'm not knowledgeable enough on query optimization, but I feel it kinda works for my local test cases.
Test time grows slowly with the SAMLResponse size, but without the fix the same payload triggers a timeout.

[cjbarth]

@pp-ps, is this what you are trying to defend against: https://www.w3.org/TR/2013/NOTE-xmldsig-bestpractices-20130411/#xslt-denial ?

[pp-ps]

@cjbarth Look at section "2.1.7 Example: Denial of service caused by too many transforms" (but skip the XPath part).
The fragment on your link is pointing me to "2.1.1 Example: XSLT transform that causes denial of service", which I feel is more a xml-crypto concern (it would need first to support the http://www.w3.org/TR/1999/REC-xslt-19991116 Transform algorithm to be potentially vulnerable).

I'm a bit shy about pushing a test as a PoC but I've been successful in triggering timeouts (> 2000ms in tests) with a specific Transform repeated hundreds of times. We can move to email for that though.

[pp-ps]

I've pushed to another branch on my repo not to pollute the PR: https://github.com/pp-ps/passport-saml/commits/timeout After all, we have disclosed pretty much everything already in this conversation.

[pp-ps]

I've pushed to another branch on my repo not to pollute the PR: https://github.com/pp-ps/passport-saml/commits/timeout

@cjbarth @markstos
Have you had a chance to look at the tests in the timeout branch above?

[markstos]

@pp-ps If we are limiting the number of transforms to 2, why not write a test with 3 or 5 transforms, since it would run faster and produce the same result?

[cjbarth]

@pp-ps I see you reverted the changes on your branch. Have you found something out about them that causes you to change your mind?

[pp-ps]

@markstos that's doable, I'm OOO but when back I'll push a faster test (3/5 transforms) to this PR branch. I created the test on the "timeout" branch with thousands of Transforms to show the potential for DOS abuse (CPU is busy doing pointless work).
@cjbarth I think there is a misunderstanding, I reverted my commit only on the "timeout" branch to show passport-saml default behavior with a lot of Transforms, namely a timeout of over 2000ms.

[pp-ps]

@markstos @cjbarth I've just pushed a quick test. If you have checked the behavior on the timeout branch and agree that this can be abused by unauthenticated requests (limited by POST size only), I would suggest a minor release now that all the details are public.

[cjbarth]

There is one little spelling change, otherwise, I think this might be something to land and then to release. We will also need this duplicated against node-saml so they don't get out-of-sync during the transition.