Fix audit findings: path traversal, exec shell, glob dup, dead bread-sync, version drift
- modules_mgmt.rs: reject module names containing path separators, `..`,
or absolute paths before joining onto modules_dir (install_from_local,
remove_module, read_module_manifest); adds canonicalized containment
check as defense in depth. Manifest-supplied names and CLI args were
previously joined unsanitized, allowing path traversal on install/remove.
- breadd/src/lua/mod.rs: bread.exec now runs via `sh -c` instead of
`sh -lc`; no documented reason was found for login-shell semantics.
- Unify the two independently hand-written glob matchers (subscription
dispatch in breadd/src/core/subscriptions.rs vs. the CLI --filter path
in breadd/src/ipc/mod.rs) into one implementation in
bread-shared/src/glob.rs, used by both call sites.
- Remove the dead bread-sync/ tree (already excluded from the workspace
and fully unreferenced) and its stale PKGBUILD deps (libgit2, git
optdepend) and packaging docs mention.
- Correct the version-number transposition bug ("6.2.0" instead of
"0.6.2"/"0.6.6") across bread-shared, breadd, and bread-cli Cargo.toml,
and fix PKGBUILD's stale pkgver, so Cargo.toml/doctor/PKGBUILD all agree
with the latest git tag (v0.6.6).
This commit is contained in:
parent
1fda781b4c
commit
89c5849539
25 changed files with 319 additions and 3085 deletions
|
|
@ -1,6 +1,6 @@
|
|||
[package]
|
||||
name = "bread-cli"
|
||||
version = "6.2.0"
|
||||
version = "0.6.6"
|
||||
edition = "2021"
|
||||
|
||||
[[bin]]
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ use anyhow::{bail, Context, Result};
|
|||
use chrono::Utc;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::fs;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::path::{Component, Path, PathBuf};
|
||||
|
||||
/// Contents of `bread.module.toml`.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
|
|
@ -45,6 +45,67 @@ pub fn parse_source(source: &str) -> Result<PathBuf> {
|
|||
}
|
||||
}
|
||||
|
||||
/// Validate that a module name is safe to join onto `modules_dir`.
|
||||
///
|
||||
/// Module names ultimately come from untrusted input: a manifest file
|
||||
/// (`bread.module.toml`, which could be crafted by anyone who hands the user
|
||||
/// a "module" to install) or a raw CLI argument (`bread modules remove
|
||||
/// <name>`). Without this check, a name like `../../../../etc` or an
|
||||
/// absolute path would let install/remove escape `modules_dir` entirely —
|
||||
/// classic path traversal. Reject any name containing a path separator,
|
||||
/// a `..` component, or that is otherwise not a single plain path segment.
|
||||
fn validate_module_name(name: &str) -> Result<()> {
|
||||
if name.is_empty() {
|
||||
bail!("bread: module name must not be empty");
|
||||
}
|
||||
let path = Path::new(name);
|
||||
// A valid module name must be exactly one normal path component, e.g.
|
||||
// it must not contain `/`, must not be `.`/`..`, and must not be an
|
||||
// absolute path or reference a prefix/root.
|
||||
let mut components = path.components();
|
||||
match (components.next(), components.next()) {
|
||||
(Some(Component::Normal(seg)), None) if seg == name => {}
|
||||
_ => {
|
||||
bail!(
|
||||
"bread: invalid module name '{}' (must be a single path segment, \
|
||||
no '/', '..', or absolute paths)",
|
||||
name
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Join `name` onto `modules_dir`, validating the name and verifying the
|
||||
/// resulting path is still contained within `modules_dir`.
|
||||
///
|
||||
/// This is defense in depth on top of [`validate_module_name`]: even if the
|
||||
/// name passes the component check, we canonicalize the parent directory
|
||||
/// and confirm the joined path's parent resolves to it before allowing any
|
||||
/// filesystem operation on the result.
|
||||
fn resolve_module_dir(name: &str, modules_dir: &Path) -> Result<PathBuf> {
|
||||
validate_module_name(name)?;
|
||||
let dest = modules_dir.join(name);
|
||||
|
||||
// Canonicalize modules_dir itself (it should exist by the time we're
|
||||
// installing/removing/reading from it in practice; callers that need it
|
||||
// pre-creation call fs::create_dir_all first).
|
||||
if let Ok(canonical_root) = modules_dir.canonicalize() {
|
||||
if let Some(parent) = dest.parent() {
|
||||
if let Ok(canonical_parent) = parent.canonicalize() {
|
||||
if canonical_parent != canonical_root {
|
||||
bail!(
|
||||
"bread: resolved module path '{}' escapes modules directory",
|
||||
dest.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(dest)
|
||||
}
|
||||
|
||||
/// Install a module from a local directory into `modules_dir`.
|
||||
/// `source_str` is the original source string recorded in the manifest.
|
||||
pub fn install_from_local(
|
||||
|
|
@ -65,7 +126,9 @@ pub fn install_from_local(
|
|||
manifest.source = source_str.to_string();
|
||||
manifest.installed_at = Utc::now().to_rfc3339();
|
||||
|
||||
let dest = modules_dir.join(&manifest.name);
|
||||
fs::create_dir_all(modules_dir)
|
||||
.with_context(|| format!("failed to create {}", modules_dir.display()))?;
|
||||
let dest = resolve_module_dir(&manifest.name, modules_dir)?;
|
||||
if dest.exists() {
|
||||
fs::remove_dir_all(&dest)
|
||||
.with_context(|| format!("failed to remove existing module at {}", dest.display()))?;
|
||||
|
|
@ -83,7 +146,7 @@ pub fn install_from_local(
|
|||
|
||||
/// Remove a module directory from `modules_dir`.
|
||||
pub fn remove_module(name: &str, modules_dir: &Path) -> Result<()> {
|
||||
let module_dir = modules_dir.join(name);
|
||||
let module_dir = resolve_module_dir(name, modules_dir)?;
|
||||
if !module_dir.exists() {
|
||||
bail!("bread: module '{}' is not installed", name);
|
||||
}
|
||||
|
|
@ -115,7 +178,8 @@ pub fn list_modules(modules_dir: &Path) -> Result<Vec<ModuleManifest>> {
|
|||
|
||||
/// Read a module manifest by name.
|
||||
pub fn read_module_manifest(name: &str, modules_dir: &Path) -> Result<ModuleManifest> {
|
||||
let manifest_path = modules_dir.join(name).join("bread.module.toml");
|
||||
let module_dir = resolve_module_dir(name, modules_dir)?;
|
||||
let manifest_path = module_dir.join("bread.module.toml");
|
||||
if !manifest_path.exists() {
|
||||
bail!("bread: module '{}' is not installed", name);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -103,6 +103,63 @@ fn list_reads_manifests_from_disk() {
|
|||
assert_eq!(modules[1].name, "beta");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remove_rejects_path_traversal_name() {
|
||||
let modules_tmp = TempDir::new().unwrap();
|
||||
// A sibling directory outside modules_dir that an attacker would want to delete.
|
||||
let victim_parent = modules_tmp.path().parent().unwrap();
|
||||
let victim = victim_parent.join("victim-dir");
|
||||
fs::create_dir_all(&victim).unwrap();
|
||||
fs::write(victim.join("keepme.txt"), "do not delete").unwrap();
|
||||
|
||||
let result = modules_mgmt::remove_module("../victim-dir", modules_tmp.path());
|
||||
assert!(result.is_err(), "path traversal name must be rejected");
|
||||
assert!(victim.join("keepme.txt").exists(), "victim dir must survive");
|
||||
|
||||
let _ = fs::remove_dir_all(&victim);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remove_rejects_absolute_path_name() {
|
||||
let modules_tmp = TempDir::new().unwrap();
|
||||
let result = modules_mgmt::remove_module("/etc/passwd", modules_tmp.path());
|
||||
assert!(result.is_err(), "absolute path name must be rejected");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remove_rejects_name_with_slash() {
|
||||
let modules_tmp = TempDir::new().unwrap();
|
||||
make_module_dir(modules_tmp.path(), "alpha", "1.0.0");
|
||||
let result = modules_mgmt::remove_module("alpha/../alpha", modules_tmp.path());
|
||||
assert!(result.is_err(), "name containing a slash must be rejected");
|
||||
// Original module must be untouched.
|
||||
assert!(modules_tmp.path().join("alpha").exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn install_rejects_manifest_with_path_traversal_name() {
|
||||
let src_tmp = TempDir::new().unwrap();
|
||||
let modules_tmp = TempDir::new().unwrap();
|
||||
|
||||
// Craft a manifest whose `name` field is a traversal attempt.
|
||||
let manifest = r#"name = "../evil"
|
||||
version = "1.0.0"
|
||||
description = "malicious"
|
||||
author = "attacker"
|
||||
source = "/tmp/test"
|
||||
installed_at = ""
|
||||
"#;
|
||||
fs::write(src_tmp.path().join("bread.module.toml"), manifest).unwrap();
|
||||
fs::write(src_tmp.path().join("init.lua"), "-- evil\n").unwrap();
|
||||
|
||||
let result =
|
||||
modules_mgmt::install_from_local(src_tmp.path(), "test:evil", modules_tmp.path());
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"install must reject a manifest name containing path traversal"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn manifest_written_correctly_on_install() {
|
||||
let src_tmp = TempDir::new().unwrap();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue