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

Address security vulnerabilities #2912

Merged
merged 20 commits into from
Jul 25, 2023
Merged

Address security vulnerabilities #2912

merged 20 commits into from
Jul 25, 2023

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Jul 21, 2023 •
edited
Loading

Addressing 12 vulnerabilities re issue: #2903

  1. Devp2p
  • Removed dependency: @types/chalk@2.2.0:
  • Update import of multiaddr/convert to remove Require
  1. Client:
  • Update peer-id dependency
  • Update multiaddr dependency to @multiformats/multiaddr
  1. Trie
  • Remove 0x dependency

ALL REMAINING VULNERABILITIES
caused by:

multiaddr v10.0.01 is deprecated
to be replaced by @multiformats/multiaddr

currently unable to update due to ESM conflicts.

@codecov
Copy link

codecov bot commented Jul 21, 2023 •
edited
Loading

Codecov Report

Merging #2912 (35228cf) into master (84c57cb) will increase coverage by 2.99%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client 88.34% <100.00%> (+0.04%) ⬆️
common ?
ethash ?
evm ?
statemanager ?
trie ?
tx 95.96% <ø> (ø)
util ?
vm ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@ScottyPoi
Copy link
Contributor Author

Halfway there!

Screenshot from 2023-07-21 06-05-43

@ScottyPoi
Copy link
Contributor Author

Screenshot from 2023-07-21 07-18-00

holgerd77
holgerd77 reviewed Jul 21, 2023
View reviewed changes
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks already pretty good, thanks for tackling! 🤩

(1-2 CI runs failing, just to mention)

packages/client/package.json
"peer-id": "^0.14.3",
"@multiformats/multiaddr": "^12.1.3",
"@libp2p/interface-peer-id": "2.0.2",
"@libp2p/peer-id-factory": "2.0.4",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe @acolytec3 can confirm, but we removed libp2p completely from the core build, so I would think these two dependencies shouldn't show up here at all? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on how we're using peer-id. That library got radically changed a while back so you might need that factory for even our limited usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly found in client/libp2pBrowserBuild

packages/devp2p/src/dns/enr.ts
import { sscanf } from 'scanf'

import { keccak256, toNewUint8Array } from '../util.js'

import type { PeerInfo } from '../types.js'

const Convert = require('multiaddr/src/convert')
Copy link
Member

Choose a reason for hiding this comment

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

Ah, another require gone, great! 😃

packages/trie/package.json
@@ -61,7 +61,6 @@
},
"devDependencies": {
"@ethereumjs/genesis": "0.1.0-rc.1",
"0x": "^4.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

I am personally ok with dropping the 0x dependency, this was added for convenience I guess when we were very much into "flame-graphing" 😂, but I guess it's totally ok respectively likely better if such a tools are just installed in a global scope by everyone who uses them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't in use anywhere that i could find 🤷

and it was responsible for like 9 vulnerabilities 😆

@acolytec3
Copy link
Contributor

So some potential bad news on the front of trying to upgrade the multiaddr dep. That module is now ESM only, so the reason we're seeing this Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in... error in the devp2p examples scripts and in the client is that those being run as CommonJS modules and like so many other challenges with ESM, you can't statically import ESM modules in CJS code. So...since the IPFS/libp2p Javascript world has elected to go ESM only (and not provide CJS builds), we either need to internalize all the multiaddr usage in our code or just pin this old dependency. Multiaddrs are futureproof so in theory our usage should never change.

@acolytec3
Copy link
Contributor

So just to follow up, I'd be a fan of backing out the multiaddr and merging this with just the other stuff removed for now and we tackle multiaddrs in a future update.

@ScottyPoi
Copy link
Contributor Author

So just to follow up, I'd be a fan of backing out the multiaddr and merging this with just the other stuff removed for now and we tackle multiaddrs in a future update.

Ok, I'll rewind that change and make an issue about it for future us.

@ScottyPoi
Copy link
Contributor Author

ScottyPoi commented Jul 25, 2023 •
edited
Loading

strangely -- reverting back to the deprecated mutliaddr v10.0.1 did not bring back the vulnerability warning
🤔

oh, actually it did, 🙄

@ScottyPoi ScottyPoi marked this pull request as ready for review July 25, 2023 04:13
acolytec3
acolytec3 approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM! I think we might be able to get rid of the peerId stuff entirely but that can be a further PR as needed.

@acolytec3 acolytec3 merged commit d971c39 into master Jul 25, 2023
@ScottyPoi ScottyPoi linked an issue Jul 26, 2023 that may be closed by this pull request
Fix Security Vulnerabilities #2903
Closed
@ScottyPoi ScottyPoi mentioned this pull request Jul 26, 2023
Open
@holgerd77 holgerd77 deleted the security-vulnerability branch July 11, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holgerd77 holgerd77 holgerd77 left review comments

@acolytec3 acolytec3 acolytec3 approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file package: client package: devp2p package: monorepo package: mpt PR state: needs review target: master Work to be done towards master branch type: maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix Security Vulnerabilities
3 participants
@ScottyPoi @acolytec3 @holgerd77