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(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039 #2321

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

ruzell22
Copy link
Contributor

fixes: #2039

related to: #2241

@ruzell22
Copy link
Contributor Author

Hello @petermetz , the listed vulnerabilities in the ticket are now fixed in cactus-cmd-api-server. New vulnerabilities were detected after. The 4 new vulnerabilities are the same ones in #2241 after the committed change. The same changes were applied on this PR as well.

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 This looks good in general, but please apply this changes:

  1. Clarify in the commit message and the commit subject the CVE being fixed so that it is unique. vulnerabilities will always be found in all packages as we are approaching infinity and the current commit subject will be duplicated unless you mention the CVE ID.
  2. upgrade express in lockstep across all the packages. I know that this will make the PR much larger and I'm usually advocating for smaller PRs, but in this case it is necessary because sometimes when packages have different versions of express the compilation across packages that depend on each other can have hard to trace issues because of it.

    The other reason to do it this way is because if you are fixing a vulnerability then it's not really fixed until all the packages have had their dependencies upgraded and therefore there is no occurrence of the vulnerability in the entire repo.

    You can get a complete list of express dependencies like this:
$ grep -ri --exclude-dir=node_modules --exclude-dir=.tmp --include=\*.json '"express": "' ./
./weaver/samples/fabric/fabric-cli/package.json:    "express": "^4.18.1",
./weaver/samples/fabric/fabric-cli/package-local.json:    "express": "^4.18.1",
./examples/cactus-example-supply-chain-backend/package.json:    "express": "4.17.1",
./examples/cactus-example-supply-chain-business-logic-plugin/package.json:    "express": "4.17.1",
./examples/cactus-example-carbon-accounting-business-logic-plugin/package.json:    "express": "4.17.1",
./examples/cactus-example-discounted-asset-trade/package.json:    "express": "4.16.4",
./examples/cactus-example-electricity-trade/package.json:    "express": "4.16.4",
./examples/test-run-transaction/package.json:    "express": "4.16.4",
./examples/test-run-transaction/supply-chain-app-stub/package.json:    "express": "4.17.3",
./packages/cactus-plugin-keychain-google-sm/package.json:    "express": "4.17.1",
./packages/cactus-core/package.json:    "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-quorum/package.json:    "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-fabric-socketio/package.json:    "express": "4.17.3",
./packages/cactus-plugin-ledger-connector-fabric/package.json:    "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-iroha/package.json:    "express": "4.17.1",
./packages/cactus-plugin-htlc-eth-besu/package.json:    "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-besu/package.json:    "express": "4.17.1",
./packages/cactus-plugin-keychain-azure-kv/package.json:    "express": "4.17.1",
./packages/cactus-test-plugin-htlc-eth-besu-erc20/package.json:    "express": "4.17.1"
./packages/cactus-plugin-keychain-memory-wasm/package.json:    "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-xdai/package.json:    "express": "4.17.1",
./packages/cactus-cmd-socketio-server/package.json:    "express": "4.16.4",
./packages/cactus-plugin-htlc-eth-besu-erc20/package.json:        "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-go-ethereum-socketio/package.json:    "express": "4.17.3",
./packages/cactus-plugin-ledger-connector-sawtooth-socketio/package.json:    "express": "4.17.3",
./packages/cactus-cmd-api-server/package.json:    "express": "4.17.1",
./packages/cactus-plugin-keychain-memory/package.json:    "express": "4.17.1",
./packages/cactus-plugin-consortium-manual/package.json:    "express": "4.17.1",
./extensions/cactus-plugin-object-store-ipfs/package.json:    "express": "4.17.1",

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 Sorry, never mind my second point from above. I just noticed that you have PRs open already for the other upgrades so it's fine to merge those separately. Just refactor the PR title and the commit subject line/message and we are good to go.

@ruzell22 ruzell22 changed the title fix(security): vulnerabilities found in cactus-cmd-api-server #2039 fix(security): vulnerabilities found in cactus-cmd-api-server #2039 - fix CVE-2022-24434 and CVE-2022-24999 Mar 28, 2023
@ruzell22
Copy link
Contributor Author

Hello @petermetz , PR title and commit message are now changed with the additional CVE IDs. Thank you.
image

@ruzell22 ruzell22 requested review from petermetz and removed request for VRamakrishna, sandeepnRES and izuru0 March 28, 2023 05:32
Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 Thank you for the update. Now you just need to make sure that the commit lint is passing as well.

The linter is complaining because you added the extra information without refactoring the existing commit message so now it's too long.
I suggest something like this which ultimately has the same amount of information but it does fit into the 72 characters limit (which is annoying when you write commit messages but super helpful when you are scanning through a large amount of commits searching for something)

Suggested commit subject:

fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039

@ruzell22 ruzell22 changed the title fix(security): vulnerabilities found in cactus-cmd-api-server #2039 - fix CVE-2022-24434 and CVE-2022-24999 fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039 Mar 29, 2023
@ruzell22 ruzell22 changed the title fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039 fix(security): vulnerabilities found in cactus-cmd-api-server #2039 - fix CVE-2022-24434 and CVE-2022-24999 Mar 29, 2023
@ruzell22 ruzell22 changed the title fix(security): vulnerabilities found in cactus-cmd-api-server #2039 - fix CVE-2022-24434 and CVE-2022-24999 fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039 Mar 29, 2023
@ruzell22
Copy link
Contributor Author

Hi @petermetz , successfully changed the commit subject and PR title as per the suggested change. Commit lint is also passing now. Thank you

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 Great, thank you very much! LGTM

…ledger#2039

fixes: hyperledger#2039

related to: hyperledger#2241

Verified that these changes will fix the vulnerabilities in
cactus-cmd-api-server in addition to the following CVE IDs:
- CVE-2022-24434
- CVE-2022-24999 (express)
- CVE-2022-24999 (qs)

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: ruzell22 <ruzell.vince.aquino@accenture.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz merged commit 1cc9667 into hyperledger:main Apr 3, 2023
@petermetz petermetz deleted the cmdapiserver2039 branch April 3, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(security): vulnerabilities found in cactus-cmd-api-server
4 participants