use ECDSA with SHA256 signers and verifiers for elliptic curve algorithms

[shea256]

Please correct me if I'm wrong here, but I've noticed two things that together seem strange:

1. The RSA-SHAxxx hash functions are being used to create the signers and verifiers in createKeySigner and createKeyVerifier.
2. createECDSASigner and createECDSAVerifer are simply wrappers around createKeySigner and createKeyVerifier, with a single modification to reformat the signature

This leads me to believe that the signers and verifiers are performing signing and verifying with the RSA-SHAxxx hash functions provided by Node's built-in crypto library, when they should be using ECDSA with SHA256.

Is there something I'm missing here? Is there another place where the signers and verifiers are being defined?

I came across an issue when I browserify-ed node-jsonwebtoken and noticed that the signing wasn't working.

Thanks!

Reference code:

function createKeySigner(bits) {
 return function sign(thing, privateKey) {
    if (!bufferOrString(privateKey) && !(typeof privateKey === 'object'))
      throw typeError(MSG_INVALID_SIGNER_KEY);
    thing = normalizeInput(thing);
    // Even though we are specifying "RSA" here, this works with ECDSA
    // keys as well.
    const signer = crypto.createSign('RSA-SHA' + bits);
    const sig = (signer.update(thing), signer.sign(privateKey, 'base64'));
    return base64url.fromBase64(sig);
  }
}

function createKeyVerifier(bits) {
  return function verify(thing, signature, publicKey) {
    if (!bufferOrString(publicKey))
      throw typeError(MSG_INVALID_VERIFIER_KEY);
    thing = normalizeInput(thing);
    signature = base64url.toBase64(signature);
    const verifier = crypto.createVerify('RSA-SHA' + bits);
    verifier.update(thing);
    return verifier.verify(publicKey, signature, 'base64');
  }
}

function createECDSASigner(bits) {
  const inner = createKeySigner(bits);
  return function sign() {
    var signature = inner.apply(null, arguments);
    signature = formatEcdsa.derToJose(signature, 'ES' + bits);
    return signature;
  };
}

function createECDSAVerifer(bits) {
  const inner = createKeyVerifier(bits);
  return function verify(thing, signature, publicKey) {
    signature = formatEcdsa.joseToDer(signature, 'ES' + bits).toString('base64');
    const result = inner(thing, signature, publicKey);
    return result;
  };
}

[pgaubatz]

Hi,

I ran in exactly the same issue.
My (probably too quick-and-dirty) solution looks like this: pgaubatz/browserify-sign@0dd151d
I'm not quite sure if it makes sense to submit a PR...

@shea256 What do you think?

Cheers,
Patrick

[shea256]

@pgaubatz I don't think this actually fixes the problem.

I just ended up writing my own JWT library: https://github.com/blockstack/jwt-js

Maybe it'll be helpful for you. It only currently supports the curve SECP256k1, but it is designed in a way that anyone can easily write a client for ES256, RS256, etc.

[omsmith]

Yeah, so at the time the module was written, while openssl supported EC, there wasn't an algorithm available to call it with. So specifying RSA here gets it into openssl, and aftet it parses the PEM, it does the right thing.

I would be happy to accept a pull request which used the correct name (I looked when this issue first opened - it appears it might exist in openssl now) - perhaps deciding based off if the openssl or node.js version.

[omsmith]

@shea256 ES256 is secp256r1, not k1

[shea256]

@omsmith yes, I'm aware.

I mentioned that my library only currently supports SECP256k1 (which I abbreviate as ES256k) and while my library currently doesn't support the widely accepted ES256 and RS256 standards, those could easily be added in. I might actually add in an ES256 client soon, which will use SECP256r1 (according to the standards).

[samuelhorwitz]

There is an open PR on browserify-sign (what browserify uses to get node crypto to work) which addresses this. When that PR goes through, a change to this library to not masquerade as rsa and just use 'sha' + bits should work. Tried it out by monkeypatching locally.

[samuelhorwitz]

Alright they merged that PR over in Browserify. @omsmith what were the limitations of a PR over here? I have a patch I wrote that's very dumb that basically just switches from RSA-SHA to sha prefixing for all ECDSA and works with Browserify as soon as that PR lands in a release. Unfortunately, I don't know too much about how this works with regards to Node or other versioning caveats. All I know is that Browserifying with these fixes works.

[omsmith]

@samuelhorwitz feel free to submit your changes as a PR and we'll see what happens with the test suite (which will run it against older versions of node as well).