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

bump xmldom to 0.5.x since all lower versions have security issue #551

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

eero3
Copy link
Contributor

@eero3 eero3 commented Mar 9, 2021

Description

xmldom versions <= 0.4.0 have security issue. Update xmldom dependency to 0.5.x and bump package version.
More information: GHSA-h6q6-9hqw-rwfv

After this fix as been merged, a new version of passport-saml should be released to npm.

@gugu gugu merged commit 4d2b909 into node-saml:master Mar 9, 2021
@ghost
Copy link

ghost commented Mar 10, 2021

Is there already release scheduled to get this change online?

@markstos
Copy link
Contributor

@gugu are there any blockers to merging this into a 2.x release?

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 10, 2021

The head of master currently has semver major changes in it. If we want this in a 2.x release, then we should pull these changes in to a branch off the last 2.x tag. I'd be happy to help with that if needed, though it won't be today :) In any case, we'll end up branching off the 2.x tag to put in some more deprecation notices for the move to 3.x.

@forty
Copy link
Contributor

forty commented Mar 15, 2021

I created a branch v2.x on my forked repo then open this PR #552

I think a maintainer will have to create a v2.x branch on this repo? Let me know if I can help.

Thanks

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 15, 2021

You have to create the PR against the commit you want to fork off of, for example, the latest 2.x tag. You created that PR to merge against master.

@ghost
Copy link

ghost commented Aug 6, 2021

Just wanted to know was this vulnerability exploitable in passport saml? I am going through https://mattermost.com/blog/securing-xml-implementations-across-the-web/. It seems like we need a round trip so that the signature is validated against different input and email is from different. Do we do that in passport-saml? cc @gugu

@markstos
Copy link
Contributor

markstos commented Aug 6, 2021

@meetsuraj No one has reported that the vuln was exploitable through passport-saml. You are welcome to test yourself.

@forty
Copy link
Contributor

forty commented Aug 6, 2021

I can confirm we are parsing data we have serialized ourselves, so it could very well be exploitable. Same for the new xmldom issue. Short term we should update xmldom whenever it's possible. Longer term we should avoid parsing anything we produced. I have plans, but it's a bit of work ;)

@catamphetamine
Copy link

FYI: The latest version of XMLDOM is:
https://www.npmjs.com/package/@xmldom/xmldom

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 18, 2021

This update has already been made @catamphetamine : https://github.com/node-saml/node-saml/blob/42589290ecb3f800e5129d5d1a49eabf8b2026ae/package.json#L54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants