brokerd: refuse / and a symbolic link as a socket's directory
brokerd makes a socket's directory 0700. With a socket directly in / it would chmod /, and through a symbolic link it would change the link's target. Both are now refused at start with #brokerd-start-failed. Without the fix the link case started and served, with the shared directory made private. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -99,9 +99,18 @@ pub fn start(
|
|||||||
/// Bind one socket: prepare its directory at 0700, make it private, remove any stale
|
/// Bind one socket: prepare its directory at 0700, make it private, remove any stale
|
||||||
/// socket, bind, then make the socket itself private at 0600.
|
/// socket, bind, then make the socket itself private at 0600.
|
||||||
fn listen(socket: &Path) -> Result<UnixListener, ServeError> {
|
fn listen(socket: &Path) -> Result<UnixListener, ServeError> {
|
||||||
|
// A socket needs a directory of its own: step 2 makes that directory 0700, which must never
|
||||||
|
// be `/` or a shared directory reached through a symbolic link.
|
||||||
let dir = match socket.parent() {
|
let dir = match socket.parent() {
|
||||||
Some(parent) => parent.to_path_buf(),
|
Some(parent) if parent.parent().is_some() && !parent.as_os_str().is_empty() => {
|
||||||
None => PathBuf::from("/"),
|
parent.to_path_buf()
|
||||||
|
}
|
||||||
|
_ => {
|
||||||
|
return Err(ServeError::Dir(
|
||||||
|
PathBuf::from(socket),
|
||||||
|
io::Error::other("a socket needs a directory of its own, not / or none"),
|
||||||
|
));
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
// 1. The directory, at 0700.
|
// 1. The directory, at 0700.
|
||||||
@@ -110,6 +119,19 @@ fn listen(socket: &Path) -> Result<UnixListener, ServeError> {
|
|||||||
.mode(0o700)
|
.mode(0o700)
|
||||||
.create(&dir)
|
.create(&dir)
|
||||||
.map_err(|e| ServeError::Dir(dir.clone(), e))?;
|
.map_err(|e| ServeError::Dir(dir.clone(), e))?;
|
||||||
|
// `set_permissions` follows a symbolic link, so a link would make the directory it points to
|
||||||
|
// private instead: refuse it.
|
||||||
|
let kind = std::fs::symlink_metadata(&dir)
|
||||||
|
.map_err(|e| ServeError::Dir(dir.clone(), e))?
|
||||||
|
.file_type();
|
||||||
|
if !kind.is_dir() {
|
||||||
|
return Err(ServeError::Dir(
|
||||||
|
dir,
|
||||||
|
io::Error::other(
|
||||||
|
"is a symbolic link or not a directory; brokerd will not change its mode",
|
||||||
|
),
|
||||||
|
));
|
||||||
|
}
|
||||||
// 2. Make it private, always, even when it was already there at 0755.
|
// 2. Make it private, always, even when it was already there at 0755.
|
||||||
std::fs::set_permissions(&dir, Permissions::from_mode(0o700))
|
std::fs::set_permissions(&dir, Permissions::from_mode(0o700))
|
||||||
.map_err(|e| ServeError::Dir(dir.clone(), e))?;
|
.map_err(|e| ServeError::Dir(dir.clone(), e))?;
|
||||||
|
|||||||
@@ -99,6 +99,51 @@ fn a_socket_directory_that_cannot_be_made_names_the_entry() {
|
|||||||
]));
|
]));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The socket's directory is made private (0700). Through a symbolic link that would change the
|
||||||
|
/// directory it points to, so a link is refused, and the target keeps its mode.
|
||||||
|
#[test]
|
||||||
|
fn a_socket_directory_that_is_a_symbolic_link_is_refused() {
|
||||||
|
use std::os::unix::fs::PermissionsExt;
|
||||||
|
let dir = TempDir::new("ptr-link");
|
||||||
|
let shared = dir.path().join("shared");
|
||||||
|
std::fs::create_dir(&shared).unwrap();
|
||||||
|
std::fs::set_permissions(&shared, std::fs::Permissions::from_mode(0o755)).unwrap();
|
||||||
|
let link = dir.path().join("link");
|
||||||
|
std::os::unix::fs::symlink(&shared, &link).unwrap();
|
||||||
|
let path = config(
|
||||||
|
&dir,
|
||||||
|
&format!("broker = \"{}\"", link.join("broker.sock").display()),
|
||||||
|
);
|
||||||
|
|
||||||
|
let out = brokerd([
|
||||||
|
OsStr::new("serve"),
|
||||||
|
OsStr::new("--config"),
|
||||||
|
path.as_os_str(),
|
||||||
|
]);
|
||||||
|
fails_with_pointer(&out);
|
||||||
|
assert!(stderr(&out).contains("symbolic link"), "{}", stderr(&out));
|
||||||
|
let mode = std::fs::metadata(&shared).unwrap().permissions().mode() & 0o777;
|
||||||
|
assert_eq!(mode, 0o755, "the link's target keeps its mode");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A socket directly in `/` would make `/` private: refused before anything is changed.
|
||||||
|
#[test]
|
||||||
|
fn a_socket_in_the_root_directory_is_refused() {
|
||||||
|
let dir = TempDir::new("ptr-root");
|
||||||
|
let path = config(&dir, "broker = \"/broker.sock\"");
|
||||||
|
let out = brokerd([
|
||||||
|
OsStr::new("serve"),
|
||||||
|
OsStr::new("--config"),
|
||||||
|
path.as_os_str(),
|
||||||
|
]);
|
||||||
|
fails_with_pointer(&out);
|
||||||
|
assert!(
|
||||||
|
stderr(&out).contains("directory of its own"),
|
||||||
|
"{}",
|
||||||
|
stderr(&out)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// A config path that is not UTF-8 is still a path: it is read (here: not found), not a panic.
|
/// A config path that is not UTF-8 is still a path: it is read (here: not found), not a panic.
|
||||||
#[test]
|
#[test]
|
||||||
fn a_config_path_that_is_not_utf8_is_read_as_a_path() {
|
fn a_config_path_that_is_not_utf8_is_read_as_a_path() {
|
||||||
|
|||||||
@@ -182,6 +182,10 @@ programs off it.
|
|||||||
- `cannot prepare <dir>: …` — the directory cannot be made or `chmod`ed:
|
- `cannot prepare <dir>: …` — the directory cannot be made or `chmod`ed:
|
||||||
`ls -ld <dir> "$(dirname <dir>)"`. A path that runs through a file, or a directory owned by
|
`ls -ld <dir> "$(dirname <dir>)"`. A path that runs through a file, or a directory owned by
|
||||||
another user, gives this.
|
another user, gives this.
|
||||||
|
- `cannot prepare <socket or dir>: a socket needs a directory of its own` or `…: is a symbolic
|
||||||
|
link or not a directory` — `brokerd` makes a socket's directory 0700, so it refuses `/` and a
|
||||||
|
directory reached through a link (the link's target would be changed instead). Give each socket
|
||||||
|
its own real directory, as the defaults under `$BOXMAKER_HOME/run/` are.
|
||||||
- `cannot listen on <socket>: …` — `path must be shorter than SUN_LEN` means the socket path is
|
- `cannot listen on <socket>: …` — `path must be shorter than SUN_LEN` means the socket path is
|
||||||
longer than 107 bytes; `Address already in use` means something still listens there
|
longer than 107 bytes; `Address already in use` means something still listens there
|
||||||
(`ss -xlp | grep <socket>`).
|
(`ss -xlp | grep <socket>`).
|
||||||
|
|||||||
Reference in New Issue
Block a user