fix(security): close S3 (arg-injection), S4 (presence-name), S1 (panic-slice)
Three findings from the first security pass: - S3 (Medium): the peer-supplied screen-share ticket was passed to pixelpass as the first positional CLI arg with no end-of-options guard, so a ticket starting with `-`/`--` could be reinterpreted as a flag (argument injection). New pure `viewer_args()` puts flags first, then a `--` guard, then the ticket positionally; spawn_viewer uses it. +2 tests. - S4 (Medium): peer presence display-names (gossip `Announce`, untrusted and spoofable) were rendered unsanitized/unbounded, unlike the chat path. New `sanitize::sanitize_name` strips bidi/zero-width format chars + control chars, collapses whitespace, and caps at 48 chars; applied at the gossip ingest point so every consumer gets a safe value. +4 tests. - S1 (Low): `&id[..8]` byte-slices could panic on a short/non-ASCII id. New panic-free `short_id()` (char-based take) replaces both slices. +1 test. 158 lib tests (was 151), clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+38
-3
@@ -93,6 +93,21 @@ fn json_u32(v: &serde_json::Value, key: &str) -> u32 {
|
||||
v.get(key).and_then(|x| x.as_u64()).unwrap_or(0) as u32
|
||||
}
|
||||
|
||||
/// Build the argv for a pixelpass *viewer*. The `ticket` is peer-supplied (it
|
||||
/// rides gossip presence, which is untrusted and spoofable), so flags come first
|
||||
/// and the ticket is passed as a positional **after a `--` end-of-options
|
||||
/// guard**. That guard means a ticket beginning with `-`/`--` can never be
|
||||
/// reinterpreted as a pixelpass flag — closing the argument-injection vector
|
||||
/// (security finding S3). Pure: no I/O.
|
||||
pub fn viewer_args(ticket: &str) -> Vec<String> {
|
||||
vec![
|
||||
"--output".to_string(),
|
||||
"json".to_string(),
|
||||
"--".to_string(),
|
||||
ticket.to_string(),
|
||||
]
|
||||
}
|
||||
|
||||
/// Resolve the pixelpass binary: an explicit config override (used only if it
|
||||
/// points at an existing file), otherwise the first `pixelpass` found on
|
||||
/// `$PATH`. `None` means it isn't installed — a normal, handled state. An
|
||||
@@ -172,9 +187,7 @@ pub async fn spawn_host(bin: &Path) -> std::io::Result<(Child, String)> {
|
||||
/// player window closes (its tunnel ends).
|
||||
pub async fn spawn_viewer(bin: &Path, ticket: &str) -> std::io::Result<Child> {
|
||||
let mut child = Command::new(bin)
|
||||
.arg(ticket)
|
||||
.arg("--output")
|
||||
.arg("json")
|
||||
.args(viewer_args(ticket))
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null())
|
||||
@@ -305,6 +318,28 @@ fn spawn_player(bin: &str, args: &[&str], url: &str) -> std::io::Result<Child> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn viewer_args_guard_neutralizes_flag_like_ticket() {
|
||||
// A malicious "ticket" that looks like a flag must end up positional,
|
||||
// i.e. after the `--` end-of-options guard, never parsed as a flag.
|
||||
let args = viewer_args("--malicious-flag");
|
||||
assert_eq!(args.last().unwrap(), "--malicious-flag", "ticket is last");
|
||||
let guard = args.iter().position(|a| a == "--").expect("`--` guard present");
|
||||
let ticket = args.len() - 1;
|
||||
assert!(guard < ticket, "ticket must follow the `--` guard");
|
||||
// The real flags are parsed before the guard.
|
||||
let out = args.iter().position(|a| a == "--output").unwrap();
|
||||
assert!(out < guard, "flags precede the `--` guard");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn viewer_args_ordinary_ticket() {
|
||||
assert_eq!(
|
||||
viewer_args("abc123"),
|
||||
vec!["--output", "json", "--", "abc123"]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_ticket() {
|
||||
assert_eq!(
|
||||
|
||||
Reference in New Issue
Block a user