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

crypto: throw proper errors if out enc is UTF-16 #12752

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@addaleax
Member

addaleax commented Apr 29, 2017

Throw Errors instead of hard crashing when the .digest() output
encoding is UTF-16.

Fixes: #9817

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)

crypto

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: #9817
@refack

refack approved these changes Apr 29, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn
Member

gibfahn commented Apr 30, 2017

@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki May 1, 2017

Contributor

The doc says that only the following three encoding can be accepted.

The encoding can be 'hex', 'latin1' or 'base64'

Is it better to have white-listed check rather than having only UCS2 check?

Contributor

shigeki commented May 1, 2017

The doc says that only the following three encoding can be accepted.

The encoding can be 'hex', 'latin1' or 'base64'

Is it better to have white-listed check rather than having only UCS2 check?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 1, 2017

Member

@shigeki Yeah, those are the only encodings that actually make some sense here, so whitelisting sounds okay to me; that would be a breaking change, though.

Member

addaleax commented May 1, 2017

@shigeki Yeah, those are the only encodings that actually make some sense here, so whitelisting sounds okay to me; that would be a breaking change, though.

@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki May 1, 2017

Contributor

that would be a breaking change, though.

I think that only utf-16le and other wrong encoding strings would be affected.
But I agree it gets semver major. It is a good chance to have in Node-v8 if possible.
If we need to have this in LTS, this PR is good.

Contributor

shigeki commented May 1, 2017

that would be a breaking change, though.

I think that only utf-16le and other wrong encoding strings would be affected.
But I agree it gets semver major. It is a good chance to have in Node-v8 if possible.
If we need to have this in LTS, this PR is good.

@jasnell

jasnell approved these changes May 1, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 3, 2017

Member

Landed in 6c2daf0

Member

addaleax commented May 3, 2017

Landed in 6c2daf0

@addaleax addaleax closed this May 3, 2017

@addaleax addaleax deleted the addaleax:fix-crypto-9817 branch May 3, 2017

addaleax added a commit that referenced this pull request May 3, 2017

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: #9817
PR-URL: #12752
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: nodejs#9817
PR-URL: nodejs#12752
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

gibfahn added a commit that referenced this pull request Jun 18, 2017

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: #9817
PR-URL: #12752
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

@gibfahn gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 18, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 18, 2017

Member

Landed this on v6.x, seems reasonably safe (node was previously hard crashing so it shouldn't get much worse).

LMK if this was a mistake.

Member

gibfahn commented Jun 18, 2017

Landed this on v6.x, seems reasonably safe (node was previously hard crashing so it shouldn't get much worse).

LMK if this was a mistake.

gibfahn added a commit that referenced this pull request Jun 20, 2017

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: #9817
PR-URL: #12752
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins added a commit that referenced this pull request Jul 11, 2017

crypto: throw proper errors if out enc is UTF-16
Throw `Error`s instead of hard crashing when the `.digest()` output
encoding is UTF-16.

Fixes: #9817
PR-URL: #12752
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

Merged

v6.11.2 proposal #14356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment