Review requested:

• @nodejs/crypto

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

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

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

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.

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

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

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.

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.

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.

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?

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

#35093 landed that includes these fixes