FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix CipherBase Update int32 overflow by marco-ippolito · Pull Request #45769 · nodejs/node · GitHub

/ node Public

crypto: fix CipherBase Update int32 overflow - #45769

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
marco-ippolito:fix/cipher-update-int-size
Dec 9, 2022
ConversationCommits (2)ChecksFiles changed
Merged

crypto: fix CipherBase Update int32 overflow#45769
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
marco-ippolito:fix/cipher-update-int-size

Conversation

marco-ippolito commented Dec 7, 2022

Copy link
Copy Markdown
Member

resolves: #45757
I've followed @bnoordhuis solution and added a test
Now will throw the default error:
ThrowCryptoError(env, ERR_get_error(), "Trying to add data in unsupported state");

nodejs-github-bot commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Dec 7, 2022
addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view
panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 7, 2022

panva commented Dec 7, 2022

Copy link
Copy Markdown
Member

There are builds that do not end up with the correct error that need to be handled before this can land.

node:assert:636
      throw err;
      ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   name: 'RangeError'
-   name: 'Error'
  }
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/test/parallel/test-crypto-cipheriv-decipheriv.js:221:10)
    at Module._compile (node:internal/modules/cjs/loader:1218:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
    at Module.load (node:internal/modules/cjs/loader:1081:32)
    at Module._load (node:internal/modules/cjs/loader:922:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:82:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: RangeError [ERR_INVALID_ARG_VALUE]: The argument 'size' is invalid. Received 2147483647
      at Function.allocUnsafeSlow (node:buffer:393:3)
      at assert.throws.name (/home/iojs/build/workspace/node-test-commit-arm/test/parallel/test-crypto-cipheriv-decipheriv.js:222:18)
      at getActual (node:assert:757:5)
      at Function.throws (node:assert:903:24)
      at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/test/parallel/test-crypto-cipheriv-decipheriv.js:221:10)
      at Module._compile (node:internal/modules/cjs/loader:1218:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
      at Module.load (node:internal/modules/cjs/loader:1081:32)
      at Module._load (node:internal/modules/cjs/loader:922:12)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:82:12) {
    code: 'ERR_INVALID_ARG_VALUE'
  },
  expected: { name: 'Error' },
  operator: 'throws'
}

panva reviewed Dec 7, 2022
View reviewed changes
Comment thread test/parallel/test-crypto-cipheriv-decipheriv.js Outdated
panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 7, 2022

marco-ippolito commented Dec 7, 2022

Copy link
Copy Markdown
Member Author

probably on some builds it exceeds the kMaxLength I'll look into it

panva commented Dec 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

probably on some builds it exceeds the kMaxLength I'll look into it

already done, this is good to go 👍

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022

nodejs-github-bot commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48324/

bnoordhuis approved these changes Dec 7, 2022
View reviewed changes
Comment thread test/parallel/test-crypto-cipheriv-decipheriv.js
ShogunPanda approved these changes Dec 9, 2022
View reviewed changes

ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 9, 2022
nodejs-github-bot merged commit 5fe0795 into nodejs:main Dec 9, 2022

nodejs-github-bot commented Dec 9, 2022

Copy link
Copy Markdown
Collaborator

Landed in 5fe0795

targos pushed a commit that referenced this pull request Dec 12, 2022
crypto: fix CipherBase Update int32 overflow
556b270 
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos mentioned this pull request Dec 12, 2022
Merged
targos pushed a commit that referenced this pull request Dec 13, 2022
crypto: fix CipherBase Update int32 overflow
2e4d37e 
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
crypto: fix CipherBase Update int32 overflow
64f36d0 
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
crypto: fix CipherBase Update int32 overflow
2fbf956 
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
crypto: fix CipherBase Update int32 overflow
ae6f377 
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

panva panva approved these changes
ShogunPanda ShogunPanda approved these changes
bnoordhuis bnoordhuis approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

crypto: cipher update process crash with input of max int32 length

7 participants

Back | FazBrowse Home | New Git URL