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: fix CipherBase Update int32 overflow #45769

Merged
merged 2 commits into from
Dec 9, 2022
Merged

crypto: fix CipherBase Update int32 overflow #45769

merged 2 commits into from
Dec 9, 2022

Conversation

marco-ippolito
Copy link
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
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot 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 addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2022
@github-actions 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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, 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 pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 7, 2022
@panva
Copy link
Member

panva commented Dec 7, 2022

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 panva added author ready PRs that have at least one approval, no pending requests for changes, 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
Copy link
Member Author

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

@panva
Copy link
Member

panva commented Dec 7, 2022 •
edited
Loading

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

already done, this is good to go 👍

@github-actions 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
Copy link
Collaborator

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

ShogunPanda
ShogunPanda approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@nodejs-github-bot 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 nodejs-github-bot merged commit 5fe0795 into nodejs:main Dec 9, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 5fe0795

ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Dec 12, 2022
@marco-ippolito @ErickWendel
crypto: fix CipherBase Update int32 overflow
615c8aa 
PR-URL: nodejs#45769
Fixes: nodejs#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 pushed a commit that referenced this pull request Dec 12, 2022
@marco-ippolito @targos
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 targos mentioned this pull request Dec 12, 2022
Merged
targos pushed a commit that referenced this pull request Dec 13, 2022
@marco-ippolito @targos
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 pushed a commit that referenced this pull request Dec 30, 2022
@marco-ippolito @danielleadams
crypto: fix CipherBase Update int32 overflow
7740dc0 
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 danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@marco-ippolito @danielleadams
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
@marco-ippolito @danielleadams
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 4, 2023
@marco-ippolito @danielleadams
crypto: fix CipherBase Update int32 overflow
4f04474 
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
@marco-ippolito @danielleadams
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panva panva panva approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca 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. 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
@marco-ippolito @nodejs-github-bot @panva @ShogunPanda @bnoordhuis @addaleax @lpinca