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

Hannes-Magnusson-CK · 2017-12-14T23:59:06Z

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

danbev · 2017-12-15T06:28:26Z

src/node_crypto.cc

@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
 }


+static void addFingerprintDigest(unsigned char *md,


Nit: could you make the * left-leaning?

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

bnoordhuis

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.

bnoordhuis · 2017-12-28T21:35:59Z

src/node_crypto.cc

@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
 }


+static void addFingerprintDigest(unsigned char* md,


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

bnoordhuis · 2017-12-28T21:42:32Z

src/node_crypto.cc

+    fingerprint[(3*(md_size-1))+2] = '\0';
+  } else {
+    fingerprint[0] = '\0';
+  }


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

bnoordhuis · 2017-12-28T21:44:17Z

src/node_crypto.cc

+
+    int size = i2d_RSA_PUBKEY(rsa, nullptr);
+    Local<Object> pubbuff = Buffer::New(env, size).ToLocalChecked();
+    unsigned char *pubserialized = reinterpret_cast<unsigned char *>(


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

bnoordhuis · 2017-12-28T22:06:47Z

src/node_crypto.cc

@@ -1879,6 +1898,13 @@ static Local<Object> X509ToObject(Environment* env, X509* cert) {
              String::NewFromUtf8(env->isolate(), mem->data,
                                  String::kNormalString, mem->length));
    USE(BIO_reset(bio));
+
+    int size = i2d_RSA_PUBKEY(rsa, nullptr);


Can you CHECK_GE(size, 0) here?

bnoordhuis · 2017-12-28T22:07:24Z

src/node_crypto.cc

-    info->Set(env->fingerprint_string(),
+      addFingerprintDigest(md, md_size, fingerprint);
+      info->Set(env->fingerprint_string(),
+              OneByteString(env->isolate(), fingerprint));


Style: can you line up the arguments?

bnoordhuis · 2017-12-28T22:11:53Z

doc/api/https.md

+
+const options = {
+  checkServerIdentity: function(host, cert) {
+    /* Make sure the certificate is issued to the host we are connected to */


Can you use // comments here?

bnoordhuis · 2017-12-28T22:12:39Z

doc/api/https.md

+    } while (cert.fingerprint256 !== lastprint256);
+
+  },
+  hostname: 'github.com',


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

bnoordhuis · 2017-12-28T22:16:19Z

doc/api/https.md

+    const pubkey256 = 'pL1+qb9HTMRZJmuC/bB/ZI9d302BYrrqiVuRyW+DGrU=';
+    hash = crypto.createHash('sha256');
+    hash.update(cert.pubkey);
+    if (hash.digest('base64') !== pubkey256) {


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.

bnoordhuis · 2017-12-28T22:24:33Z

src/node_crypto.cc

  unsigned char md[EVP_MAX_MD_SIZE];
+  unsigned int md_size;
+  char fingerprint[EVP_MAX_MD_SIZE * 3];


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.)

bnoordhuis · 2017-12-28T22:28:53Z

src/node_crypto.cc

@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
 }


+static void addFingerprintDigest(unsigned char* md,
+                                 unsigned int md_size,
+                                 char* fingerprint) {


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

joyeecheung · 2018-01-17T17:33:51Z

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

Hannes-Magnusson-CK · 2018-01-17T17:39:27Z

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.

Hannes-Magnusson-CK

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.

Hannes-Magnusson-CK · 2018-01-19T22:36:01Z

doc/api/https.md

+
+const options = {
+  checkServerIdentity: function(host, cert) {
+    /* Make sure the certificate is issued to the host we are connected to */


Hannes-Magnusson-CK · 2018-01-19T22:38:18Z

doc/api/https.md

+    const pubkey256 = 'pL1+qb9HTMRZJmuC/bB/ZI9d302BYrrqiVuRyW+DGrU=';
+    hash = crypto.createHash('sha256');
+    hash.update(cert.pubkey);
+    if (hash.digest('base64') !== pubkey256) {


Done, good idea.

Hannes-Magnusson-CK · 2018-01-19T22:38:53Z

doc/api/https.md

+    } while (cert.fingerprint256 !== lastprint256);
+
+  },
+  hostname: 'github.com',


Hannes-Magnusson-CK · 2018-01-19T22:39:41Z

src/node_crypto.cc

@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
 }


+static void addFingerprintDigest(unsigned char* md,


Hannes-Magnusson-CK · 2018-01-19T22:42:05Z

src/node_crypto.cc

@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
 }


+static void addFingerprintDigest(unsigned char* md,
+                                 unsigned int md_size,
+                                 char* fingerprint) {


Nice. Good catch

Hannes-Magnusson-CK · 2018-01-19T22:58:19Z

src/node_crypto.cc

+
+    int size = i2d_RSA_PUBKEY(rsa, nullptr);
+    Local<Object> pubbuff = Buffer::New(env, size).ToLocalChecked();
+    unsigned char *pubserialized = reinterpret_cast<unsigned char *>(


Hannes-Magnusson-CK · 2018-01-19T22:58:54Z

src/node_crypto.cc

  unsigned char md[EVP_MAX_MD_SIZE];
+  unsigned int md_size;
+  char fingerprint[EVP_MAX_MD_SIZE * 3];


Hannes-Magnusson-CK · 2018-01-19T23:00:15Z

src/node_crypto.cc

-    info->Set(env->fingerprint_string(),
+      addFingerprintDigest(md, md_size, fingerprint);
+      info->Set(env->fingerprint_string(),
+              OneByteString(env->isolate(), fingerprint));


Hannes-Magnusson-CK · 2018-01-19T23:00:38Z

test/parallel/test-tls-peer-certificate.js

 const fixtures = require('../common/fixtures');
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+  return;


Hannes-Magnusson-CK · 2018-01-19T23:09:35Z

test/parallel/test-tls-peer-certificate.js

+    ':4C:A4:00:53:93:A9:66:07:A7:BC:13:32'
+  );
+
+  /* SHA256 fingerprint of the public key */


Refactored it into sha256 function as you pointed out earlier

bnoordhuis

Thanks, LGTM with some closing comments.

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

bnoordhuis · 2018-01-22T22:27:07Z

doc/api/https.md

+     * Print the certificate and public key fingerprints of all certs in the
+     * chain. Its common to pin the public key of the issuer on the public
+     * internet, while pinning the public key of the service in sensitive
+     * environments.


Can you use // comments here?

bnoordhuis · 2018-01-22T22:28:26Z

doc/api/https.md

+     */
+    do {
+      console.log('Subject Common Name: ' + cert.subject.CN);
+      console.log('\tCertificate SHA256 fingerprint: ' + cert.fingerprint256);


,, 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

Hannes-Magnusson-CK

Thanks for the thorough review !

Hannes-Magnusson-CK · 2018-01-23T18:34:02Z

doc/api/https.md

+     * Print the certificate and public key fingerprints of all certs in the
+     * chain. Its common to pin the public key of the issuer on the public
+     * internet, while pinning the public key of the service in sensitive
+     * environments.


Hannes-Magnusson-CK · 2018-01-23T18:35:06Z

doc/api/https.md

+     */
+    do {
+      console.log('Subject Common Name: ' + cert.subject.CN);
+      console.log('\tCertificate SHA256 fingerprint: ' + cert.fingerprint256);


magic =)
done

Hannes-Magnusson-CK · 2018-01-23T18:35:38Z

doc/api/https.md

+     */
+    do {
+      console.log('Subject Common Name: ' + cert.subject.CN);
+      console.log('\tCertificate SHA256 fingerprint: ' + cert.fingerprint256);


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

bnoordhuis · 2018-01-23T22:17:29Z

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

BridgeAR · 2018-02-01T09:47:36Z

@Hannes-Magnusson-CK this needs a rebase.

Hannes-Magnusson-CK · 2018-02-01T22:01:04Z

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

BridgeAR · 2018-02-07T03:43:49Z

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

Hannes-Magnusson-CK · 2018-02-07T05:35:17Z

Simple rebase this time, done :)

BridgeAR · 2018-02-07T11:06:34Z

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

BridgeAR · 2018-02-10T13:56:20Z

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.

PR-URL: nodejs#17690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-03-06T20:51:43Z

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.

MylesBorins · 2018-03-06T20:53:05Z

it looks like 7876aeb already made itself into a release

Hannes-Magnusson-CK · 2018-03-06T22:27:05Z

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

MylesBorins · 2018-03-20T01:44:13Z

@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

Hannes-Magnusson-CK · 2018-03-20T03:34:56Z

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

MylesBorins · 2018-03-20T14:57:50Z

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

targos · 2018-03-24T19:34:18Z

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

PR-URL: #17690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Include example on how to pin certificate and/or public key PR-URL: #17690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Include example on how to pin certificate and/or public key PR-URL: nodejs#17690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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

MylesBorins · 2018-08-07T18:56:08Z

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

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 pending requests for changes, 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 pending requests for changes, 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 pending requests for changes, 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 pending requests for changes, and a CI started. backport-requested-v9.x labels Mar 24, 2018

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

		@@ -1795,6 +1795,25 @@ static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
		}


		static void addFingerprintDigest(unsigned char *md,

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

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

Conversation

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

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Jan 17, 2018

Hannes-Magnusson-CK commented Jan 17, 2018

Hannes-Magnusson-CK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hannes-Magnusson-CK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 23, 2018

BridgeAR commented Feb 1, 2018

Hannes-Magnusson-CK commented Feb 1, 2018

BridgeAR commented Feb 7, 2018

Hannes-Magnusson-CK commented Feb 7, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 10, 2018

MylesBorins commented Mar 6, 2018

MylesBorins commented Mar 6, 2018

Hannes-Magnusson-CK commented Mar 6, 2018

MylesBorins commented Mar 20, 2018

Hannes-Magnusson-CK commented Mar 20, 2018

MylesBorins commented Mar 20, 2018

targos commented Mar 24, 2018

MylesBorins commented Aug 7, 2018

Hannes-Magnusson-CK commented Dec 14, 2017 •

edited

Loading