crypto: Expose the public key of a certificate & cert sha256 by Hannes-Magnusson-CK · Pull Request #17690 · nodejs/node · GitHub

Expose the raw public key of the certificate. This is needed for applications to be able to pin the public key rather then the exact certificate. This also makes it a lot easier to implement HPKP, but to be able to do proper HPKP we need to have access the the issuer certificate too, so we are now passing the "detailed" certificate to checkServerIdentity.

The certificate object contains the SHA1 fingerprint of the certificate. That is getting a little date so I've added the SHA256 as cert.fingerprint256.

Also added docs on how to do cert pinning and pubkey pinning.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto
doc

Nit: could you make the * left-leaning?

Ah. Whops :) Thanks!
I've amended the commit

Calling i2d_RSA_PUBKEY() is not very cheap, calling it twice even less so. Since you make it the default, there may be performance implications.

You might be able to claw back some of the overhead with i2d_RSA_PUBKEY_bio() + an in-memory BIO so it's only decoded once, but you'd have to measure that.

Style: should be e.g.AddFingerprintDigest(). md can be const unsigned char*.

thanks!

Since you're here: just fingerprint[3*i] = '\0';?

I'm not sure I understand.
You mean instead of fingerprint[0]? Not sure how that makes it any more readable?

Note that 3*(md_size-1)+2 !== 3*i

Style: unsigned char* (star leans left) on LHS and RHS and line break after the =.

done

Can you CHECK_GE(size, 0) here?

done

Style: can you line up the arguments?

done

Can you use // comments here?

Done

I'd put these before checkServerIdentity, they get kind of obscured now.

done

For legibility/succinctness/DRY, maybe put this in a separate function:

function sha256(s) {
  return crypto.createHash('sha256').update(s).digest('base64');
}

And then you can write:

if (pubkey256 !== sha256(cert.pubkey)) {

And below:

console.log('\tPublic key ping-sha256:', sha256(cert.pubkey));  // also note the , instead of +

Done, good idea.

Since you're here, can you change that to EVP_MAX_MD_SIZE * 3 + 1? There is a (hypothetical) write-after-end in case md_size == EVP_MAX_MD_SIZE. (Hypothetical because that can only happen with SHA-512.)

nice

If you change this to char (*fingerprint)[3 * EVP_MAX_MD_SIZE + 1], then people can never inadvertently pass a buffer that's too small and the compiler is more likely to warn you when you try to write past the end. You index into it with (*fingerprint)[3*i] = ....

Nice. Good catch

@Hannes-Magnusson-CK Can you address the comments by @bnoordhuis ? Thanks!

Whooops. I lost track of this and missed the comments. Thanks for the review @bnoordhuis & @jasnell ! I'll try to address the comments by the end of week.

I believe I've addressed all the feedback.
Thank you @bnoordhuis so much for thorough review!

As for the performance of i2d_RSA_PUBKEY() I haven't been able to reproduce a performance issue there that isn't drowned out by the actual tls handshake itself.

Done

Done, good idea.

done

thanks!

Nice. Good catch

done

nice

done

Refactored it into sha256 function as you pointed out earlier

Thanks, LGTM with some closing comments.

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

Can you use // comments here?

done

,, not +, and then you don't need the blank after the semicolon. Ditto three lines below.

magic =)
done

I also fixed the '\t' to 2spaces has the ci linter complained about tab being used in the output text

Thanks for the thorough review !

done

magic =)
done

I also fixed the '\t' to 2spaces has the ci linter complained about tab being used in the output text

The two CI failures are known flakes. This should be good to go.

@Hannes-Magnusson-CK this needs a rebase.

Rebased on current master, fixed the conflict, and squashed couple of commits

@Hannes-Magnusson-CK I am very sorry but this needs another rebase :/

Simple rebase this time, done :)

CI https://ci.nodejs.org/job/node-test-pull-request/13004/

Hm, it seems like there was a older conflicting PR #16402. That one just landed and the current code has to be rebased. @Hannes-Magnusson-CK I have the feeling this PR is now obsolete but I might be wrong. If so: I am sorry that this was not detected earlier. Otherwise it would be great if you could check what is necessary and what not! Thanks for being so patient and for sticking to it! That is much appreciated.

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

it looks like 7876aeb already made itself into a release

The other 3 commits don't seem to be included in that branch.
@MylesBorins should I follow the guide to create a PR for v9.8.0-proposal?
Or is that something @addaleax is working on? (looks like @addaleax included one of the commits already)

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) #17690 * lib: - v8_prof_processor works again 🎉 (Anna Henningsen) #19059 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) #18949 * src: - handle exceptions in env-\>SetImmediates (James M Snell) #18297 - make process.dlopen() load well-known symbol (Ben Noordhuis) #18934 * trace_events: - add file pattern cli option (Andreas Madsen) #18480 PR-URL: #19181

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) #17690 * http2: - Fixed issues with aborted connections in the HTTP/2 implementation (Anna Henningsen) #18987 #19002 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) #18949 * src: - make process.dlopen() load well-known symbol (Ben Noordhuis) #18934 * trace_events: - add file pattern cli option (Andreas Madsen) #18480 * Added new collaborators: - Chen Gang (MoonBall) https://github.com/MoonBall PR-URL: #19181

@Hannes-Magnusson-CK no need to make a proposal for the release, what would really help would be making sure every commit from this PR made it on to the staging branch

Looks like v10.x-staging is up2date with all the commits here. Thanks @MylesBorins !

To clarify. This PR has not landed in it's entirety on v9.x. Could someone please open up a backport PR

There was just one trivial conflict to solve, no need for a backport

PR-URL: #17690 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Include example on how to pin certificate and/or public key PR-URL: #17690 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable changes: * cluster: - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava) #19165 * crypto: - Expose the public key of a certificate (Hannes Magnusson) #17690 * n-api: - Add `napi_fatal_exception` to trigger an `uncaughtException` in JavaScript (Mathias Buus) #19337 * path: - Fix regression in `posix.normalize` (Michaël Zasso) #19520 * stream: - Improve stream creation performance (Brian White) #19401 * Added new collaborators - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

This is a security release. All Node.js users should consult the security release summary at: https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2018-7158 * CVE-2018-7159 * CVE-2018-7160 Notable changes: * Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that are known to impact Node.js. * **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**: A malicious website could use a DNS rebinding attack to trick a web browser to bypass same-origin-policy checks and allow HTTP connections to localhost or to hosts on the local network, potentially to an open inspector port as a debugger, therefore gaining full code execution access. The inspector now only allows connections that have a browser `Host` value of `localhost` or `localhost6`. * **Fix for `'path'` module regular expression denial of service (CVE-2018-7158)**: A regular expression used for parsing POSIX an Windows paths could be used to cause a denial of service if an attacker were able to have a specially crafted path string passed through one of the impacted `'path'` module functions. * **Reject spaces in HTTP `Content-Length` header values (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside `Content-Length` header values. Such values now lead to rejected connections in the same way as non-numeric values. * **Update root certificates**: 5 additional root certificates have been added to the Node.js binary and 30 have been removed. * cluster: - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava) #19165 * crypto: - Expose the public key of a certificate (Hannes Magnusson) #17690 * n-api: - Add `napi_fatal_exception` to trigger an `uncaughtException` in JavaScript (Mathias Buus) #19337 * path: - Fix regression in `posix.normalize` (Michaël Zasso) #19520 * stream: - Improve stream creation performance (Brian White) #19401 * Added new collaborators - [BethGriggs](https://github.com/BethGriggs) Beth Griggs PR-URL: https://github.com/nodejs-private/node-private/pull/111

PR-URL: nodejs#17690 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Include example on how to pin certificate and/or public key PR-URL: nodejs#17690 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) nodejs#17690 * http2: - Fixed issues with aborted connections in the HTTP/2 implementation (Anna Henningsen) nodejs#18987 nodejs#19002 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) nodejs#18949 * src: - make process.dlopen() load well-known symbol (Ben Noordhuis) nodejs#18934 * trace_events: - add file pattern cli option (Andreas Madsen) nodejs#18480 * Added new collaborators: - Chen Gang (MoonBall) https://github.com/MoonBall PR-URL: nodejs#19181

is this something we would want to backport to v8.x?

This is a security release. All Node.js users should consult the security release summary at: https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2018-7158 * CVE-2018-7159 * CVE-2018-7160 Notable changes: * Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that are known to impact Node.js. * **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**: A malicious website could use a DNS rebinding attack to trick a web browser to bypass same-origin-policy checks and allow HTTP connections to localhost or to hosts on the local network, potentially to an open inspector port as a debugger, therefore gaining full code execution access. The inspector now only allows connections that have a browser `Host` value of `localhost` or `localhost6`. * **Fix for `'path'` module regular expression denial of service (CVE-2018-7158)**: A regular expression used for parsing POSIX an Windows paths could be used to cause a denial of service if an attacker were able to have a specially crafted path string passed through one of the impacted `'path'` module functions. * **Reject spaces in HTTP `Content-Length` header values (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside `Content-Length` header values. Such values now lead to rejected connections in the same way as non-numeric values. * **Update root certificates**: 5 additional root certificates have been added to the Node.js binary and 30 have been removed. * cluster: - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava) nodejs/node#19165 * crypto: - Expose the public key of a certificate (Hannes Magnusson) nodejs/node#17690 * n-api: - Add `napi_fatal_exception` to trigger an `uncaughtException` in JavaScript (Mathias Buus) nodejs/node#19337 * path: - Fix regression in `posix.normalize` (Michaël Zasso) nodejs/node#19520 * stream: - Improve stream creation performance (Brian White) nodejs/node#19401 * Added new collaborators - [BethGriggs](https://github.com/BethGriggs) Beth Griggs PR-URL: https://github.com/nodejs-private/node-private/pull/111

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

Hannes-Magnusson-CK changed the title Sha256 and pubkey crypto: Expose the public key of a certificate & cert sha256 Dec 15, 2017

danbev reviewed Dec 15, 2017

View reviewed changes

Hannes-Magnusson-CK force-pushed the sha256-and-pubkey branch from 128749e to 7ea2aba Compare December 15, 2017 16:46

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 18, 2017

jasnell approved these changes Dec 28, 2017

View reviewed changes

bnoordhuis reviewed Dec 28, 2017

View reviewed changes

BridgeAR added the wip Issues and PRs that are still a work in progress. label Jan 19, 2018

Hannes-Magnusson-CK commented Jan 22, 2018

View reviewed changes

bnoordhuis approved these changes Jan 22, 2018

View reviewed changes

Hannes-Magnusson-CK commented Jan 23, 2018

View reviewed changes

bnoordhuis added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 23, 2018

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 1, 2018

Hannes-Magnusson-CK force-pushed the sha256-and-pubkey branch from 7afb2cb to 6ff9195 Compare February 1, 2018 22:00

Hannes-Magnusson-CK force-pushed the sha256-and-pubkey branch from 6ff9195 to 3afabc9 Compare February 7, 2018 05:34

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 7, 2018

MylesBorins added the backport-requested-v9.x label Mar 6, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

targos removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-requested-v9.x labels Mar 24, 2018

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

crypto: Expose the public key of a certificate & cert sha256 by Hannes-Magnusson-CK · Pull Request #17690 · nodejs/node · GitHub

Navigation Menu

Uh oh!

Uh oh!

crypto: Expose the public key of a certificate & cert sha256 - #17690

crypto: Expose the public key of a certificate & cert sha256#17690Hannes-Magnusson-CK wants to merge 4 commits intonodejs:masternodejs/node:masterfrom Hannes-Magnusson-CK:sha256-and-pubkeyHannes-Magnusson-CK/node:sha256-and-pubkeyCopy head branch name to clipboard

Conversation

Hannes-Magnusson-CK commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 17, 2018

Uh oh!

Hannes-Magnusson-CK commented Jan 17, 2018

Uh oh!

Hannes-Magnusson-CK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crypto: Expose the public key of a certificate & cert sha256#17690
Hannes-Magnusson-CK wants to merge 4 commits into
nodejs:masterfrom
Hannes-Magnusson-CK:sha256-and-pubkey

Hannes-Magnusson-CK commented Dec 14, 2017 •
edited
Loading