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

vuln(NSWG-ECO-403): atob #211

Merged
merged 4 commits into from
Apr 29, 2018
Merged

vuln(NSWG-ECO-403): atob #211

merged 4 commits into from
Apr 29, 2018

Conversation

lirantal
Copy link
Member

@lirantal lirantal commented Apr 8, 2018

No description provided.

@lirantal lirantal self-assigned this Apr 8, 2018
@lirantal lirantal requested a review from ChALkeR April 8, 2018 21:07
@ChALkeR
Copy link
Member

ChALkeR commented Apr 9, 2018

Citing my comment:

The package info is now inconsistent — it claims "engines": { "node": ">= 0.4.0" }, but the code works only on 4.5.0 and above (as they are using Buffer.from without a polyfill). That is not a security issue though.

/cc @coolaj86

"slug": "atob-out-of-bounds-read",
"recommendation": "update atob to 2.1.0 or higher",
"references": "- https://hackerone.com/reports/321686",
"cvss_vector": "CVSS:3.0/AV:P/AC:L/PR:N/UI:N/S:U/C:L/I:N/A:H",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AV:P seems incorrect. AC:H is better fit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you thin AV is N? is this because you'd come up with a use-case where atob() is given a user input originated string?

Also, why AC: H? seems like the buffer vulnerability is of the same nature as others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ChALkeR ^

Copy link
Member

@ChALkeR ChALkeR Apr 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lirantal Should be the same as 309, 391, 392, 393.
Those do not require physical access. There is no reason for this to be AC:P.

AC:H because it's hard to actually reproduce this on real apps — attacker-supplied content should end up there, which requires playing around other logic. I.e. it's not just «feed input over http», it's «try to find where user input is fed to atob and attempt to supply a typed number there». It requires inpecting other components and, perhaps, bypassing them.

I.e. there are other things required to be passed for this to be exploited, this is not trivially exploitable.

Note: I also don't agree on AC:L (and A:N) for 309, but whatever.

ChALkeR
ChALkeR previously requested changes Apr 9, 2018
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CVSS vecor might need fixing.

@lirantal
Copy link
Member Author

@ChALkeR ping for my comment on the CVSS part

@ChALkeR
Copy link
Member

ChALkeR commented Apr 16, 2018

@lirantal Will take a look today, thanks!

@coolaj86
Copy link

Sorry, I've been slow.

I'll update the engines property and republish.

@coolaj86
Copy link

Published as atob@2.1.1

@lirantal
Copy link
Member Author

@coolaj86 still >= 2.1.0 is not vulnerable, right?
@ChALkeR need you to review my comments on this PR

@lirantal
Copy link
Member Author

Ok, updated report.

@lirantal lirantal dismissed ChALkeR’s stale review April 29, 2018 15:56

Applied all required changes

@lirantal lirantal merged commit 0f5cb43 into nodejs:master Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants