Skip to content

DiffieHellman/ECDH Generates invalid keypairs #14628

Closed
Closed
DiffieHellman/ECDH Generates invalid keypairs#14628
Labels
cryptoIssues and PRs related to the crypto subsystem.feature requestIssues requesting new Node.js features.

Description

@mikegwhit
mikegwhit
opened on Aug 4, 2017
  • Version: 6.11.1
  • Platform: Windows 10
  • Subsystem:

Error is thrown:
error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long

User Experience Log:
A user should not have to manually wrap the generated keys in the proper "PEM" format since PEM is very precious about spaces. Without wanting to be pejorative, that sucks. The fact that this code example still doesn't work after the fact also sucks.

Suggestion:
Remove/deprecate this API or fix it or provide clearer examples to perform this common functionality.

Workaround:
Utilize openssl executable as a child proc to generate keys instead.

/** 
 * @fileoverview Everything below this comment is from the documentation example.
 */
const crypto = require('crypto');
const assert = require('assert');

// Generate Alice's keys...
const alice = crypto.createDiffieHellman(2048);
const aliceKey = alice.generateKeys();

// Generate Bob's keys...
const bob = crypto.createDiffieHellman(alice.getPrime(), alice.getGenerator());
const bobKey = bob.generateKeys();

// Exchange and generate the secret...
const aliceSecret = alice.computeSecret(bobKey);
const bobSecret = bob.computeSecret(aliceKey);

// OK
assert.strictEqual(aliceSecret.toString('hex'), bobSecret.toString('hex'));

/**
 *  Everything below this comment is in addition to the docs example!!!
 * @omg
 */
// Attempts improvement...
const publicKey = '-----BEGIN PUBLIC KEY-----\n' +
    aliceKey.toString('base64').match(/.{1,64}/g).join('\n') +
    '\n-----END PUBLIC KEY-----\n';
const privateKey = '-----BEGIN PRIVATE KEY-----\n' +
    aliceSecret.toString('base64').match(/.{1,64}/g).join('\n') +
    '\n-----END PRIVATE KEY-----\n';
console.log(crypto.privateDecrypt(crypto.publicEncrypt(publicKey, new Buffer('racecar')), privateKey)); // throws!

Activity

  1. addaleax
    added
    cryptoIssues and PRs related to the crypto subsystem.
    on Aug 4, 2017

  2. addaleax commented on Aug 4, 2017

    @addaleax
    addaleax
    on Aug 4, 2017
    Member

    /cc @nodejs/crypto

    (also edited the OP to add syntax highlighting)

  3. mscdex
    added
    feature requestIssues requesting new Node.js features.
    on Aug 4, 2017

  4. shigeki commented on Aug 4, 2017

    @shigeki
    shigeki
    on Aug 4, 2017
    Contributor

    Let me confirm that in your sample code you created a DH public and private key pair and tried to use them in publicEncrypt/publicDecrypt by formatting the DH key pair into PEM. It that right?

    publicEncrypt/publicDecrypt only support RSA not ElGamal. Is that the reason why the error was occurred?

  5. bnoordhuis commented on Aug 4, 2017

    @bnoordhuis
    bnoordhuis
    on Aug 4, 2017
    Member

    This seems like a wrong expectation to me. The documentation is clear that publicEncrypt() and friends use RSA, nowhere does it mention ElGamal; OpenSSL supports only the former, not the latter.

    I'll close this out.

  6. bnoordhuis
    closed this as completedon Aug 4, 2017
  7. bnoordhuis
    added
    invalidIssues and PRs that are invalid.
    and removed
    feature requestIssues requesting new Node.js features.
    on Aug 4, 2017

  8. mscdex commented on Aug 4, 2017

    @mscdex
    mscdex
    on Aug 4, 2017
    Contributor

    FWIW I have to do the same thing in my ssh2-streams module before passing a key to verifier.verify(). It's annoying that non-PEM keys are not allowed, but at least the code to wrap it in a PEM format isn't too horrible...

    I think that is what the OP is requesting, to be able to pass in an RSA, etc. key without the PEM formatting (just binary). That is a valid feature request to me IMHO.

  9. mscdex
    added
    feature requestIssues requesting new Node.js features.
    and removed
    invalidIssues and PRs that are invalid.
    on Aug 4, 2017
  10. mscdex
    reopened this on Aug 4, 2017

  11. bnoordhuis commented on Aug 4, 2017

    @bnoordhuis
    bnoordhuis
    on Aug 4, 2017
    Member

    @mgwhitfield Is ^ what you are requesting?

  12. bnoordhuis commented on Aug 8, 2017

    @bnoordhuis
    bnoordhuis
    on Aug 8, 2017
    Member

    I'm going to close this out again since OP didn't follow up.

    @mscdex If you think this is a worthwhile feature, can you open a new issue with a better description?

  13. bnoordhuis
    closed this as completedon Aug 8, 2017

  14. shigeki commented on Aug 9, 2017

    @shigeki
    shigeki
    on Aug 9, 2017
    Contributor

    I think that is what the OP is requesting, to be able to pass in an RSA, etc. key without the PEM formatting (just binary). That is a valid feature request to me IMHO.

    Currently, verifyer.verify() supports three types of pem formats of an RSA public key as pcks#1, pcks#8 and x509 cert with looking at each pem headers. If we have an additional support of der(binary) format, I guess we need to add an new option to specify such types. So I think the current supporting only pem format is better.

  15. sam-github commented on Aug 10, 2017

    @sam-github
    sam-github
    on Aug 10, 2017
    Contributor

    Though the heuristics to detect DER are pretty robust, IIRC its 2 fixed bytes at the beginning for a constructed sequence, then a couple bytes for a length that has to match the buffer length, so hard to confuse with PEM.

  16. bnoordhuis commented on Aug 10, 2017

    @bnoordhuis
    bnoordhuis
    on Aug 10, 2017
    Member

    Yeah, but a DER-encoded value can be one of several things: a public or private RSA, DSA or EC key (maybe more) in PKCS#1, PKCS#8 or PKCS#12 format (again, maybe more.)

    I know my crypto standards pretty well and I still wouldn't want to vouch you can parse them unambiguously.

  17. sam-github commented on Aug 10, 2017

    @sam-github
    sam-github
    on Aug 10, 2017
    Contributor

    Yes, if its not just PEM vs DER, its more than a quick check. I implemented all three of those PKCS standards for all three of those key types and I'm pretty sure my CLI detected them just fine, but I had a BER decoder API so I could peek into the structure for OIDs and element counts, it might be hard without that. I've added it to my (exceedingly deep) list of things to try, running code being the most convincing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    cryptoIssues and PRs related to the crypto subsystem.feature requestIssues requesting new Node.js features.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions