Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit transforms for signed nodes #595

Merged
merged 3 commits into from Jun 17, 2021
Merged

Limit transforms for signed nodes #595

merged 3 commits into from Jun 17, 2021

Conversation

pp-ps
Copy link
Contributor

@pp-ps pp-ps commented May 19, 2021

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
Copy link
Collaborator

cjbarth commented May 19, 2021

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.

@markstos
Copy link
Contributor

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
Copy link
Contributor Author

pp-ps commented May 20, 2021

@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
Copy link
Collaborator

cjbarth commented May 20, 2021

@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
Copy link
Contributor Author

pp-ps commented May 20, 2021

@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
Copy link
Contributor Author

pp-ps commented May 24, 2021 via email

@pp-ps
Copy link
Contributor Author

pp-ps commented May 28, 2021

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
Copy link
Contributor

@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
Copy link
Collaborator

cjbarth commented May 31, 2021

@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
Copy link
Contributor Author

pp-ps commented May 31, 2021

@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
Copy link
Contributor Author

pp-ps commented Jun 3, 2021

@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.

src/node-saml/saml.ts Outdated Show resolved Hide resolved
cjbarth
cjbarth previously approved these changes Jun 16, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Jun 16, 2021

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.

@cjbarth cjbarth changed the title limit transforms for signed node Limit transforms for signed nodes Jun 17, 2021
@cjbarth cjbarth merged commit f1e00b6 into node-saml:master Jun 17, 2021
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants