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.

Sign up for GitHub

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

Jump to bottom

crypto: multiple webcrypto fixes #43431

Closed
wants to merge 5 commits into from
Closed

crypto: multiple webcrypto fixes #43431

wants to merge 5 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Jun 14, 2022 •
edited
Loading

While working on updating WPTs (#43421 put on hold) I have found the following issues.

  • digest() invalid algorithm which happens to collide with other normalizeHashName names was accepted
  • empty usages in generateKey() were accepted
  • generateKey() publicExponent was not used
  • deriveBits() ECDH / X25519 / X448 returned incorrect slice when non multiple of 8 length was requested
  • generateKey() AES-KW accepted invalid usages

Review individual commits

@panva panva added crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. webcrypto commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jun 14, 2022
@panva panva requested review from jasnell and tniessen June 14, 2022 19:33
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 14, 2022
@panva panva mentioned this pull request Jun 14, 2022
Closed
tniessen
tniessen approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

lib/internal/crypto/aes.js Show resolved Hide resolved
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Open
17 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
aduh95
aduh95 approved these changes Jun 15, 2022
View reviewed changes
test/parallel/test-webcrypto-digest.js
Comment on lines +172 to +176
(async () => {
await assert.rejects(subtle.digest('RSA-OAEP', Buffer.alloc(1)), {
name: 'NotSupportedError',
});
})().then(common.mustCall());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the async wrapper is technically not needed (although it's fine to keep it if you prefer as is)

Suggested change
(async () => {
await assert.rejects(subtle.digest('RSA-OAEP', Buffer.alloc(1)), {
name: 'NotSupportedError',
});
})().then(common.mustCall());
assert.rejects(subtle.digest('RSA-OAEP', Buffer.alloc(1)), {
name: 'NotSupportedError',
}).then(common.mustCall());

panva added a commit that referenced this pull request Jun 16, 2022
@panva
crypto: fix webcrypto RSA generateKey() use of publicExponent
be03c09 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
panva added a commit that referenced this pull request Jun 16, 2022
@panva
crypto: fix webcrypto digest() invalid algorithm
288304c 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
panva added a commit that referenced this pull request Jun 16, 2022
@panva
crypto: fix webcrypto generateKey() with empty usages
677bd66 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This was referenced Jun 17, 2022
Open
Open
@F3n67u F3n67u mentioned this pull request Jun 18, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jun 19, 2022
Open
30 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: fix webcrypto deriveBits for non-byte lengths
3ee0bb8 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: fix webcrypto AES-KW keys accepting encrypt/decrypt usages
018f61c 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: fix webcrypto RSA generateKey() use of publicExponent
dedb22e 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: fix webcrypto digest() invalid algorithm
64a9dd7 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@panva @targos
crypto: fix webcrypto generateKey() with empty usages
679f191 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 18, 2022
@panva @targos
crypto: fix webcrypto deriveBits for non-byte lengths
968b7d5 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 18, 2022
@panva @targos
crypto: fix webcrypto AES-KW keys accepting encrypt/decrypt usages
f7f940d 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 18, 2022
@panva @targos
crypto: fix webcrypto RSA generateKey() use of publicExponent
695591e 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 18, 2022
@panva @targos
crypto: fix webcrypto digest() invalid algorithm
8f687d7 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 18, 2022
@panva @targos
crypto: fix webcrypto generateKey() with empty usages
821d419 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: fix webcrypto deriveBits for non-byte lengths
52ea869 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: fix webcrypto AES-KW keys accepting encrypt/decrypt usages
04572ec 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: fix webcrypto RSA generateKey() use of publicExponent
077ad7e 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: fix webcrypto digest() invalid algorithm
bef600e 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@panva @targos
crypto: fix webcrypto generateKey() with empty usages
8dca8d7 
PR-URL: #43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: fix webcrypto deriveBits for non-byte lengths
eb72559 
PR-URL: nodejs/node#43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: fix webcrypto AES-KW keys accepting encrypt/decrypt usages
0a78d7c 
PR-URL: nodejs/node#43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: fix webcrypto RSA generateKey() use of publicExponent
0172b8c 
PR-URL: nodejs/node#43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: fix webcrypto digest() invalid algorithm
72c725b 
PR-URL: nodejs/node#43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@panva @guangwong
crypto: fix webcrypto generateKey() with empty usages
db70e83 
PR-URL: nodejs/node#43431
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@panva panva deleted the webcrypto-fixes branch October 13, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@tniessen tniessen tniessen approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@panva @nodejs-github-bot @jasnell @tniessen @aduh95