fix: RUSTSEC-2020-0071 #3150

hanabi1224 · 2023-02-25T02:18:54Z

This PR tries to fix https://rustsec.org/advisories/RUSTSEC-2020-0071 by removing time 0.1 from dependency tree

CLAassistant · 2023-02-25T02:18:59Z

All committers have signed the CLA.

hanabi1224 · 2023-02-25T02:19:49Z

Cargo.lock

@@ -3387,17 +3484,6 @@ version = "0.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "3bf63baf9f5039dadc247375c29eb13706706cfde997d0330d05aa63a77d8820"

-[[package]]
-name = "time"


time v0.1 removed

Manishearth · 2023-02-25T04:51:33Z

Not a fan of zip getting even more dependencies, but whatever

Manishearth

Is there a reason behind the additional crate updates?

Manishearth · 2023-03-01T17:11:44Z

provider/datagen/Cargo.toml

 syn = {version = "1.0", features = ["parsing"] }
 tinystr = { version = "0.7.1", path = "../../utils/tinystr", features = ["alloc", "serde", "zerovec"], default-features = false }
 toml = "0.5"
 writeable = { version = "0.5.1", path = "../../utils/writeable" }
 zerovec = { version = "0.9.2", path = "../../utils/zerovec", features = ["serde", "yoke"] }
-zip = { version = "0.5", default-features = false, features = ["deflate"] }
+zip = { version = "0.6", default-features = false, features = ["deflate"] }


I'd rather not update zip to 0.6 just yet since we're trying to keep dependencies in sync with other places.

zip 0.5 depends on time 0.1, which is the root cause of this rustsec issue

We are not using zip with that feature enabled

If you would like to, you can add a comment to the dependency allowlist for zip as used by datagen and mention that the list should never contain time unless zip is 0.6.

Another thing that would be fine is using a range dep: ">=0.5.0, <0.7.0"; basically I do not want to lose the ability to use zip 0.5

zip v0.6 is also one of the dependencies of cached-path v0.6.1, a range dep >=0.5.0, <0.7.0 does not change the version resolution tho

cached_path can also get a range, then

It should be possible to use ICU4X with zip 0.5, basically.

Again, all of this is moot for the time vuln: we're not using zip with that feature enabled

Ah, i guess cached-path does, which is a bit of a bummer

Manishearth · 2023-03-01T17:23:53Z

Also this fails MSRV

error: package clap_lex v0.3.2 cannot be built because it requires rustc 1.64.0 or newer, while the currently active rustc version is 1.61.0

jira-pull-request-webhook · 2023-03-01T17:39:56Z

Notice: the branch changed across the force-push!

Cargo.lock is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

hanabi1224 · 2023-03-01T18:04:42Z

Also this fails MSRV

error: package clap_lex v0.3.2 cannot be built because it requires rustc 1.64.0 or newer, while the currently active rustc version is 1.61.0

@Manishearth Correct me if I'm wrong but I think to achieve the MSRV rule, versions of clap_lex, time-macros needs to be pinned to certain patch versions explicitly in Cargo.toml, another crate that depends on this lib does not inherit the Cargo.lock and it won't build with rust 1.61.0 out-of-box.

IMHO, to properly test MSRV for a lib crate, it needs to build a crate that depends on this lib without Cargo.lock (or by simply removing Cargo.lock before building this crate), which is the real world scenario. Having the current MSRV CI passing or not does not really matter.

Manishearth · 2023-03-01T18:22:45Z

No, it is sufficient to pin it in the lockfile here.

A crate that depends on this will not build with rust 1.61 out-of-the-box, but they can fix this themselves as needed in their own lockfile, it's a pretty normal workflow. While some high-profile crates do guarantee MSRV for themselves and their dependencies, by and large the community does not do this as it usually involves a lot of coordination, and library authors often do not appreciate being asked to sign up for an MSRV because their dependents wish to do so. There are issues open to make Cargo dependency resolution take msrv into account, and most library authors seem to be holding out for that.

We achieve an approximation of the out-of-the-box property by having a relatively small number of external dependencies on the main ICU4X crates. If a lockfile pin was needed for a dependency of a main ICU4X crate (not datagen) it would give me some pause, but that's not the case here. We do plan on adding lockfile tests (minimal/maximal versions) and probably will add an MSRV test scoped for the core ICU4X crate.

Datagen is a bit special since we don't actually care as much about MSRV there especially in the binary mode. We test it because it's easier to test everything than to test a subset and then accidentally missing stuff, and we do care about MSRV a little bit for library mode (but not enough to want to maintain the out-of-the-box property, datagen is mostly a build time dep for people if they use it via Cargo at all, which is somewhat rare).

Manishearth · 2023-03-01T18:25:20Z

Having the current MSRV CI passing or not does not really matter.

The MSRV CI matters because it means that users have the option to build with MSRV. They are not our only users, there will be users who do not care about MSRV but do have newer versions of a crate in their deptree, and we disadvantage them by forcing multiple versions of a crate (which Cargo can often handle, but sometimes it breaks other things). It's a tradeoff without a single clear answer.

Manishearth · 2023-03-01T18:27:58Z

Regarding zip, I'm working on resolving the 0.5 issue elsewhere. I'd still like a range dep for now, though.

Manishearth · 2023-03-01T18:38:21Z

(As to why I don't mind that cached-path depends on zip: cached-path is a feature that can be disabled and is not relevant in the context where I need zip 0.5)

hanabi1224 · 2023-03-01T18:44:10Z

@Manishearth fixed as u suggested

Manishearth · 2023-03-01T19:02:52Z

provider/datagen/Cargo.toml

@@ -77,7 +77,7 @@ tinystr = { version = "0.7.1", path = "../../utils/tinystr", features = ["alloc"
 toml = "0.5"
 writeable = { version = "0.5.1", path = "../../utils/writeable" }
 zerovec = { version = "0.9.2", path = "../../utils/zerovec", features = ["serde", "yoke"] }
-zip = { version = "0.6", default-features = false, features = ["deflate"] }
+zip = { version = ">=0.5, <0.7", default-features = false, features = ["deflate"] }


you should also cargo update -p zip to get the new version so that the time 0.1 dep disappears from the lockfile

and cached-path as well

I'm fine maintaining the property that time 0.1 isn't in our lockfile, even if clients will have the opportunity to use older crates.

Sure but again the lock file does not effect the crate user, I thought it would be easier for u to keep it 0.5 while I can upgrade to 0.6 if that's preferable

Yes, I understand how lockfiles work.

"A crate with a vulnerability is not in the lockfile" is easy for tooling to check and maintain, even though it doesn't mean we've completely excised the dep from all theoretical dependency chains.

And like I said clients needing to stick to zip 0.5 can, because we are giving them the flexibility.

Ultimately dependency management is something that the application developer at the end of the dependency tree must do: as library authors we can make the job easy for them by reducing deps and keeping things flexible, but different people have different needs and we cannot guess those as library authors. Some people care about MSRV. Others have vendored crates and want to stick to those versions as much as possible. Different people have different security stances around vulnerabilities.

You're seeing the confluence of a bunch of different requirements here.

@Manishearth time v0.1 is now removed from Cargo.lock

Because a lot of larger projects (for example Firefox, but there's also stuff in Google) vendor their dependencies, and also care about auditing dependencies. If they already have zip 0.5 they may prefer to stick to that. This is what I meant by "we're trying to keep dependencies in sync with other places": new dependencies are additional overhead for some people, and allowing users the flexibility to avoid that is good.

Not everyone is okay with a bloated dependency tree with multiple versions of the same crate. Different people have different needs and tradeoffs. Not every user of ICU4X is even using Cargo to drive the build.

I think it's the best to use mono repo and workspace dependencies to avoid duplicate crates in dependency tree. For isolated crate repos it adds unnecessary and unmanageable maintenance burden. Anyways thanks for the review!

........ yes, precisely, that's my point. We have clients that use monorepos. They would like to avoid duplicate crates, and they would like to avoid having to patch ICU4X's dep spec (or have to figure out when our dependencies are malleable). Expressing that flexibility with range deps helps.

We cannot use a dependency-vendored monorepo because ultimately we are a library and have multiple clients. We aren't trying to avoid duplicate crates, but we are trying to ensure that we do not force duplicate crates onto clients as much as possible.

This is a perfectly manageable maintenance burden for us. Aside from datagen, ICU4X has a low number of dependencies, and even datagen has a manageable number of dependencies if built in certain modes, and we have tests ensuring that.

Arguably, as a lib crate I think the CI needs to cover all combinations of zip, cached-path versions. And if more crate more versions are added the num of combinations soon become 'unmanageable'

As I said already, we do have plans for minimal/maximal version testing, and hope to do an MSRV test with minimal/maximal versions for a subset of the project (so, not datagen, would not affect zip/cached-path). Currently we achieve some of these properties to an acceptable level by having very few dependencies on the main part of the project.

There are diminishing returns after you have minimal/maximal testing.

And ultimately it is normal in the Rust community for applications to run a bunch of cargo update --precises if they have specific additional constraints for their dependencies.

robertbastian

lgtm

…2685) After this, we are still waiting for `indexmap` & `dashmap` to provide the new `hashbrown` to reduce duplicate dependencies, and for `criterion` to remove `clap` and release a new version. We're also waiting for a new version of `icu_datagen` that bumps the `zip` dependency to avoid a potential vulnerability. Ideally, they would also bump the `simple_logger` dependency, which is pretty outdated. In any case, `simple_logger` still uses an unmaintained `atty` dependency. Relevant issues: - xacrimon/dashmap#250 - unicode-org/icu4x#3150 - bheisler/criterion.rs#599 - borntyping/rust-simple_logger#74

fix: RUSTSEC-2020-0071

21f0486

hanabi1224 requested review from sffc, robertbastian, Manishearth, filmil and a team as code owners February 25, 2023 02:18

hanabi1224 commented Feb 25, 2023

View reviewed changes

Manishearth previously approved these changes Feb 25, 2023

View reviewed changes

sffc removed their request for review February 27, 2023 20:35

Merge remote-tracking branch 'origin/main' into fix-rustsec-2020-0071

dc9834c

hanabi1224 dismissed Manishearth’s stale review via dc9834c March 1, 2023 13:56

Manishearth requested changes Mar 1, 2023

View reviewed changes

revert clap_lex to "=0.3.0"

f87a6c1

hanabi1224 force-pushed the fix-rustsec-2020-0071 branch from d3203e2 to f87a6c1 Compare March 1, 2023 17:39

range dep

1944af9

Manishearth reviewed Mar 1, 2023

View reviewed changes

remove time v0.1 from Cargo.lock

7102582

Manishearth approved these changes Mar 1, 2023

View reviewed changes

Manishearth merged commit d8e0a06 into unicode-org:main Mar 1, 2023

hanabi1224 deleted the fix-rustsec-2020-0071 branch March 1, 2023 20:28

robertbastian reviewed Mar 7, 2023

View reviewed changes

Razican mentioned this pull request Mar 17, 2023

[Merged by Bors] - Updated dependencies, removes remove_dir_all, which is vulnerable boa-dev/boa#2685

Closed

Razican mentioned this pull request Mar 31, 2023

RUSTSEC-2020-0071: Potential segfault in the time crate boa-dev/boa#1671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: RUSTSEC-2020-0071 #3150

fix: RUSTSEC-2020-0071 #3150

hanabi1224 commented Feb 25, 2023

CLAassistant commented Feb 25, 2023 •

edited

Loading

hanabi1224 Feb 25, 2023 •

edited

Loading

Manishearth commented Feb 25, 2023

Manishearth left a comment

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth commented Mar 1, 2023

jira-pull-request-webhook bot commented Mar 1, 2023

hanabi1224 commented Mar 1, 2023 •

edited

Loading

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

hanabi1224 commented Mar 1, 2023

Manishearth Mar 1, 2023

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023 •

edited

Loading

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023

Manishearth Mar 1, 2023

hanabi1224 Mar 1, 2023

Manishearth Mar 1, 2023

robertbastian left a comment

fix: RUSTSEC-2020-0071 #3150

fix: RUSTSEC-2020-0071 #3150

Conversation

hanabi1224 commented Feb 25, 2023

CLAassistant commented Feb 25, 2023 • edited Loading

hanabi1224 Feb 25, 2023 • edited Loading

Choose a reason for hiding this comment

Manishearth commented Feb 25, 2023

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Mar 1, 2023

jira-pull-request-webhook bot commented Mar 1, 2023

hanabi1224 commented Mar 1, 2023 • edited Loading

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

Manishearth commented Mar 1, 2023

hanabi1224 commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

CLAassistant commented Feb 25, 2023 •

edited

Loading

hanabi1224 Feb 25, 2023 •

edited

Loading

hanabi1224 commented Mar 1, 2023 •

edited

Loading

hanabi1224 Mar 1, 2023 •

edited

Loading