crypto: Better documentation for cases where peer's public key is invalid

[Emill]

• Version: 6.11.3
• Platform: Linux
• Subsystem: crypto

The documentation for ecdh.computeSecret doesn't discuss what happens when the other public key doesn't lie on the curve. It just states that either a string with the given encoding or a Buffer will be returned. For other methods in the same class there are explicit documentation that these throws an error if for example the input is not valid for this curve, but not on this one.

If the own private key is static or not re-generated in each exchange, there are attacks that recover the private key. So all implementers need to handle this case as part of implementing a protocol where ECDH is used, by validating the provided public key and gracefully abort if the point is invalid. Using prime256v1 as an example, a first attempt might be to simply check that the buffer length of the input public key is 65 and the first byte is 0x04. But this is not enough since the point must also be checked against the curve equation. Since bignums are needed for this it's non-trivial to do this directly before the call, and one must rely on the library handling this.

Thankfully the actual implementation throws a generic error with the string Failed to translate Buffer to a EC_POINT, at least when I test it with prime256v1. I was hoping for a more catchable-friendly error, at least having a documented type or error code so it's easily distinguished to other errors. As far as my brain goes, I cannot find a single use case where one would not also want to check for this error since it's not a programmer error but always a possible user-input error.

Since people are generally not aware how to use crypto, it's extremely important that the APIs are clear, easy to use and no mistakes are possible. So I was googling around a bit for "createECDH computeSecret" and found some code examples and tutorials. None of them handled this case or didn't even bring up the possibility that an attacker might give an incorrect public key. The only case I found testing this was the official test file. Just guessing, but I'm pretty sure there's a bunch of production systems that will crash if one gives an incorrect public key. And when the programmer then checks the crash log generated I'm not sure he would even know what Failed to translate Buffer to a EC_POINT means.

My proposal is to first change the code to use some kind of typed error so it can be easily identified for this specific case and then update the documentation to catch this error in the example code as well as document in the computeSecret method how the invalid point error is reported. The example currently only uses assert.strictEqual to confirm that the two values are equal.

[bnoordhuis]

Documentation pull request welcome but apropos this suggestion:

to use some kind of typed error

No, no other built-in APIs do that either, crypto or otherwise.

[Emill]

But the fs API has an extended Error type, which has properties like code and errno. There you can easily detect specific errors as they are properly documented.

[bnoordhuis]

Maybe we have a different definition of 'typed error'.

Putting a couple of extra properties on a new Error() instance? That's fine.

CryptoError extends Error? Non-idiomatic.

[Emill]

Yeah sure extra properties on the new Error() instance works.

[j0t3x]

Hi @Emill
I'd like to pick this up as a good first issue. I've already forked, built and tested the master. I would love to get this done and in the mean time understand nodejs deeper.
Any guidance is more than appreciated.

[tniessen]

@bnoordhuis Are you going to act as a mentor here?

[bnoordhuis]

I can.

@j0t3x If you are asking for assistance, can you phrase you questions as closed (or at least directed) questions? "Any guidance" is a bit open-ended.

[j0t3x]

Thanks @bnoordhuis!,

Here it goes:
I see 2 changes, 1 in the code and 1 in the documentation.

For the first one, i have to use a generic error with description for when the public key lies out of the curve, right?
Im not sure on where this change should happen. Heres an attempt of solving it:

From what i saw here, the crypto binding should throw the generic error and i should catch it?.
Should i use throw new errors.Error('ERR_INVALID_OPT_VALUE', 'a', 'b');(i bet this is my mistake, since it doesnt make sense to use -errors- as those are designed for tight specific cases ) or just a new Error( 'with a clear, succinct explanation of why it failed' )

For the second one, i can follow similar explanations like the one in net module notes here? :

The ecdh.computeSecret() method should throw an 'ERROR_NAME' for when the provided public key is invalid.

[bnoordhuis]

From what i saw here, the crypto binding should throw the generic error and i should catch it?

Ideally, return an error code and construct the exception object in JS land.

Should i use throw new errors.Error('ERR_INVALID_OPT_VALUE', 'a', 'b');

Yes, that's the one you want.

i can follow similar explanations like the one in net module

Yep.

[j0t3x]

Sorry to bother @bnoordhuis,

Just to make it crystal clear for me, this implies using a try catch for the 2 error cases coming from crypto to then in the catch, construct the exception for every one of the 2?

Thanks in advance.

[bnoordhuis]

What I mean is that you replace the ThrowError() calls with args.GetReturnValue().Set(...), where ... is the error code.

It's usually a number but it could be a string in this case, as long as you can distinguish it from a genuine return value in JS land (which in this function is a buffer.)

While a try/catch would work, it's a bit inefficient because you end up creating two exception objects - one in C++, one in JS land - and that's fairly heavyweight because exceptions have to capture the stack.

[j0t3x]

Ok, so it's getting clearer now 😄 . That part of the change should happen in node_crypto.cc.
Makes sense to make this arguments numbers? maybe 1 and 2(properly commented), or should i leave the texts as they are.

if (!ecdh->IsKeyPairValid())
    args.GetReturnValue().Set( the error here 1)//Invalid key pair

  if (!r) {
    free(out);
    args.GetReturnValue().Set(the error here 2)//Failed to compute ECDH key
  }

While a try/catch would work, it's a bit inefficient because you end up creating two exception objects - one in C++, one in JS land - and that's fairly heavyweight because exceptions have to capture the stack.

Understood.

Thanks @bnoordhuis

[bnoordhuis]

Returning strings seems like the least-effort path. Numbers you'd have to map back to error messages first.

[j0t3x]

Hi @bnoordhuis,

Im almost done with the changes, just have a few questions:

• changes in c++ are in the ComputeSecret function, but the thrown exceptions are in BufferToPoint function. What im doing here is let this be thrown and dont throwing this to then catch the nullptr which will be the only nullptr to pass to computeSecret function to then return a string with the errorcode.
• In JS land i throw an error which i declared in errors.js and in errors.md, i put the code again to have code clarity( but i send it from c++, is it ok?)
• In the test-crypto-dh.js i replaced the assert.throws for common.expectsError to catch the correct errorcode.

Are those correct?

Thanks in advance.

[bnoordhuis]

Sounds alright to me and if not, I'll point it out during review. :-)

[j0t3x]

Hi @bnoordhuis,

heres the pr.

Thanks to all involved for this 1st stage help :D