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

fix: update P-521 bit length in Web Crypto #2710

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

twiss
Copy link

@twiss twiss commented Sep 20, 2024

ECDH over P-521 returns 66 bytes, i.e. 528 bits. Requesting 521 bits causes the last 7 bits of the derived value to be set to 0, which seems unintended (the Node.js version doesn't do this). So, request 66 * 8 bits instead.

Also, remove the superfluous namedCurve property in the algorithm identifier passed to deriveBits (it's only necessary when importing or generating keys).

ECDH over P-521 returns 66 bytes, i.e. 528 bits. Requesting 521 bits
entails a truncation of the derived value, which seems unintended.
@twiss twiss requested a review from a team as a code owner September 20, 2024 00:56
@achingbrain achingbrain changed the title Fix P-521 bit length in Web Crypto fix: update P-521 bit length in Web Crypto Sep 20, 2024
@achingbrain
Copy link
Member

Thanks for opening this PR. Is there a test you could add that shows how it's currently broken in main? The tests are run in node, chrome, firefox, etc so this will help prevent regressions in future.

@twiss
Copy link
Author

twiss commented Sep 20, 2024

Hmhm, done :)

Btw, the comment in the tests said 'P-521' fails in tests :( no clue why, ironically enough, but they actually passed for me even without this patch. So I've expanded the tests a bit to compare the derived key with a fixture generated in Go as well. That test fails without this patch (in the browser) and succeeds with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants