Security advisory for crates.io, 2017-09-19

announcements
1

Hey all; a cross post from the announcement list at Redirecting to Google Groups

That link contains a signed version with our PGP key, as well.

The Rust team was recently notified of a security vulnerability affecting
crates.io. It has since been resolved, and there is no indication that the bug
has been exploited. No action is necessary from any user, this email is a
notice about the vulnerability in addition to a disclosure of the timeline of
events.

When Cargo extracted the source code for a package on crates.io it could
overwrite the source code for another package.

Source code is stored on crates.io as a tarball, and Cargo uses the tar crate
to parse these tarballs and extract them. Historically Cargo extracted these
tarballs directly into the global crate cache 1, relying on each tarball to
only actually have one directory of the form foo-0.1.0 ($name-$version).
This was not verified, however, and if a tarball contained multiple top-level
directories then the extraction would overwrite or create new files in the
global crate cache.

Cargo itself would never manufacture a tarball with more than one top-level
directory, but a malicious tarball could be uploaded directly to crates.io
which could exhibit this behavior. Cargo's cache for downloaded crates is a
global one, which means that an unrelated project could poison anothers'
dependency graph by overwriting a crate's source code, causing malicious code
to be compiled and executed eventually.

A fix for this issue was first implemented on crates.io 2 to reject any
tarball being published which contains a top-level directory not of the form
expected ($crate_name-$crate_version). After this patch was merged and
deployed all tarballs ever published to crates.io were downloaded and
inspected. No tarball ever uploaded to crates.io exhibited malicious behavior
with multiple top-level directories.

As a second precaution a change was also landed in Cargo 3 which causes Cargo
to return an error if a tarball looks ill-formed. This change will be present
in Cargo of Rust 1.22.0, to be released 2017-11-23. To reiterate, this change
is not needed to protect from malicious downloads from crates.io. Only
well-formed tarballs can be uploaded to crates.io as of a few days ago.

Many thanks to Justin Geibel (@jtgeibel) for responsibly reporting this 4 and
helping us identify and test a fix! The timeline of events is as follows:

  • 2017-09-13 @ 19:19 - Justin sends mail to security@rust-lang.org about this
  • 2017-09-13 @ 19:28 - response sent to Justin acknowledging the vulnerability
  • 2017-09-13 @ 21:14 - a patch to crates.io was finished, all current
    tarballs verified not-malicious
  • 2017-09-14 @ 9:10 - crates.io PR proposed
  • 2017-09-14 @ 10:04 - crates.io PR merged and deployed
  • 2017-09-14 @ 12:08 - cargo PR proposed
  • 2017-09-14 @ 12:26 - crates.io verified again to contain no malicious tarballs
  • 2017-09-17 @ 16:10 - cargo PR merged
29 Likes
2

Just for my curiousity: the description of the issue implies that the tarball downloaded when Cargo fetches a dependency is not the one that was originally uploaded, but is recreated on the fly from an uncompressed directory. Is that how it works? Is there a reason not to store tarballs instead?

3

@SimonSapin as I understand it, crates.io does store and serve the originally uploaded tarball. The user's local cargo downloads the tarball, unpacks it, and builds the code. The local unpack is the one that could attack the end user's global crate cache.

1 Like
4

Oh that’s right, Cargo’s crate cache on clients. I assumed some cache on the crates.io server.

1 Like
5

In the future it would be useful to describe the attack vector that would have allowed a user to be compromised. From my initial reading of this bug I believed that this was a server-side flaw that would have allowed every package to be compromised for every user of crates.io, but upon closer inspection it seems like it's a client-side vuln where the attack would have required the user to add a malicious dep. These two scenarios are vastly different in severity. :slight_smile:

7 Likes
6

If tar handling of Cargo being fixed then maybe handling of permission bits should be fixed along the way (if not already)?

As far as I remember, tar archive can contain permission bits different than typical 0644/0755 and unpacking tries to preserve them instead of behaving like Git, complicating shared cache multiuser setup.

7

So, correct me if I'm wrong but tar archives can contain files named like mypackage-1.0/../anotherpackage-1.0/somefile.rs and as far as I can see the code doesn't check for that.

1 Like
8

But the other code apparently does: tar::Entry - Rust

This function carefully avoids writing outside of dst. If the file has a '..' in its path, this function will skip it and return false.

2 Likes
9

Ah yes that seems safe.