Skip to content

Conversation

@panva
Copy link
Member

@panvapanva commented Jun 14, 2022

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

@panvapanva 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
@panvapanva requested review from jasnell and tniessenJune 14, 2022 19:33
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-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
@panvapanva mentioned this pull request Jun 14, 2022
Copy link
Member

@tniessentniessen 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!

@panvapanva 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-actionsgithub-actionsbot 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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Comment on lines +172 to +176
(async()=>{
awaitassert.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()=>{
awaitassert.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
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
panva added a commit that referenced this pull request Jun 16, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
panva added a commit that referenced this pull request Jun 16, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@targostargos mentioned this pull request Jul 12, 2022
targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43431 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@panvapanva deleted the webcrypto-fixes branch October 13, 2022 09:12
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.cryptoIssues and PRs related to the crypto subsystem.experimentalIssues and PRs related to experimental features.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.webcrypto

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panva@nodejs-github-bot@jasnell@tniessen@aduh95