crypto: forbid setting the PBKDF2 iter count to 0

[tniessen]

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

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

[tniessen]

Not sure whether to mark this as semver-major. It might break code, but that code was most likely not working as intended to begin with.

[sam-github]

I like the better error checking and forcing of passing an explicit value.

I would mark semver-major, people could be using 0 as "use default", and if its been like this for a long time, we don't need to get this change backported to LTS.

[tniessen]

@sam-github I understand that, and I am fine with landing this as semver-major. However, I suspect that most invocations with iterations === 0 are bugs, and in the case that they are not, they are still using a relatively insecure value (probably unknowingly).

I marked this as semver-major, so cc @nodejs/tsc.

[sam-github]

Agreed its probably bugs. If it was resulting in zero iterations actually occurring, that would be terrible, and we would backport it as a sec fix, breaking or not. But since using < 1 results in 1, I don't think its worth forcing on people in stable branches.

[bnoordhuis]

Thanks, Tobias.

[nodejs-github-bot]

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

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2097/

[addaleax]

Landed in 10f5fa7

[muctemi]

What a weird commit. You broke all prev codes. It shouldn't be a problem setting this number explicitly as 0 if someone intentionally did that. In my case, I need that to be 0 due to performance concerns which I have already a random seed and uses this function to hide randomness information. So, I don't need to iterate over. Anyway, you should be careful about commits that you made that can easily break codes.

[tniessen]

@muctemi None of that is true.

What a weird commit.

It seems like this commit led to the discovery of misuse of PBKDF2 in your code. You are welcome.

You broke all prev codes.

No, only code that incorrectly passed 0, which is not a valid value and never behaved as you appear to think it should.

It shouldn't be a problem setting this number explicitly as 0 if someone intentionally did that.

It is a problem. It is not allowed by the PBKDF2 specification, the result would be undefined, and it is unsupported by OpenSSL.

In my case, I need that to be 0 due to performance concerns which I have already a random seed and uses this function to hide randomness information. So, I don't need to iterate over.

If you don't need iteration, then why do you even use PBKDF2? That's not what PBKDF2 was designed for.

Also, as discussed in earlier comments, passing the invalid value 0 never had any benefit over the valid value 1. Check the code if you still don't believe us (it behaves the same, whether iter == 0 or iter == 1).

Anyway, you should be careful about commits that you made that can easily break codes.

We are extremely careful. This was marked as semver-major, which means that it was exluded from all minor and patch releases of existing release lines.