Skip to content
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: add support for IEEE-P1363 DSA signatures #29292

Closed
wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Aug 24, 2019

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)

@nodejs-github-bot 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 tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 24, 2019
@nodejs-github-bot

This comment has been minimized.

src/node_crypto.cc Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
@bnoordhuis
Copy link
Member

I have a slight preference towards adding this to core

For the record, I'm okay with that.

@nodejs-github-bot

This comment has been minimized.

lib/internal/crypto/sig.js Outdated Show resolved Hide resolved
@tniessen
Copy link
Member Author

tniessen commented Sep 2, 2019

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@panva
Copy link
Member

panva commented Sep 3, 2019

@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
Copy link
Member Author

tniessen commented Sep 5, 2019

@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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

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.

const unsigned char* sig_data =
reinterpret_cast<unsigned char*>(signature.data());

ECDSA_SIG* asn1_sig = d2i_ECDSA_SIG(nullptr, &sig_data, signature.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ditto line 4913.

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

inline operator bool() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fhinkel
Copy link
Member

fhinkel commented Nov 1, 2019

Should this remain open?

@tniessen
Copy link
Member Author

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

@bnoordhuis
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 20, 2019

@Trott 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 added a commit that referenced this pull request Nov 20, 2019
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
Copy link
Member Author

Landed in c63af4f.

@tniessen tniessen closed this Nov 20, 2019
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Nov 21, 2019
tniessen added a commit to tniessen/node that referenced this pull request Nov 25, 2019
addaleax pushed a commit that referenced this pull request Nov 30, 2019
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>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
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>
targos pushed a commit that referenced this pull request Jan 13, 2020
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>
targos pushed a commit that referenced this pull request Jan 13, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
targos pushed a commit that referenced this pull request Feb 11, 2020
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
targos pushed a commit that referenced this pull request Feb 11, 2020
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
@tniessen 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants