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

Ed25519/Ed448 does not throw when used with invalid digest in crypto.createSign #52097

Closed
xicilion opened this issue Mar 15, 2024 · 4 comments · Fixed by #52340
Closed

Ed25519/Ed448 does not throw when used with invalid digest in crypto.createSign #52097

xicilion opened this issue Mar 15, 2024 · 4 comments · Fixed by #52340
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@xicilion
Copy link
Contributor

What is the problem this feature will solve?

The following example:

const crypto = require('crypto');

const key = crypto.generateKeyPairSync("ed25519");

var signature = crypto.sign(null, "hello", key.privateKey);
console.log("crypto.sign:", signature.toString('hex'));

var signer = crypto.createSign('SHA256');
signer.update('hello');
var signature = signer.sign(key.privateKey);
console.log("signer.sign:", signature.toString('hex'));

will output the following:

crypto.sign: d58fdfa69f9d0bf4fd5358a6ed22031af3585ce9812b2bde5a5045a4e3cefd719a5af771af85e5a0c406fd48852574debe4deb32845785b761c59978b21fa903
signer.sign: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

When Signer is used for Ed25519 signing, Signer does not report any errors, but returns an all-zero Buffer as the signature result.
This can be confusing for programmers who think they have signed successfully, but in fact the returned signature is not available.

What is the feature you are proposing to solve the problem?

I think in this case Signer.sign should throw an error so that the programmer can deal with the problem early on.

What alternatives have you considered?

No response

@xicilion xicilion added the feature request Issues that request new features to be added to Node.js. label Mar 15, 2024
@panva
Copy link
Member

panva commented Mar 17, 2024

This seems like a bug. Ed25519 and Ed448 should only be available via the oneshot apis in the first place.

@xicilion
Copy link
Contributor Author

Yes, there is indeed an error here, so an error should be thrown here instead of returning a zero-filled Buffer.

@panva panva added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. and removed feature request Issues that request new features to be added to Node.js. labels Mar 19, 2024
@panva panva changed the title It is recommended to throw an error when passing Ed25519 private key to Signer. Ed25519/Ed448 does not throw when used with invalid digest in crypto.createSign Mar 19, 2024
@panva
Copy link
Member

panva commented Mar 19, 2024

cc @nodejs/crypto

@medic-code
Copy link

I would like to work on this if nobody else is.

panva added a commit to panva/node that referenced this issue Apr 3, 2024
panva added a commit to panva/node that referenced this issue Apr 3, 2024
panva added a commit to panva/node that referenced this issue Apr 4, 2024
nodejs-github-bot pushed a commit that referenced this issue Apr 8, 2024
fixes: #52097
PR-URL: #52340
Fixes: #52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
fixes: #52097
PR-URL: #52340
Fixes: #52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
fixes: #52097
PR-URL: #52340
Fixes: #52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants