Skip to content

Commit 65f5ecd

Browse files
committed
Eliminate post-install directory-walk
This was only needed because we were not controlling the permissions accurately during unpack, which can now be corrected.
1 parent 35c9cad commit 65f5ecd

File tree

2 files changed

+27
-62
lines changed

2 files changed

+27
-62
lines changed

README.md

+3
Original file line numberDiff line numberDiff line change
@@ -710,6 +710,9 @@ yet validate signatures of downloads.
710710

711711
[s]: https://github.com/rust-lang/rustup.rs/issues?q=is%3Aopen+is%3Aissue+label%3Asecurity
712712

713+
File modes on installation honor umask as of 1.18.4, use umask if
714+
very tight controls are desired.
715+
713716
## FAQ
714717

715718
### Is this an official Rust project?

src/dist/component/package.rs

+24-62
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,6 @@ impl Package for DirectoryPackage {
120120
}
121121
_ => return Err(ErrorKind::CorruptComponent(name.to_owned()).into()),
122122
}
123-
124-
set_file_perms(&target.prefix().path().join(path), &src_path)?;
125123
}
126124

127125
let tx = builder.finish()?;
@@ -134,65 +132,6 @@ impl Package for DirectoryPackage {
134132
}
135133
}
136134

137-
// On Unix we need to set up the file permissions correctly so
138-
// binaries are executable and directories readable. This shouldn't be
139-
// necessary: the source files *should* have the right permissions,
140-
// but due to rust-lang/rust#25479 they don't.
141-
#[cfg(unix)]
142-
fn set_file_perms(dest_path: &Path, src_path: &Path) -> Result<()> {
143-
use std::fs::{self, Metadata};
144-
use std::os::unix::fs::PermissionsExt;
145-
use walkdir::WalkDir;
146-
147-
// Compute whether this entry needs the X bit
148-
fn needs_x(meta: &Metadata) -> bool {
149-
meta.is_dir() || // Directories need it
150-
meta.permissions().mode() & 0o700 == 0o700 // If it is rwx for the user, it gets the X bit
151-
}
152-
153-
// By convention, anything in the bin/ directory of the package is a binary
154-
let is_bin = if let Some(p) = src_path.parent() {
155-
p.ends_with("bin")
156-
} else {
157-
false
158-
};
159-
160-
let is_dir = utils::is_directory(dest_path);
161-
162-
if is_dir {
163-
// Walk the directory setting everything
164-
for entry in WalkDir::new(dest_path) {
165-
let entry = entry.chain_err(|| ErrorKind::ComponentDirPermissionsFailed)?;
166-
let meta = entry
167-
.metadata()
168-
.chain_err(|| ErrorKind::ComponentDirPermissionsFailed)?;
169-
170-
let mut perm = meta.permissions();
171-
perm.set_mode(if needs_x(&meta) { 0o755 } else { 0o644 });
172-
fs::set_permissions(entry.path(), perm)
173-
.chain_err(|| ErrorKind::ComponentFilePermissionsFailed)?;
174-
}
175-
} else {
176-
let meta =
177-
fs::metadata(dest_path).chain_err(|| ErrorKind::ComponentFilePermissionsFailed)?;
178-
let mut perm = meta.permissions();
179-
perm.set_mode(if is_bin || needs_x(&meta) {
180-
0o755
181-
} else {
182-
0o644
183-
});
184-
fs::set_permissions(dest_path, perm)
185-
.chain_err(|| ErrorKind::ComponentFilePermissionsFailed)?;
186-
}
187-
188-
Ok(())
189-
}
190-
191-
#[cfg(windows)]
192-
fn set_file_perms(_dest_path: &Path, _src_path: &Path) -> Result<()> {
193-
Ok(())
194-
}
195-
196135
#[derive(Debug)]
197136
pub struct TarPackage<'a>(DirectoryPackage, temp::Dir<'a>);
198137

@@ -273,7 +212,30 @@ fn unpack_without_first_dir<'a, R: Read>(
273212
// - it is most likely an attack, as rusts cross-platform nature precludes
274213
// such artifacts
275214
let kind = entry.header().entry_type();
276-
let mode = entry.header().mode().ok().unwrap();
215+
// https://github.com/rust-lang/rustup.rs/issues/1140 and before that
216+
// https://github.com/rust-lang/rust/issues/25479
217+
// tl;dr: code got convoluted and we *may* have damaged tarballs out
218+
// there.
219+
// However the mandate we have is very simple: unpack as the current
220+
// user with modes matching the tar contents. No documented tars with
221+
// bad modes are in the bug tracker : the previous permission splatting
222+
// code was inherited from interactions with sudo that are best
223+
// addressed outside of rustup (run with an appropriate effective uid).
224+
// THAT SAID: If regressions turn up immediately post release this code -
225+
// https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a8549057f0827bf3a068d8917256765a
226+
// is a translation of the prior helper function into an in-iterator
227+
// application.
228+
let tar_mode = entry.header().mode().ok().unwrap();
229+
// That said, the tarballs that are shipped way back have single-user
230+
// permissions:
231+
// -rwx------ rustbuild/rustbuild ..... release/test-release.sh
232+
// so we should normalise the mode to match the previous behaviour users
233+
// may be expecting where the above file would end up with mode 0o755
234+
let u_mode = tar_mode & 0o700;
235+
let g_mode = (u_mode & 0o0500) >> 3;
236+
let o_mode = g_mode >> 3;
237+
let mode = u_mode | g_mode | o_mode;
238+
277239
let item = match kind {
278240
EntryType::Directory => Item::make_dir(full_path, mode),
279241
EntryType::Regular => {

0 commit comments

Comments
 (0)