Skip to content
This repository was archived by the owner on Aug 28, 2023. It is now read-only.

updated base64url to fix out-of-bounds read vulnerability #382

Closed

Conversation

leoschweizer
Copy link

There is a vulnerability in the base64url dependency which unfortunately did not publish a fix as minor or patch update:

┌────────────┬────────────────────────────────────────────────────────────────────┐
│             │ Out-of-bounds Read                                                 
├────────────┼────────────────────────────────────────────────────────────────────┤
│ Name        │ base64url                                                          
├────────────┼────────────────────────────────────────────────────────────────────┤
│ CVSS        │ 7.1 (High)                                                         
├────────────┼────────────────────────────────────────────────────────────────────┤
│ Installed   │ 2.0.0                                                              
├────────────┼────────────────────────────────────────────────────────────────────┤
│ Vulnerable  │ <3.0.0                                                             
├────────────┼────────────────────────────────────────────────────────────────────┤
│ Patched     │ >=3.0.0                                                            
├────────────┼────────────────────────────────────────────────────────────────────┤
│ Path        │ redacted > passport-azure-ad@3.0.12 >              
│             │ base64url@2.0.0                                                    
├────────────┼────────────────────────────────────────────────────────────────────┤
│ More Info   │ https://nodesecurity.io/advisories/658                             
└────────────┴────────────────────────────────────────────────────────────────────┘

see https://nodesecurity.io/advisories/658

This pull requests thus updates base64url to the latest major version to fix this vulnerability.

@leoschweizer
Copy link
Author

So the CI build does not pass. Looks like the dependency breaks compatibility with older node versions, they have the same error in their ci logs:
https://travis-ci.org/brianloveswords/base64url/jobs/379778757

Not sure what to do with that...

@BlackyWolf
Copy link

@leoschweizer Is it not possible to drop support for for Node 4 and 5 since they're no longer being maintained?

@leoschweizer
Copy link
Author

@BlackyWolf I'm not a maintainer of this project so that's not for me to decide...

@jeffwilcox
Copy link
Contributor

Can someone who maintains this project comment? The Node LTS schedule (https://github.com/nodejs/Release) seems like the best thing to snap to.

At this point I'm receiving warnings both from GitHub about potential vulnerabilities through this plus also when installing passport-azure-ad via the modern NPM version, so it feels worrisome...

Happy to help but some direction would help...

@brunoborges
Copy link

Maybe @lovemaths can help here.

@navyasric
Copy link
Contributor

Adding @nehaagrawal to take a look at this PR.

@jeffwilcox
Copy link
Contributor

Hi @nehaagrawal are you able to take a look at this issue? I am available to help as necessary to help with remediation here, too, because I work on an impacted project and would love to get clean.

@vladbarosan
Copy link

@nehaagrawal can you help here please ? this vulnerability shows everytime npm install is used and this PR has been out for months.

@nehaagrawal
Copy link
Contributor

nehaagrawal commented Aug 14, 2018

@vladbarosan @jeffwilcox @brunoborges @leoschweizer @BlackyWolf This is a breaking change. After updating version of base64url, build is failing for all node.js versions from 4 to 5. Build is passing for only node.js 6.0 and above. If we upgrade the version of base64url, we wouldn't be able to support passport-azure-ad for node.js version 5 and below. Please let us know if you guys are using node.js verison 5 or below.

@nehaagrawal nehaagrawal requested review from manoj-rath and removed request for manoj-rath August 14, 2018 02:42
@leoschweizer
Copy link
Author

@nehaagrawal we are on node 10. I guess the sensible option would be to publish this change as major version bump (after all it is a breaking change) and discourage the continued usage of the current major version. Alternatively you would have to look for a different base64url library which does support these old node versions (or you would have to fork base64url itself and fix the issue in a nonbreaking way).

@vladbarosan
Copy link

@nehaagrawal we are on node:10 and node:8 but with no problem in upgrading.

Note that current Node LTS is 8 with Node 10 being the LTS starting with October. For node 4 even maintenance LTS finished ( https://github.com/nodejs/Release ) so I would be surprised if there is a lot of impact.

But as @leoschweizer mentioned if that is a concern you can just bump the major version.

@nehaagrawal
Copy link
Contributor

@vladbarosan @jeffwilcox @brunoborges @leoschweizer @BlackyWolf I have released 4.0.0 version. I had to create another PR since there were more changes. Please close this PR. Thank you for your patience and sorry it took so long.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants