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 d3-color and potential security issue #2454

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Sep 29, 2022

Description

Addresses potential ReDoS issue from d3-color version < 3.1.0

d3-color is used as a sub-dependency of elastic/chart package. since there is no function change form 1.4 to 3.1.0, we could add resolution in package.json to overwrite the version.

yarn why d3-color
yarn why v1.22.19
[1/4] Why do we have the module "d3-color"...?
[2/4] Initialising dependency graph...
warning Resolution field "[email protected]" is incompatible with requested version "typescript@~4.5.2"
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "[email protected]"
info Has been hoisted to "d3-color"
info Reasons this module exists
   - "workspace-aggregator-26115f01-dd15-42ff-8fa9-cd8d075aae02" depends on it
   - Hoisted from "_project_#@elastic#charts#d3-color"
   - Hoisted from "_project_#@elastic#charts#d3-interpolate#d3-color"
   - Hoisted from "_project_#@elastic#charts#d3-scale#d3-color"
   - Hoisted from "_project_#@elastic#charts#d3-scale#d3-interpolate#d3-color"

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@ananzh ananzh requested a review from a team as a code owner September 29, 2022 17:48
AMoo-Miki
AMoo-Miki previously approved these changes Sep 29, 2022
kaddy645
kaddy645 previously approved these changes Sep 29, 2022
@ananzh ananzh self-assigned this Sep 29, 2022
@ananzh ananzh added backport 2.x backport 1.x v2.4.0 'Issues and PRs related to version v2.4.0' backport 1.3 labels Sep 29, 2022
@kavilla
Copy link
Member

kavilla commented Sep 29, 2022

Looks like it's require snapshot update?

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
@ananzh
Copy link
Member Author

ananzh commented Sep 29, 2022

Looks like it's require snapshot update?

Not just snapshot. but see a lot of

Jest SyntaxError: Unexpected token 'export' 

due to d3-color pre-compiled source code. fix that. Then see locally some failing tests are from src/plugins/data_source. since this is a security issue and will backport to 1.x. I think we should have another issue or PR to fix the following unit tests. what do you think? @kavilla

Summary of all failing tests
 FAIL  src/plugins/data_source/server/cryptography/cryptography_client.test.ts
  ● Test encrpyt and decrypt module › Positive test cases › Encrypt and Decrypt with same in memory keyring

    Unsupported plaintext

      55 |    */
      56 |   public async encryptAndEncode(plainText: string): Promise<string> {
    > 57 |     const result = await this.encrypt(this.keyring, plainText);
         |                               ^
      58 |     return result.result.toString(ENCODING_STRATEGY);
      59 |   }
      60 |

      at Object.encrypt (node_modules/@aws-crypto/encrypt-node/src/encrypt.ts:54:11)
      at CryptographyClient.encryptAndEncode (src/plugins/data_source/server/cryptography/cryptography_client.ts:57:31)
      at Object.<anonymous> (src/plugins/data_source/server/cryptography/cryptography_client.test.ts:32:50)

  ● Test encrpyt and decrypt module › Positive test cases › Encrypt and Decrypt with two different keyrings with exact same identifiers

    Unsupported plaintext

      55 |    */
      56 |   public async encryptAndEncode(plainText: string): Promise<string> {
    > 57 |     const result = await this.encrypt(this.keyring, plainText);
         |                               ^
      58 |     return result.result.toString(ENCODING_STRATEGY);
      59 |   }
      60 |

      at Object.encrypt (node_modules/@aws-crypto/encrypt-node/src/encrypt.ts:54:11)
      at CryptographyClient.encryptAndEncode (src/plugins/data_source/server/cryptography/cryptography_client.ts:57:31)
      at Object.<anonymous> (src/plugins/data_source/server/cryptography/cryptography_client.test.ts:42:51)

  ● Test encrpyt and decrypt module › Negative test cases › Encrypt and Decrypt with different key names

    Unsupported plaintext

      55 |    */
      56 |   public async encryptAndEncode(plainText: string): Promise<string> {
    > 57 |     const result = await this.encrypt(this.keyring, plainText);
         |                               ^
      58 |     return result.result.toString(ENCODING_STRATEGY);
      59 |   }
      60 |

      at Object.encrypt (node_modules/@aws-crypto/encrypt-node/src/encrypt.ts:54:11)
      at CryptographyClient.encryptAndEncode (src/plugins/data_source/server/cryptography/cryptography_client.ts:57:31)
      at Object.<anonymous> (src/plugins/data_source/server/cryptography/cryptography_client.test.ts:65:51)

  ● Test encrpyt and decrypt module › Negative test cases › Encrypt and Decrypt with different key namespaces

    Unsupported plaintext

      55 |    */
      56 |   public async encryptAndEncode(plainText: string): Promise<string> {
    > 57 |     const result = await this.encrypt(this.keyring, plainText);
         |                               ^
      58 |     return result.result.toString(ENCODING_STRATEGY);
      59 |   }
      60 |

      at Object.encrypt (node_modules/@aws-crypto/encrypt-node/src/encrypt.ts:54:11)
      at CryptographyClient.encryptAndEncode (src/plugins/data_source/server/cryptography/cryptography_client.ts:57:31)
      at Object.<anonymous> (src/plugins/data_source/server/cryptography/cryptography_client.test.ts:84:51)

  ● Test encrpyt and decrypt module › Negative test cases › Encrypt and Decrypt with different wrapping keys

    Unsupported plaintext

      55 |    */
      56 |   public async encryptAndEncode(plainText: string): Promise<string> {
    > 57 |     const result = await this.encrypt(this.keyring, plainText);
         |                               ^
      58 |     return result.result.toString(ENCODING_STRATEGY);
      59 |   }
      60 |

      at Object.encrypt (node_modules/@aws-crypto/encrypt-node/src/encrypt.ts:54:11)
      at CryptographyClient.encryptAndEncode (src/plugins/data_source/server/cryptography/cryptography_client.ts:57:31)
      at Object.<anonymous> (src/plugins/data_source/server/cryptography/cryptography_client.test.ts:103:51)


Test Suites: 1 failed, 1 skipped, 1495 passed, 1496 of 1497 total
Tests:       5 failed, 24 skipped, 9 todo, 11329 passed, 11367 total
Snapshots:   2504 passed, 2504 total
Time:        136.72 s

@codecov-commenter
Copy link

Codecov Report

Merging #2454 (a0f54f9) into main (a528965) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2454   +/-   ##
=======================================
  Coverage   66.75%   66.75%           
=======================================
  Files        3200     3200           
  Lines       60886    60886           
  Branches     9250     9250           
=======================================
+ Hits        40644    40646    +2     
+ Misses      18030    18028    -2     
  Partials     2212     2212           
Impacted Files Coverage Δ
...ared/static/forms/hook_form_lib/hooks/use_field.ts 66.66% <0.00%> (+0.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ananzh
Copy link
Member Author

ananzh commented Sep 29, 2022

I only see the above 5 failed unit tests locally
CI doesn't catch this.

@seraphjiang
Copy link
Member

seraphjiang commented Sep 29, 2022

@noCharger i saw some src/plugins/data_source UT failed, could you take a look

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

looks good, just a couple small questions.

src/dev/jest/config.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
ashwin-pc
ashwin-pc previously approved these changes Sep 29, 2022
@ananzh
Copy link
Member Author

ananzh commented Sep 29, 2022

add a tracking issue for the failed local unit tests:
#2459

joshuarrrr
joshuarrrr previously approved these changes Sep 29, 2022
@ananzh ananzh dismissed stale reviews from joshuarrrr and ashwin-pc via 41fb469 September 29, 2022 23:13
@noCharger
Copy link
Contributor

@noCharger i saw some src/plugins/data_source UT failed, could you take a look

will do

@ananzh ananzh merged commit d802a3e into opensearch-project:main Sep 30, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-2454-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d802a3ef5b3c75418e2cf4d5f480e48274842dfd
# Push it to GitHub
git push --set-upstream origin backport/backport-2454-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-2454-to-1.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-2454-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d802a3ef5b3c75418e2cf4d5f480e48274842dfd
# Push it to GitHub
git push --set-upstream origin backport/backport-2454-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-2454-to-1.3.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 30, 2022
* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
(cherry picked from commit d802a3e)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 30, 2022
* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
(cherry picked from commit d802a3e)
ananzh added a commit that referenced this pull request Sep 30, 2022
* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
(cherry picked from commit d802a3e)

Co-authored-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Sep 30, 2022
…ecurity issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

backport PR:
opensearch-project#2454

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Sep 30, 2022
* Addresses potential ReDoS issue from d3-color version < 3.1.0

backport PR:
opensearch-project#2454

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit that referenced this pull request Sep 30, 2022
* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
(cherry picked from commit d802a3e)

Co-authored-by: Anan Zhuang <[email protected]>
@ananzh ananzh removed the v1.3.6 label Oct 23, 2022
@AMoo-Miki AMoo-Miki added the cve Security vulnerabilities detected by Dependabot or Mend label Nov 5, 2022
@joshuarrrr
Copy link
Member

not backportable to any 1.x due to node version conflicts

pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
…pensearch-project#2466)

* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
(cherry picked from commit d802a3e)

Co-authored-by: Anan Zhuang <[email protected]>
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
* Resolve sub-dependent d3-color version and potencial security issue

* Addresses potential ReDoS issue from d3-color version < 3.1.0

Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x cve Security vulnerabilities detected by Dependabot or Mend v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.