crypto: fix size_t/int regression node_crypto

[jasnell]

#31406 introduced a regression in Hash and Hmac update operations.

Haven't looked yet, but it's possible that this also affects other Stream-based crypto operations (e.g. sig, verify, etc) Definitely impacted... adding those to the changeset here.

/cc @addaleax @bnoordhuis

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[nodejs-github-bot]

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

[jasnell]

@addaleax ... Please take a look, I added changes for SignBase and PublicKeyCipher.

CipherBase also technically has a challenge here also but it doesn't segfault, the cipher update operation just fails with an error, so definitely less critical.

[nodejs-github-bot]

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

[addaleax]

Is there any reason not to update this for CipherBase as well, though? Semantically size_t should be the right type here

[jasnell]

Is there any reason not to update this for CipherBase as well, though? Semantically size_t should be the right type here

No, not really a good reason not to. We should add a similar size check tho.

[jasnell]

Ok @addaleax, added a fixup for CipherBase Update also.

[nodejs-github-bot]

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

[bnoordhuis]

I think it would be better to handle this right after the JS -> C++ transition. OpenSSL by and large uses ints for sizes and that's not something we can change. Concretely, I'd check like this:

void CryptoOp(const FunctionCallbackInfo<Value>& args) {
  // ...
  if (!args[0]->IsInt32()) {
    // throw TypeError
  }
  int32_t size = args[0]->Int32Value(context).FromJust();
  if (size < 0) {
    // throw TypeErorr or RangeError
  }
  // now either cast to size_t (because known-good range) or stick with int32_t
}

That way, code deeper down can safely assume sizes are in a known-good range.

With this PR, the range checking is too much all over the place, making it harder to get it right.

[bnoordhuis]

Forgot to mention: the alternative is to have Node.js transparently break up operations over ranges > INT_MAX into smaller ones but that's error prone, might not always be possible, and probably so exceedingly rare as to not be worth the effort.

[jasnell]

With this PR, the range checking is too much all over the place, making it harder to get it right.

In a separate PR I'm making much more far reaching changes that should make it significantly easier to deal with moving forward. It does not yet centralize the type checking / length checking but that's something that can be done easily.

I am perfectly fine with an across-the-board restriction for all crypto operation lengths to be <= INT_MAX

Forgot to mention: the alternative is to have Node.js transparently break up operations over ranges > INT_MAX into smaller ones but that's error prone, might not always be possible, and probably so exceedingly rare as to not be worth the effort.

Yeah, I had considered this also and ruled it out for the same reason.

[bnoordhuis]

I am perfectly fine with an across-the-board restriction for all crypto operation lengths to be <= INT_MAX

I think that'd be best. @nodejs/crypto?

[jasnell]

I'll work on that in the crypto refactor I'm doing and will get it backported for 14 and 12 also. Thanks Ben.

[jasnell]

#35093 landed that includes these fixes