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

[deps,bazel] Rust dependency updates for build determinism & cargo audit #25795

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

AlexJones0
Copy link
Contributor

@AlexJones0 AlexJones0 commented Jan 7, 2025

This PR updates the rust dependencies found in third_party/rust to solve determinism and security issues. Primarily due to the usage of outdated versions of mdbook (due to patching) and the unused rust-crypto crate, we had a lot of transitive dependencies with security advisories / vulnerabilities. There is also a separate commit for directly pulling in updates to the rustix and object crates which had build determinism issues (see commit message).

It might be nice to do a more general cargo update, but I'm conservatively updating only what is necessary for now as I was running into build issues in CI that presumably need fixes.

See the commit messages for more details.

Updates the `object` and `rustix` crate dependencies to later versions,
in order to pull in upstream fixes for their non-determinism during
compilation, which can affect FPGA test caching in Bazel and cause
tooling to require unecessary rebuilds. These fixes ensure the output
directory is cleaned appropriately, removing all references to relative
filepaths when the build scripts of the `thiserror-core` and `rustix`
invoke rustc directly, such that compilation is deterministic.

The two commands run are:

cargo update -p object
cargo update -p rustix --precise 0.38.42

Signed-off-by: Alex Jones <[email protected]>
Updates the mdBook dependendency from pinned version 0.4.31 to the
latest version 0.4.43. This update is made because:
 - mdBook (transitively) depends on the `idna` crate, but version
 0.4.31 of mdBook used idna 0.4.0, which is vulnerable:
 https://rustsec.org/advisories/RUSTSEC-2024-0421
 - mdBook uses shlex 1.1.0, which is vulnerable:
 https://rustsec.org/advisories/RUSTSEC-2024-0006

This also involved manually updated the mdBook patch to the newest
version - the patch is the same, but it had to be re-performed on the
newer version.

Signed-off-by: Alex Jones <[email protected]>
The rust-crypto dependency has been previously introduced but is not
being used, and is currently the cause of some security issues within
our dependencies. Specifically:
  - `rust-crypto` itself is un-maintained and has vulnerabilities:
    https://rustsec.org/advisories/RUSTSEC-2022-0011
  - It uses `time` 0.1.45: https://rustsec.org/advisories/RUSTSEC-2020-0071
  - It uses `rustc-serialize` 0.3.25:
    https://rustsec.org/advisories/RUSTSEC-2022-0004

Because this crate is unused, unmaintained and vulnerable, this commit
drops this rust dependency.

Signed-off-by: Alex Jones <[email protected]>
@AlexJones0 AlexJones0 changed the title [deps,bazel] Rust dependency updates for build determinism & security issues [deps,bazel] Rust dependency updates for build determinism & cargo audit Jan 7, 2025
@nbdd0121 nbdd0121 self-requested a review January 7, 2025 15:39
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jan 7, 2025

I think it might worth separating out mdbook into a separate dependency like tock

EDIT: opened #25797

This commit includes a variety of updates to the `Cargo.toml` and
`Cargo.lock` files used by OpenTitan to manage rust dependencies, such
that the majority of the issues/vulnerabilities reported by `cargo
audit` are appropriately addressed. This was chosen in lieu of a full
`cargo update` for now, because that seemed to cause some builds to
break.

The packages updated in this way include:
 - `zerocopy` (vulnerable, version yanked).
 - `indicatif` (used `instant`, which is un-maintained).
 - `rsa` (side-channel vulnerability still remains)
 - `openssl` (vulnerability)
 - `mio` (vulnerability)
 - `idna` (vulnerability)
 - `url` (depends on `idna`)
 - `smallvec` (dependency of `url` and `rsa`)

Signed-off-by: Alex Jones <[email protected]>
@AlexJones0 AlexJones0 force-pushed the cargo_crate_updates branch from 1a3dc53 to d000706 Compare January 7, 2025 15:54
@AlexJones0 AlexJones0 requested review from pamaury and jwnrt January 8, 2025 10:05
Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

Thanks @AlexJones0 for the great effort in addressing non-determinism in the build Regarding cargo audit, we should really have an automated test to make sure that we do not regress without realizing.

Copy link
Contributor

@jwnrt jwnrt 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, thanks

@jwnrt jwnrt merged commit 4a7d59b into lowRISC:master Jan 8, 2025
38 checks passed
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.

4 participants