Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: signing and verifying not supported for ed25519/ed448 #26320

Closed
mscdex opened this issue Feb 26, 2019 · 7 comments
Closed

crypto: signing and verifying not supported for ed25519/ed448 #26320

mscdex opened this issue Feb 26, 2019 · 7 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2019

  • Version: master
  • Platform: n/a
  • Subsystem: crypto

From https://www.openssl.org/docs/manmaster/man7/Ed25519.html:

The PureEdDSA algorithm does not support the streaming mechanism of other signature algorithms using, for example, EVP_DigestUpdate(). The message to sign or verify must be passed using the one-shot EVP_DigestSign() and EVP_DigestVerify() functions.

When calling EVP_DigestSignInit() or EVP_DigestVerifyInit(), the digest type parameter MUST be set to NULL.

I'm not sure how we want to implement support for this in node, perhaps with special algorithm names for crypto.createSign()/crypto.createVerify() that only permit a single call to .update() or that buffer all data passed to each .update() for the one-shot sign/verify at the end?

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Feb 26, 2019
@bnoordhuis
Copy link
Member

that only permit a single call to .update()

That would be my preference. Buffering is a performance pitfall and DoS vector.

@tniessen
Copy link
Member

tniessen commented Mar 9, 2019

I agree with @bnoordhuis. Also, we have a similar behavior for Cipher in CCM mode which does not support streaming either.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 11, 2019

Well there will still have to be some kind of buffering (the single chunk) if we're going to allow some compatibility with the existing API.

There's also the problem of what users are expected to use for the algorithm name with createSign()/createVerify()? Do they specify something like 'ed25519', despite not being a valid hash name and then also have to verify that if the algorithm was 'ed25519' that the user supplies an 'ed25519' key to match?

We could avoid all of this if we added a separate one-shot sign/verify API, but I'm not sure what that would look like or if people would support something like that.

@tniessen
Copy link
Member

tniessen commented Mar 11, 2019

That's a good question...

The PureEdDSA algorithm does not support the streaming mechanism of other signature algorithms using, for example, EVP_DigestUpdate(). The message to sign or verify must be passed using the one-shot EVP_DigestSign() and EVP_DigestVerify() functions.

When calling EVP_DigestSignInit() or EVP_DigestVerifyInit(), the digest type parameter MUST be set to NULL.

You are right, this makes compatibility with our existing APIs difficult.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 11, 2019

/cc @nodejs/collaborators ideas? thoughts?

@sam-github
Copy link
Contributor

I would support a one-shot API. Usually, crypto APIs have a one-shot, and a multi-shot API.

I'm also fine if its possible to implement with the restriction of single-call-to-update. That sounds like it might have to involve some unique to ed shenanigans, and would need to allow the 'digest' to be the 'signature algorithm' (like 'pureeddsa-sha512').

How widely used is PureEdDSA? Requiring two passes is pretty unusual.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 11, 2019

How widely used is PureEdDSA

It's used by ed25519 (and ed448 for that matter), which is used for modern SSH keys for example.

mscdex added a commit to mscdex/io.js that referenced this issue Mar 29, 2019
These methods are added primarily to allow signing and verifying
using Ed25519 and Ed448 keys, which do not support streaming of
input data. However, any key type can be used with these new
APIs, to allow better performance when only signing/verifying
a single chunk.

Fixes: nodejs#26320
PR-URL: nodejs#26611
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
These methods are added primarily to allow signing and verifying
using Ed25519 and Ed448 keys, which do not support streaming of
input data. However, any key type can be used with these new
APIs, to allow better performance when only signing/verifying
a single chunk.

Fixes: #26320
PR-URL: #26611
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants