Verify tarballs don't extract into other directories

diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs
index e35d995685deb0da02e8f90060104a15b30d711f..187a64dbef6218f7588e682750d9881c88a44f48 100644 (file)
--- a/src/cargo/sources/registry/mod.rs
+++ b/src/cargo/sources/registry/mod.rs
@@ -313,9 +313,33 @@ impl<'cfg> RegistrySource<'cfg> {
 
         let gz = GzDecoder::new(tarball.file())?;
         let mut tar = Archive::new(gz);
-        tar.unpack(dst.parent().unwrap())?;
+        let prefix = dst.file_name().unwrap();
+        let parent = dst.parent().unwrap();
+        for entry in tar.entries()? {
+            let mut entry = entry.chain_err(|| "failed to iterate over archive")?;
+            let entry_path = entry.path()
+                .chain_err(|| "failed to read entry path")?
+                .into_owned();
+
+            // We're going to unpack this tarball into the global source
+            // directory, but we want to make sure that it doesn't accidentally
+            // (or maliciously) overwrite source code from other crates. Cargo
+            // itself should never generate a tarball that hits this error, and
+            // crates.io should also block uploads with these sorts of tarballs,
+            // but be extra sure by adding a check here as well.
+            if !entry_path.starts_with(prefix) {
+                return Err(format!("invalid tarball downloaded, contains \
+                                    a file at {:?} which isn't under {:?}",
+                                   entry_path, prefix).into())
+            }
+
+            // Once that's verified, unpack the entry as usual.
+            entry.unpack_in(parent).chain_err(|| {
+                format!("failed to unpack entry at `{}`", entry_path.display())
+            })?;
+        }
         File::create(&ok)?;
-        Ok(dst)
+        Ok(dst.clone())
     }
 
     fn do_update(&mut self) -> CargoResult<()> {

diff --git a/tests/cargotest/support/registry.rs b/tests/cargotest/support/registry.rs
index 49ab93901756401026504fdd379c3c003f741d88..3046d2e20c5e0f1f76687b6fc0dea5fb4ea11130 100644 (file)
--- a/tests/cargotest/support/registry.rs
+++ b/tests/cargotest/support/registry.rs
@@ -24,6 +24,7 @@ pub struct Package {
     vers: String,
     deps: Vec<Dependency>,
     files: Vec<(String, String)>,
+    extra_files: Vec<(String, String)>,
     yanked: bool,
     features: HashMap<String, Vec<String>>,
     local: bool,
@@ -72,6 +73,7 @@ impl Package {
             vers: vers.to_string(),
             deps: Vec::new(),
             files: Vec::new(),
+            extra_files: Vec::new(),
             yanked: false,
             features: HashMap::new(),
             local: false,
@@ -88,6 +90,11 @@ impl Package {
         self
     }
 
+    pub fn extra_file(&mut self, name: &str, contents: &str) -> &mut Package {
+        self.extra_files.push((name.to_string(), contents.to_string()));
+        self
+    }
+
     pub fn dep(&mut self, name: &str, vers: &str) -> &mut Package {
         self.full_dep(name, vers, None, "normal", &[])
     }
@@ -235,14 +242,22 @@ impl Package {
                 self.append(&mut a, name, contents);
             }
         }
+        for &(ref name, ref contents) in self.extra_files.iter() {
+            self.append_extra(&mut a, name, contents);
+        }
     }
 
     fn append<W: Write>(&self, ar: &mut Builder<W>, file: &str, contents: &str) {
+        self.append_extra(ar,
+                          &format!("{}-{}/{}", self.name, self.vers, file),
+                          contents);
+    }
+
+    fn append_extra<W: Write>(&self, ar: &mut Builder<W>, path: &str, contents: &str) {
         let mut header = Header::new_ustar();
         header.set_size(contents.len() as u64);
-        t!(header.set_path(format!("{}-{}/{}", self.name, self.vers, file)));
+        t!(header.set_path(path));
         header.set_cksum();
-
         t!(ar.append(&header, contents.as_bytes()));
     }
 

diff --git a/tests/registry.rs b/tests/registry.rs
index e7bc06e4e4dbec93c006ee5132e33057e9caaffe..c63310e648694f9f6dc34088c3fea76546d1c4ed 100644 (file)
--- a/tests/registry.rs
+++ b/tests/registry.rs
@@ -1408,3 +1408,40 @@ fn vv_prints_warnings() {
     assert_that(p.cargo("build").arg("-vv"),
                 execs().with_status(0));
 }
+
+#[test]
+fn bad_and_or_malicious_packages_rejected() {
+    Package::new("foo", "0.2.0")
+            .extra_file("foo-0.1.0/src/lib.rs", "")
+            .publish();
+
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "fo"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies]
+            foo = "0.2"
+        "#)
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    assert_that(p.cargo("build").arg("-vv"),
+                execs().with_status(101)
+                       .with_stderr("\
+[UPDATING] [..]
+[DOWNLOADING] [..]
+error: unable to get packages from source
+
+Caused by:
+  failed to download [..]
+
+Caused by:
+  failed to unpack [..]
+
+Caused by:
+  [..] contains a file at \"foo-0.1.0/src/lib.rs\" which isn't under \"foo-0.2.0\"
+"));
+}