Skip to content
This repository was archived by the owner on Oct 23, 2020. It is now read-only.

Throw OperationError if PBKDF2 iterations is zero #29

Merged

Conversation

tniessen
Copy link
Member

Node.js accepts an iteration count of zero, but WebCrypto requires an OperationError. This fixes 468 WPTs.

Node.js accepts an iteration count of zero, but WebCrypto requires an
OperationError.
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM FWIW. Would you say it's a good idea to make it an error in Node.js core too? I can't think of a single legitimate reason why you would want to perform zero iterations and plenty of why an input of 0 is probably an accident.

@tniessen
Copy link
Member Author

@bnoordhuis I think it would be a breaking change. But I tend to agree, and I considered going one step further and suggesting the change to OpenSSL. According to RFC 2898, zero is not a valid parameter, but I am not so sure about NIST SP 800-132. The latter does not seem to explicitely forbid setting iterations to zero.

@tniessen
Copy link
Member Author

tniessen commented Nov 21, 2019

I just looked at OpenSSL, and the implementation in OpenSSL 1.1.1 seems to treat values <= 1 as 1. OpenSSL master, however, appears to forbid such inputs.

Experimentally confirmed: Setting the iteration count to zero in Node.js results in the same output as setting it to one.

I probably should have looked at the documentation first: "Any iter less than 1 is treated as a single iteration."

@tniessen tniessen merged commit f97c0d1 into nodejs:master Nov 21, 2019
@tniessen tniessen deleted the pbkdf2-throw-operationerror-zero-iterations branch November 21, 2019 15:12
tniessen added a commit to tniessen/node that referenced this pull request Nov 24, 2019
RFC 2898 does not permit an iteration count of zero, and OpenSSL 1.1.1
will treat it as one iteration internally.

Future OpenSSL versions will reject such inputs (already on master
branch), but until that happens, Node.js should manually reject them.

Refs: nodejs/webcrypto#29
addaleax pushed a commit to nodejs/node that referenced this pull request Nov 27, 2019
RFC 2898 does not permit an iteration count of zero, and OpenSSL 1.1.1
will treat it as one iteration internally.

Future OpenSSL versions will reject such inputs (already on master
branch), but until that happens, Node.js should manually reject them.

Refs: nodejs/webcrypto#29

PR-URL: #30578
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants