src: fix base64 decoding #11995

seishun · 2017-03-22T21:34:41Z

Make sure trailing garbage is not treated as a valid Base64 character.

Fixes: #11987

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows [commit guidelines][]

Affected core subsystem(s)

src

cc @bnoordhuis @jorangreef

seishun · 2017-03-22T21:47:20Z

Do let me know if someone has a better idea for the test.

addaleax · 2017-03-22T22:07:24Z

test/parallel/test-buffer-alloc.js

@@ -464,6 +464,9 @@ assert.strictEqual(
 // Regression test for https://github.com/nodejs/node/issues/3496.
 assert.strictEqual(Buffer.from('=bad'.repeat(1e4), 'base64').length, 0);

+// Regression test for https://github.com/nodejs/node/issues/11987.
+assert.strictEqual(Buffer.from("w0  ", 'base64').length, 1);


I think a good test would be

assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from('w0', 'base64'));

and/or checking the actual bytes?

Thanks @seishun and @addaleax.

The test is good. Agree on checking the actual bytes:

assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from([195]));

Both of the suggested tests are worthwhile...

assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from('w0', 'base64')); assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from([195]));

And also from the issue:

assert.strictEqual( Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'), '3v6YpUFyK0/hitA2tCDIfdYKw0g=' );

addaleax · 2017-03-31T18:42:11Z

@seishun Took the liberty of pushing to your branch, hope that’s okay. :) Rebased & tests added from comments.

CI: https://ci.nodejs.org/job/node-test-commit/8812/

I’m going to land this if the CI comes back green. @aqrln Heads up, this is going to conflict with your PRs. ;)

seishun · 2017-03-31T18:54:37Z

test/parallel/test-buffer-alloc.js

+  Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'),
+  '3v6YpUFyK0/hitA2tCDIfdYKw0g='
+);
+


@addaleax I think adding all these tests is redundant. This one conveys intent the best:

assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from('w0', 'base64'));

If this test passes but one of the other proposed tests fails, that means base64 encoding is fundamentally broken, which would cause one of the many existing tests to fail.

@seishun It’s your PR and I’m not going to tell you what to do. 😄 But really, having slightly more tests than necessary won’t hurt anyone…

addaleax · 2017-03-31T19:26:59Z

@seishun CI is green … I’d say you can merge this with any non-empty subset of the tests here added. I’d prefer all but every assertion here should be sufficient as a regression test for the particular issue this fixes.

aqrln · 2017-03-31T19:28:15Z

@addaleax thanks for the heads up! It'll conflict with only one of the PRs, and the conflict will be easy to fix, so I'll rebase it as soon as this one lands :)

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

seishun · 2017-03-31T20:02:05Z

Landed in 7e0c3ab.

Make sure trailing garbage is not treated as a valid base64 character. Fixes: nodejs#11987 PR-URL: nodejs#11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2017-04-18T23:21:55Z

Cherry-picked to v6. Needed manual backport for v4: 83a856af6d

please lmk if you see any issues

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12497

Make sure trailing garbage is not treated as a valid base64 character. Fixes: nodejs/node#11987 PR-URL: nodejs/node#11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 22, 2017

addaleax approved these changes Mar 22, 2017

View reviewed changes

addaleax added lts-watch-v4.x labels Mar 23, 2017

jasnell added this to the 8.0.0 milestone Mar 27, 2017

seishun commented Mar 31, 2017

View reviewed changes

src: fix base64 decoding

7e0c3ab

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

seishun closed this Mar 31, 2017

seishun merged commit 7e0c3ab into nodejs:master Mar 31, 2017

seishun deleted the base64-fix branch March 31, 2017 20:02

aqrln mentioned this pull request Mar 31, 2017

buffer: optimize decoding wrapped base64 data #12146

Closed

3 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017

src: fix base64 decoding

ffbcfdf

Make sure trailing garbage is not treated as a valid base64 character. Fixes: nodejs#11987 PR-URL: nodejs#11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017

src: fix base64 decoding

6aeaf21

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 18, 2017

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017

src: fix base64 decoding

eb393f9

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017

src: fix base64 decoding

1a63321

Make sure trailing garbage is not treated as a valid base64 character. Fixes: #11987 PR-URL: #11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>

This was referenced Apr 19, 2017

v6.10.3 proposal #12498

Merged

v4.8.3 proposal #12499

Merged

addaleax mentioned this pull request Jun 1, 2017

result of Buffer.from with base64 is different between versions nodejs/help#651

Closed

seishun mentioned this pull request Jun 14, 2017

src: replace macro with lambda #13676

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix base64 decoding #11995

src: fix base64 decoding #11995

seishun commented Mar 22, 2017 •

edited

Loading

seishun commented Mar 22, 2017

addaleax Mar 22, 2017

jorangreef Mar 24, 2017

jasnell Mar 24, 2017

targos Mar 24, 2017

addaleax commented Mar 31, 2017

seishun Mar 31, 2017 •

edited

Loading

addaleax Mar 31, 2017

addaleax commented Mar 31, 2017

aqrln commented Mar 31, 2017

seishun commented Mar 31, 2017

MylesBorins commented Apr 18, 2017 •

edited

Loading

src: fix base64 decoding #11995

src: fix base64 decoding #11995

Conversation

seishun commented Mar 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

seishun commented Mar 22, 2017

addaleax Mar 22, 2017

Choose a reason for hiding this comment

jorangreef Mar 24, 2017

Choose a reason for hiding this comment

jasnell Mar 24, 2017

Choose a reason for hiding this comment

targos Mar 24, 2017

Choose a reason for hiding this comment

addaleax commented Mar 31, 2017

seishun Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax Mar 31, 2017

Choose a reason for hiding this comment

addaleax commented Mar 31, 2017

aqrln commented Mar 31, 2017

seishun commented Mar 31, 2017

MylesBorins commented Apr 18, 2017 • edited Loading

seishun commented Mar 22, 2017 •

edited

Loading

seishun Mar 31, 2017 •

edited

Loading

MylesBorins commented Apr 18, 2017 •

edited

Loading