-
Notifications
You must be signed in to change notification settings - Fork 29k
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
buffer: fix DoS vector in atob #51670
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,10 +23,8 @@ | |
|
||
const { | ||
Array, | ||
ArrayFrom, | ||
ArrayIsArray, | ||
ArrayPrototypeForEach, | ||
ArrayPrototypeIndexOf, | ||
MathFloor, | ||
MathMin, | ||
MathTrunc, | ||
|
@@ -1255,35 +1253,41 @@ function btoa(input) { | |
throw new ERR_MISSING_ARGS('input'); | ||
} | ||
input = `${input}`; | ||
let acc = 0; | ||
for (let n = 0; n < input.length; n++) { | ||
if (input[n].charCodeAt(0) > 0xff) | ||
throw lazyDOMException('Invalid character', 'InvalidCharacterError'); | ||
acc |= StringPrototypeCharCodeAt(input, n); | ||
} | ||
if (acc & ~0xff) { | ||
throw lazyDOMException('Invalid character', 'InvalidCharacterError'); | ||
} | ||
const buf = Buffer.from(input, 'latin1'); | ||
return buf.toString('base64'); | ||
} | ||
|
||
// Refs: https://infra.spec.whatwg.org/#forgiving-base64-decode | ||
// https://infra.spec.whatwg.org/#ascii-whitespace | ||
// Valid Characters: [\t\n\f\r +/0-9=A-Za-z] | ||
// Lookup table (-1 = invalid, 0 = valid) | ||
/* eslint-disable no-multi-spaces, indent */ | ||
const kForgivingBase64AllowedChars = [ | ||
// ASCII whitespace | ||
// Refs: https://infra.spec.whatwg.org/#ascii-whitespace | ||
0x09, 0x0A, 0x0C, 0x0D, 0x20, | ||
|
||
// Uppercase letters | ||
...ArrayFrom({ length: 26 }, (_, i) => StringPrototypeCharCodeAt('A') + i), | ||
|
||
// Lowercase letters | ||
...ArrayFrom({ length: 26 }, (_, i) => StringPrototypeCharCodeAt('a') + i), | ||
|
||
// Decimal digits | ||
...ArrayFrom({ length: 10 }, (_, i) => StringPrototypeCharCodeAt('0') + i), | ||
|
||
0x2B, // + | ||
0x2F, // / | ||
0x3D, // = | ||
-1, -1, -1, -1, -1, -1, -1, -1, | ||
-1, 0, 0, -1, 0, 0, -1, -1, | ||
-1, -1, -1, -1, -1, -1, -1, -1, | ||
-1, -1, -1, -1, -1, -1, -1, -1, | ||
0, -1, -1, -1, -1, -1, -1, -1, | ||
-1, -1, -1, 0, -1, -1, -1, 0, | ||
0, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, -1, -1, -1, 0, -1, -1, | ||
-1, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, -1, -1, -1, -1, -1, | ||
-1, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, 0, 0, 0, 0, 0, | ||
0, 0, 0, -1, -1, -1, -1, -1, | ||
]; | ||
const kEqualSignIndex = ArrayPrototypeIndexOf(kForgivingBase64AllowedChars, | ||
0x3D); | ||
/* eslint-enable no-multi-spaces, indent */ | ||
|
||
function atob(input) { | ||
// The implementation here has not been performance optimized in any way and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might need to be revised.
|
||
|
@@ -1298,16 +1302,17 @@ function atob(input) { | |
let equalCharCount = 0; | ||
|
||
for (let n = 0; n < input.length; n++) { | ||
const index = ArrayPrototypeIndexOf( | ||
kForgivingBase64AllowedChars, | ||
StringPrototypeCharCodeAt(input, n)); | ||
const ch = StringPrototypeCharCodeAt(input, n); | ||
const val = kForgivingBase64AllowedChars[ch & 0x7f]; | ||
|
||
if (index > 4) { | ||
// The first 5 elements of `kForgivingBase64AllowedChars` are | ||
// ASCII whitespace char codes. | ||
if ((ch | val) & ~0x7f) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to what this line is doing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, will do. It's some non-obvious bitwise magic. It's ensuring two things: that This line could actually be OR'd onto an accumulator and checked after the loop is complete to remove a branch, but I figure people here would be opposed to it due to the "no optimization of legacy functions" philosophy. On that note, I was originally considering making this loop entirely branchless, but I'm guessing that definitely wouldn't be acceptable. It would also probably require a whole series of comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand. I strongly think that we should add comments to the code as well. |
||
throw lazyDOMException('Invalid character', 'InvalidCharacterError'); | ||
} | ||
|
||
if (ch > 0x20) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to here? |
||
nonAsciiWhitespaceCharCount++; | ||
|
||
if (index === kEqualSignIndex) { | ||
if (ch === 0x3d) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to what |
||
equalCharCount++; | ||
} else if (equalCharCount) { | ||
// The `=` char is only allowed at the end. | ||
|
@@ -1318,8 +1323,6 @@ function atob(input) { | |
// Only one more `=` is permitted after the first equal sign. | ||
throw lazyDOMException('Invalid character', 'InvalidCharacterError'); | ||
} | ||
} else if (index === -1) { | ||
throw lazyDOMException('Invalid character', 'InvalidCharacterError'); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you select -1 and 0, instead of 0 and 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment for line 1308.