crypto: add support for IEEE-P1363 DSA signatures #29292

tniessen · 2019-08-24T13:11:44Z

This patch adds support for "IEEE-P1363 signatures" for DSA and ECDSA. If you are thinking "What is that and why doesn't it have a proper name?", you are right to think that.

Currently, OpenSSL produces ASN.1 signatures which are encoded as DER, basically encoding a SEQUENCE of two INTEGER components, r and s. That works nicely and is supported by most crypto APIs including OpenSSL, pycryptodome, Crypto++, JCE, and BouncyCastle. Except, of course, WebCrypto (and .NET).

WebCrypto (and .NET) use an alternative signature format that does not have a name as far as I can tell. It was, however, suggested in IEEE P1363 in 2000, so it is not an entirely new invention, and some frameworks such as Crypto++ and pycryptodome support it. It is stupidly simple, basically just the concatenation of r and s after some padding.

The conversion can also be done in JavaScript and it isn't difficult, an example implementation is in nodejs/webcrypto#18, but since it involves some simple DER / ASN.1 parsing, it would be convenient and more efficient to put it into core. There are other frameworks that support both formats, e.g., pycryptodome and Crypto++.

After giving this a lot of thought, I have a slight preference towards adding this to core, but I would be happy to be convinced of the opposite. And I am totally open to other names for the format than ieee-p1363.

cc @nodejs/crypto @nodejs/security-wg @panva

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
test case with externally generated data (e.g., using WebCrypto)

src/node_crypto.cc

bnoordhuis · 2019-08-25T13:10:25Z

I have a slight preference towards adding this to core

For the record, I'm okay with that.

lib/internal/crypto/sig.js

tniessen · 2019-09-02T20:48:57Z

Are there alternative suggestions for the option name (dsaEncoding) or its values (der / ieee-p1363)? Has someone been able to find a different standard that has been around for some time and mentions this format? (WebCrypto / JOSE / COSE mention it, but they are rather new and certainly unrelated to older frameworks that use this format.)

panva · 2019-09-03T11:52:23Z

@tniessen is there any room for refactoring to improve the performance? From my benchmark testing using something like this yields a couple hundred more ops/s than the built-in implementation from this PR. The difference is small and possibly within the margin of error but my expectation was that built in would perform better.

sign ieee-p1363 via dsaEncoding x 30,310 ops/sec ±0.74% (90 runs sampled)
sign ieee-p1363 via module x 30,898 ops/sec ±0.95% (91 runs sampled)
Fastest is sign ieee-p1363 via module

verify ieee-p1363 via dsaEncoding x 12,875 ops/sec ±0.43% (92 runs sampled)
verify ieee-p1363 via module x 12,956 ops/sec ±0.40% (94 runs sampled)
Fastest is verify ieee-p1363 via module

tniessen · 2019-09-05T18:17:07Z

@panva I'm looking into it. I can see a < 5% performance loss with dsaEncoding: 'ieee-p1363' when compared to dsaEncoding: 'der' for very small payloads. The new function ConvertSignatureToP1363 seems to use about 5% of the CPU time, with the majority (> 3%) coming from ASN1_item_d2i.

My best guess right now is that the performance loss is due to the OpenSSL ASN.1 implementation and a couple of extra allocations. If that is true, I am not sure if there is anything we can do.

nodejs-github-bot · 2019-09-06T02:13:45Z

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

tniessen · 2019-09-13T16:45:42Z

I don't see much room for improvement right now. @bnoordhuis, @danbev, @jasnell are you in favor of landing this as it is, or would you prefer to investigate further before adding this semver-minor feature? We could most likely improve the performance by implementing the conversion ourselves instead of relying on OpenSSL, but I don't see much value in that. If a manual JS implementation is faster than OpenSSL, that might be an argument against landing this in core.

bnoordhuis · 2019-09-16T10:27:03Z

src/node_crypto.cc

+  const unsigned char* sig_data =
+      reinterpret_cast<unsigned char*>(signature.data());
+
+  ECDSA_SIG* asn1_sig = d2i_ECDSA_SIG(nullptr, &sig_data, signature.size());


It'd be nice to use automatic memory management for this. (Grep for DeleteFnPtr in node_crypto.h for examples.)

Ditto line 4913.

bnoordhuis · 2019-09-16T10:30:17Z

src/node_crypto.h

@@ -320,6 +320,13 @@ class ByteSource {
  const char* get() const;
  size_t size() const;

+  inline operator bool() const {


I'm not super fond of type coercion. You save a few keystrokes but introduce new opportunities for subtle bugs.

fhinkel · 2019-11-01T00:33:53Z

Should this remain open?

tniessen · 2019-11-13T23:44:46Z

I am not sure, I didn't get a reply to #29292 (comment).

bnoordhuis · 2019-11-14T08:03:59Z

@tniessen That question is about the performance of the implementation, not the API? If that's the case, I'm okay with landing it as-is.

This adds a test case for the dsaEncoding option using data generated in Chrome 76.

nodejs-github-bot · 2019-11-19T18:27:39Z

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

nodejs-github-bot · 2019-11-20T14:44:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26781/ ✔️

PR-URL: #29292 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen · 2019-11-20T16:56:37Z

Landed in c63af4f.

PR-URL: #29292 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs#29292

Refs: #29292 PR-URL: #30641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #29292 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #29292 PR-URL: #30641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #29292 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #29292 PR-URL: #30641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable changes: New assert APIs The `assert` module now provides experimental `assert.match()` and `assert.doesNotMatch()` methods. They will validate that the first argument is a string and matches (or does not match) the provided regular expression This is an experimental feature. Ruben Bridgewater [#30929](#30929). Advanced serialization for IPC The `child_process` and `cluster` modules now support a `serialization` option to change the serialization mechanism used for IPC. The option can have one of two values: * `'json'` (default): `JSON.stringify()` and `JSON.parse()` are used. This is how message serialization was done before. * `'advanced'`: The serialization API of the `v8` module is used. It is based on the HTML structured clone algorithm. and is able to serialize more built-in JavaScript object types, such as `BigInt`, `Map`, `Set` etc. as well as circular data structures. Anna Henningsen [#30162](#30162). CLI flags The new `--trace-exit` CLI flag makes Node.js print a stack trace whenever the Node.js environment is exited proactively (i.e. by invoking the `process.exit()` function or pressing Ctrl+C). legendecas [#30516](#30516). ___ The new `--trace-uncaught` CLI flag makes Node.js print a stack trace at the time of throwing uncaught exceptions, rather than at the creation of the `Error` object, if there is any. This option is not enabled by default because it may affect garbage collection behavior negatively. Anna Henningsen [#30025](#30025). ___ The `--disallow-code-generation-from-strings` V8 CLI flag is now whitelisted in the `NODE_OPTIONS` environment variable. Shelley Vohr [#30094](#30094). New crypto APIs For DSA and ECDSA, a new signature encoding is now supported in addition to the existing one (DER). The `verify` and `sign` methods accept a `dsaEncoding` option, which can have one of two values: * `'der'` (default): DER-encoded ASN.1 signature structure encoding `(r, s)`. * `'ieee-p1363'`: Signature format `r || s` as proposed in IEEE-P1363. Tobias Nießen [#29292](#29292). ___ A new method was added to `Hash`: `Hash.prototype.copy`. It makes it possible to clone the internal state of a `Hash` object into a new `Hash` object, allowing to compute the digest between updates. Ben Noordhuis [#29910](#29910). Dependency updates libuv was updated to 1.34.0. This includes fixes to `uv_fs_copyfile()` and `uv_interface_addresses()` and adds two new functions: `uv_sleep()` and `uv_fs_mkstemp()`. Colin Ihrig [#30783](#30783). ___ V8 was updated to 7.8.279.23. This includes performance improvements to object destructuring, RegExp match failures and WebAssembly startup time. The official release notes are available at https://v8.dev/blog/v8-release-78. Michaël Zasso [#30109](#30109). New EventEmitter APIs The new `EventEmitter.on` static method allows to async iterate over events. Matteo Collina [#27994](#27994). ___ It is now possible to monitor `'error'` events on an `EventEmitter` without consuming the emitted error by installing a listener using the symbol `EventEmitter.errorMonitor`. Gerhard Stoebich [#30932](#30932). ___ Using `async` functions with event handlers is problematic, because it can lead to an unhandled rejection in case of a thrown exception. The experimental `captureRejections` option in the `EventEmitter` constructor or the global setting change this behavior, installing a `.then(undefined, handler)` handler on the `Promise`. This handler routes the exception asynchronously to the `Symbol.for('nodejs.rejection')` method if there is one, or to the `'error'` event handler if there is none. Setting `EventEmitter.captureRejections = true` will change the default for all new instances of `EventEmitter`. This is an experimental feature. Matteo Collina [#27867](#27867). Performance Hooks are no longer experimental The `perf_hooks` module is now considered a stable API. legendecas [#31101](#31101). Introduction of experimental WebAssembly System Interface (WASI) support A new core module, `wasi`, is introduced to provide an implementation of the [WebAssembly System Interface](https://wasi.dev/) specification. WASI gives sandboxed WebAssembly applications access to the underlying operating system via a collection of POSIX-like functions. This is an experimental feature. Colin Ihrig [#30258](#30258). PR-URL: #31691

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 24, 2019

tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 24, 2019

This comment has been minimized.

Sign in to view

bnoordhuis approved these changes Aug 25, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

bnoordhuis approved these changes Aug 26, 2019

View reviewed changes

danbev approved these changes Aug 27, 2019

View reviewed changes

jasnell reviewed Aug 28, 2019

View reviewed changes

lib/internal/crypto/sig.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jasnell approved these changes Sep 3, 2019

View reviewed changes

tniessen force-pushed the crypto-add-p1363-signatures branch from c3a420d to 0b90663 Compare September 5, 2019 19:48

This comment has been minimized.

Sign in to view

bnoordhuis reviewed Sep 16, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

tniessen added 3 commits November 19, 2019 14:25

crypto: add support for IEEE-P1363 DSA signatures

83ef2e3

test: add dsaEncoding compatibility test

9caf740

This adds a test case for the dsaEncoding option using data generated in Chrome 76.

crypto: fix memory leak in sign()

65dfd30

tniessen force-pushed the crypto-add-p1363-signatures branch from 0b90663 to 65dfd30 Compare November 19, 2019 18:26

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 20, 2019

tniessen closed this Nov 20, 2019

BridgeAR mentioned this pull request Nov 21, 2019

v13.2.0 proposal #30547

Merged

tniessen added a commit to tniessen/node that referenced this pull request Nov 25, 2019

crypto: automatically manage memory for ECDSA_SIG

ee56100

Refs: nodejs#29292

tniessen mentioned this pull request Nov 25, 2019

crypto: automatically manage memory for ECDSA_SIG #30641

Closed

2 tasks

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

panva mentioned this pull request Oct 3, 2020

doc,crypto: added sign/verify method changes about dsaEncoding #35480

Closed

2 tasks

tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021

panva mentioned this pull request Sep 15, 2023

Drop support for old nodejs versions (req v14) auth0/node-jwa#49

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: add support for IEEE-P1363 DSA signatures #29292

crypto: add support for IEEE-P1363 DSA signatures #29292

tniessen commented Aug 24, 2019 •

edited

This comment has been minimized.

bnoordhuis commented Aug 25, 2019

This comment has been minimized.

tniessen commented Sep 2, 2019 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

panva commented Sep 3, 2019 •

edited

tniessen commented Sep 5, 2019 •

edited

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 6, 2019

tniessen commented Sep 13, 2019

bnoordhuis Sep 16, 2019

bnoordhuis Sep 16, 2019

fhinkel commented Nov 1, 2019

tniessen commented Nov 13, 2019

bnoordhuis commented Nov 14, 2019

nodejs-github-bot commented Nov 19, 2019

nodejs-github-bot commented Nov 20, 2019 •

edited by Trott

tniessen commented Nov 20, 2019

crypto: add support for IEEE-P1363 DSA signatures #29292

crypto: add support for IEEE-P1363 DSA signatures #29292

Conversation

tniessen commented Aug 24, 2019 • edited

Checklist

This comment has been minimized.

bnoordhuis commented Aug 25, 2019

This comment has been minimized.

tniessen commented Sep 2, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

panva commented Sep 3, 2019 • edited

tniessen commented Sep 5, 2019 • edited

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 6, 2019

tniessen commented Sep 13, 2019

bnoordhuis Sep 16, 2019

Choose a reason for hiding this comment

bnoordhuis Sep 16, 2019

Choose a reason for hiding this comment

fhinkel commented Nov 1, 2019

tniessen commented Nov 13, 2019

bnoordhuis commented Nov 14, 2019

nodejs-github-bot commented Nov 19, 2019

nodejs-github-bot commented Nov 20, 2019 • edited by Trott

tniessen commented Nov 20, 2019

tniessen commented Aug 24, 2019 •

edited

tniessen commented Sep 2, 2019 •

edited

panva commented Sep 3, 2019 •

edited

tniessen commented Sep 5, 2019 •

edited

nodejs-github-bot commented Nov 20, 2019 •

edited by Trott