diff --git a/crates/aprs-cli/src/main.rs b/crates/aprs-cli/src/main.rs index f121fa4..47c2378 100644 --- a/crates/aprs-cli/src/main.rs +++ b/crates/aprs-cli/src/main.rs @@ -186,7 +186,13 @@ fn parse_args(args: impl IntoIterator) -> Result return Err(usage()), - _ if arg.starts_with('-') => return Err(format!("unknown option: {arg}\n{}", usage())), + _ if arg.starts_with('-') => { + return Err(format!( + "unknown option: {}\n{}", + diagnostic_value(&arg), + usage() + )); + } _ => { if options.input_path.replace(arg).is_some() { return Err(format!("multiple input paths supplied\n{}", usage())); @@ -203,7 +209,11 @@ fn parse_fail_on(value: &str) -> Result { "none" => Ok(FailOn::None), "malformed" => Ok(FailOn::Malformed), "rejected" => Ok(FailOn::Rejected), - _ => Err(format!("invalid --fail-on value: {value}\n{}", usage())), + _ => Err(format!( + "invalid --fail-on value: {}\n{}", + diagnostic_value(value), + usage() + )), } } @@ -224,11 +234,14 @@ fn matches_filter(options: &CliOptions, semantic: &str) -> bool { fn read_input(path: Option<&str>) -> Result, String> { match path { - Some(path) => read_all_with_limit( - File::open(path).map_err(|err| format!("failed to open {path}: {err}"))?, - libaprs_engine::DEFAULT_TRANSPORT_READ_LIMIT, - ) - .map_err(|err| format!("failed to read {path}: {err}")), + Some(path) => { + let display_path = diagnostic_value(path); + read_all_with_limit( + File::open(path).map_err(|err| format!("failed to open {display_path}: {err}"))?, + libaprs_engine::DEFAULT_TRANSPORT_READ_LIMIT, + ) + .map_err(|err| format!("failed to read {display_path}: {err}")) + } None => read_all_with_limit(io::stdin(), libaprs_engine::DEFAULT_TRANSPORT_READ_LIMIT) .map_err(|err| format!("failed to read stdin: {err}")), } @@ -381,6 +394,10 @@ fn lossy(bytes: &[u8]) -> String { String::from_utf8_lossy(bytes).into_owned() } +fn diagnostic_value(value: &str) -> String { + format!("{value:?}") +} + fn usage() -> String { "usage: aprs-cli [parse|validate|stats|explain|replay|support-matrix] [--json] [--permissive] [--explain] [--summary] [--filter SEMANTIC] [--fail-on none|malformed|rejected] [PATH]".to_string() } diff --git a/crates/aprs-cli/tests/cli.rs b/crates/aprs-cli/tests/cli.rs index 2f5aaf7..5ace1aa 100644 --- a/crates/aprs-cli/tests/cli.rs +++ b/crates/aprs-cli/tests/cli.rs @@ -110,6 +110,30 @@ fn cli_rejects_oversized_file_input() { let _ = std::fs::remove_file(path); } +#[test] +fn cli_diagnostic_errors_escape_control_characters() { + let binary = env!("CARGO_BIN_EXE_aprs-cli"); + let output = Command::new(binary) + .arg("--bad\noption") + .output() + .expect("CLI should run"); + + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8(output.stderr).expect("stderr should be utf8"); + assert!(stderr.contains("unknown option: \"--bad\\noption\"")); + assert!(!stderr.contains("unknown option: --bad\noption")); + + let output = Command::new(binary) + .args(["--fail-on", "bad\rvalue"]) + .output() + .expect("CLI should run"); + + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8(output.stderr).expect("stderr should be utf8"); + assert!(stderr.contains("invalid --fail-on value: \"bad\\rvalue\"")); + assert!(!stderr.contains("invalid --fail-on value: bad\rvalue")); +} + #[test] fn cli_validate_command_reports_validity() { let binary = env!("CARGO_BIN_EXE_aprs-cli"); diff --git a/crates/aprs-transport-aprs-is/examples/reader.rs b/crates/aprs-transport-aprs-is/examples/reader.rs index ee1528c..d2675fd 100644 --- a/crates/aprs-transport-aprs-is/examples/reader.rs +++ b/crates/aprs-transport-aprs-is/examples/reader.rs @@ -8,7 +8,7 @@ fn main() -> Result<(), Box> { software: "libaprs-engine 1.1.0", filter: Some("r/49/-72/50"), }; - assert!(login.line()?.ends_with("\r\n")); + assert!(login.profile_line()?.ends_with("\r\n")); let input = std::io::Cursor::new(b"# aprs-is banner\r\nN0CALL>APRS:>aprs-is\n"); for packet_bytes in read_packet_lines_from_reader(input)? { diff --git a/crates/aprs-transport-aprs-is/examples/session_reconnect.rs b/crates/aprs-transport-aprs-is/examples/session_reconnect.rs index 3334d39..6ad7bb1 100644 --- a/crates/aprs-transport-aprs-is/examples/session_reconnect.rs +++ b/crates/aprs-transport-aprs-is/examples/session_reconnect.rs @@ -51,7 +51,7 @@ fn run_receive_session( where R: io::Read, { - let login_line = login.line().map_err(io::Error::other)?; + let login_line = login.profile_line().map_err(io::Error::other)?; for attempt in 0..plan.max_attempts { match connect(attempt) { diff --git a/crates/aprs-transport-aprs-is/src/lib.rs b/crates/aprs-transport-aprs-is/src/lib.rs index 4ba2099..1383b4d 100644 --- a/crates/aprs-transport-aprs-is/src/lib.rs +++ b/crates/aprs-transport-aprs-is/src/lib.rs @@ -29,8 +29,9 @@ pub struct AprsIsLogin<'a> { impl AprsIsLogin<'_> { /// Builds the APRS-IS login line terminated with CRLF. /// - /// Values containing CR or LF are rejected to prevent line injection into - /// the APRS-IS control stream. + /// Values containing CR, LF, or other ASCII control bytes are rejected to + /// prevent control-line injection. Prefer [`Self::profile_line`] when + /// fields may come from untrusted input. pub fn line(&self) -> Result { validate_login_field("callsign", self.callsign)?; validate_login_field("software", self.software)?; @@ -78,7 +79,7 @@ impl AprsIsLogin<'_> { /// APRS-IS login line validation error. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum AprsIsLoginError { - /// A login field contains CR or LF and would inject another line. + /// A login field contains CR, LF, or another ASCII control byte. LineInjection { field: &'static str }, } @@ -95,9 +96,10 @@ impl AprsIsLoginError { impl std::fmt::Display for AprsIsLoginError { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::LineInjection { field } => { - write!(formatter, "APRS-IS login field contains CR or LF: {field}") - } + Self::LineInjection { field } => write!( + formatter, + "APRS-IS login field contains CR, LF, or control byte: {field}" + ), } } } @@ -107,7 +109,7 @@ impl std::error::Error for AprsIsLoginError {} /// APRS-IS profile validation error. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum AprsIsProfileError { - /// A profile field contains CR or LF and would inject another line. + /// A profile field contains CR, LF, or another ASCII control byte. LineInjection { field: &'static str }, /// A callsign does not fit the conservative AX.25-like login shape. InvalidCallsign, @@ -136,7 +138,7 @@ impl std::fmt::Display for AprsIsProfileError { Self::LineInjection { field } => { write!( formatter, - "APRS-IS profile field contains CR or LF: {field}" + "APRS-IS profile field contains CR, LF, or control byte: {field}" ) } Self::InvalidCallsign => formatter.write_str("APRS-IS profile callsign is invalid"), @@ -351,14 +353,14 @@ fn read_all(reader: impl Read, max_bytes: usize) -> io::Result> { } fn validate_profile_field(field: &'static str, value: &str) -> Result<(), AprsIsProfileError> { - if value.as_bytes().contains(&b'\r') || value.as_bytes().contains(&b'\n') { + if value.as_bytes().iter().any(u8::is_ascii_control) { return Err(AprsIsProfileError::LineInjection { field }); } Ok(()) } fn validate_login_field(field: &'static str, value: &str) -> Result<(), AprsIsLoginError> { - if value.as_bytes().contains(&b'\r') || value.as_bytes().contains(&b'\n') { + if value.as_bytes().iter().any(u8::is_ascii_control) { return Err(AprsIsLoginError::LineInjection { field }); } Ok(()) diff --git a/crates/aprs-transport-aprs-is/tests/aprs_is_transport.rs b/crates/aprs-transport-aprs-is/tests/aprs_is_transport.rs index b9c4943..26518a0 100644 --- a/crates/aprs-transport-aprs-is/tests/aprs_is_transport.rs +++ b/crates/aprs-transport-aprs-is/tests/aprs_is_transport.rs @@ -41,6 +41,12 @@ fn aprs_is_login_line_rejects_line_injection() { software: "libaprs-engine 1.1.0", filter: Some("r/49/-72/50\r\nbad"), }, + AprsIsLogin { + callsign: "N0CALL", + passcode: -1, + software: "libaprs-engine\t1.1.0", + filter: None, + }, ]; for login in cases { @@ -75,6 +81,20 @@ fn aprs_is_profile_login_requires_uppercase_callsign_and_valid_filter() { lowercase.profile_line().expect_err("lowercase callsign"), AprsIsProfileError::LowercaseCallsign ); + + let control_byte = AprsIsLogin { + callsign: "N0CALL-7", + passcode: -1, + software: "libaprs-engine\u{1b}", + filter: None, + }; + + assert_eq!( + control_byte + .profile_line() + .expect_err("control bytes fail closed"), + AprsIsProfileError::LineInjection { field: "software" } + ); } #[test] diff --git a/crates/aprs-transport-tcp/src/lib.rs b/crates/aprs-transport-tcp/src/lib.rs index e16edc1..2c23615 100644 --- a/crates/aprs-transport-tcp/src/lib.rs +++ b/crates/aprs-transport-tcp/src/lib.rs @@ -14,6 +14,12 @@ use libaprs_engine::{ read_all_with_limit, LineTransport, DEFAULT_TRANSPORT_READ_LIMIT, MAX_PACKET_LEN, }; +/// Default timeout for establishing a TCP connection. +pub const DEFAULT_TCP_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); + +/// Default timeout for reads after a TCP connection is established. +pub const DEFAULT_TCP_READ_TIMEOUT: Duration = Duration::from_secs(10); + /// TCP read options owned by the caller. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct TcpReadOptions { @@ -51,8 +57,8 @@ impl TcpReadOptions { impl Default for TcpReadOptions { fn default() -> Self { Self { - connect_timeout: None, - read_timeout: None, + connect_timeout: Some(DEFAULT_TCP_CONNECT_TIMEOUT), + read_timeout: Some(DEFAULT_TCP_READ_TIMEOUT), max_bytes: DEFAULT_TRANSPORT_READ_LIMIT, } } diff --git a/crates/aprs-transport-tcp/tests/tcp_transport.rs b/crates/aprs-transport-tcp/tests/tcp_transport.rs index 271612d..858484d 100644 --- a/crates/aprs-transport-tcp/tests/tcp_transport.rs +++ b/crates/aprs-transport-tcp/tests/tcp_transport.rs @@ -5,7 +5,8 @@ use std::time::Duration; use aprs_transport_tcp::{ read_packet_lines_from_reader, read_packet_lines_from_reader_with_limit, - read_packet_lines_from_tcp_addr_with_options, TcpReadOptions, + read_packet_lines_from_tcp_addr_with_options, TcpReadOptions, DEFAULT_TCP_CONNECT_TIMEOUT, + DEFAULT_TCP_READ_TIMEOUT, }; #[test] @@ -71,6 +72,14 @@ fn tcp_addr_helper_applies_caller_owned_read_timeout() { server.join().expect("server thread"); } +#[test] +fn tcp_options_default_uses_finite_timeouts() { + let options = TcpReadOptions::default(); + + assert_eq!(options.connect_timeout, Some(DEFAULT_TCP_CONNECT_TIMEOUT)); + assert_eq!(options.read_timeout, Some(DEFAULT_TCP_READ_TIMEOUT)); +} + #[test] fn tcp_options_builder_api_remains_source_compatible() { let options = TcpReadOptions::default() diff --git a/crates/libaprs-engine/src/lib.rs b/crates/libaprs-engine/src/lib.rs index ec35c70..9228178 100644 --- a/crates/libaprs-engine/src/lib.rs +++ b/crates/libaprs-engine/src/lib.rs @@ -451,7 +451,7 @@ const TRANSPORT_SUPPORT: &[TransportSupport] = &[ crate_name: "aprs-transport-tcp", boundary: "blocking TCP or Read packet streams", status: SupportStatus::Supported, - notes: "caller owns socket timeouts and reconnect behavior", + notes: "finite default socket timeouts; caller owns reconnect behavior", }, TransportSupport { crate_name: "aprs-transport-aprs-is", diff --git a/docs/release-notes-v3.0.0-rc.1.md b/docs/release-notes-v3.0.0-rc.1.md index 1cc5d4b..fea2755 100644 --- a/docs/release-notes-v3.0.0-rc.1.md +++ b/docs/release-notes-v3.0.0-rc.1.md @@ -33,6 +33,8 @@ source, semver, or behavior regression before final `3.0.0`. - Post-publication downstream smoke must regenerate its lockfile from crates.io so checksum evidence matches the published RC crates. +- The RC fix-forward audit summary is tracked in + [v3.0.0-rc.1 Security Audit Summary](security-audit-v3.0.0-rc.1.md). ## Release Gates diff --git a/docs/security-audit-v3.0.0-rc.1.md b/docs/security-audit-v3.0.0-rc.1.md new file mode 100644 index 0000000..ccc6321 --- /dev/null +++ b/docs/security-audit-v3.0.0-rc.1.md @@ -0,0 +1,45 @@ +# v3.0.0-rc.1 Security Audit Summary + +## BLUF + +- Repo-wide source security audit was repeated during the `v3.0.0-rc.1` + fix-forward pass. +- No surviving reportable security findings remain after the fixes in commit + `f5179df`. +- The fixes harden CLI diagnostics, APRS-IS login field validation, and TCP + default timeout behavior. +- Verification passed with formatting, focused tests, full workspace tests, + clippy with warnings denied, and docs verification. +- Merge should proceed only through PR checks and the normal release gates. + +## Closed Findings + +- CLI diagnostic control-character injection: user-controlled option, fail-on, + and path values are now escaped before display. +- APRS-IS login/profile ASCII-control injection: login fields now reject all + ASCII control bytes, not only CR and LF. +- TCP default indefinite blocking: default TCP connect and read options now use + finite timeouts; callers can explicitly opt into blocking behavior by setting + timeout fields to `None`. + +## Verification Evidence + +- `cargo fmt --check` +- `cargo test -p aprs-cli -p aprs-transport-aprs-is` +- `cargo test -p aprs-transport-tcp -p libaprs-engine` +- `cargo test --workspace` +- `cargo clippy --workspace --all-targets --all-features -- -D warnings` +- `scripts/verify-docs.sh` + +All commands passed. `scripts/verify-docs.sh` also passed after this summary was +added and linked. + +## PR Task List + +- [x] Add tracked security-audit summary for release evidence. +- [ ] Push `codex/repo-security-audit-fix-forward`. +- [ ] Open PR to `main`. +- [ ] Wait for CI, security, merge-gate, and supply-chain checks. +- [ ] Fix forward on the same branch if any check fails. +- [ ] Merge only when all checks and secure-review findings are clean. +- [ ] Rerun release verification from fresh `main` after merge. diff --git a/docs/security.md b/docs/security.md index 6bfa059..78c5812 100644 --- a/docs/security.md +++ b/docs/security.md @@ -28,6 +28,9 @@ conservative address metadata needed to establish safe structure. See [Threat Model](threat-model.md) for the per-crate untrusted boundaries, primary abuse cases, and required controls. +For the current release-candidate audit evidence, see +[v3.0.0-rc.1 Security Audit Summary](security-audit-v3.0.0-rc.1.md). + The current trusted boundary is: ```text diff --git a/docs/threat-model.md b/docs/threat-model.md index 4c9581a..40493b6 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -41,7 +41,7 @@ no lossy text conversion before the codec boundary. | `aprs-transport-file` | packet files and reader-backed file input | oversized files, overlong lines, invalid UTF-8 | bounded reads, per-packet limits, byte-preserving records | | `aprs-transport-file-watch` | appended file bytes | unbounded append growth, partial records | appended-byte limits, packet-line limits, caller-owned polling policy | | `aprs-transport-corpus` | corpus directories and files | private data leakage, oversized corpus files, unstable ordering | bounded file reads, stable ordering, fuzz corpus guard for regression seeds | -| `aprs-transport-tcp` | TCP streams and generic readers | stalled streams, overlong lines, retry storms | caller-owned timeouts/reconnects, bounded reads, packet-line limits | +| `aprs-transport-tcp` | TCP streams and generic readers | stalled streams, overlong lines, retry storms | finite default timeouts, caller-owned reconnects, bounded reads, packet-line limits | | `aprs-transport-aprs-is` | APRS-IS server lines and login filters | line injection, server comments, oversized lines | CRLF-safe login construction, comment filtering, line limits | | `aprs-transport-serial` | serial readers | partial records, invalid bytes, oversized batches | caller-owned serial configuration, bounded reads, packet-line limits | | `aprs-transport-http` | HTTP body bytes | oversized request bodies, malformed line framing | body-size limits, packet-line limits, no text normalization | diff --git a/docs/transports.md b/docs/transports.md index 87d8f3d..ceccc2f 100644 --- a/docs/transports.md +++ b/docs/transports.md @@ -20,7 +20,7 @@ parser, preserving the protocol-first boundary. | Crate | Boundary | Primary use | Security note | | --- | --- | --- | --- | | `aprs-transport-file` | Newline-separated byte buffers and files | Offline logs, stdin-style files | Bounded path helpers reject oversized batches and packet lines | -| `aprs-transport-tcp` | Blocking `Read` and TCP address helpers | TCP-connected packet streams | Reader helpers reject oversized batches and packet lines; `TcpReadOptions` keeps connection and read timeouts caller-owned | +| `aprs-transport-tcp` | Blocking `Read` and TCP address helpers | TCP-connected packet streams | Reader helpers reject oversized batches and packet lines; `TcpReadOptions` applies finite default timeouts that callers can override | | `aprs-transport-aprs-is` | APRS-IS login, filters, q constructs, and comment filtering | APRS-IS clients | Server comment lines are filtered before parsing and packet lines are bounded | | `aprs-transport-kiss` | KISS frame encoding and decoding | TNC, serial, or TCP KISS streams | Invalid escapes and oversized decoded payloads fail closed | | `aprs-transport-serial` | Serial-like byte readers | TNC serial pipelines | Reader helpers reject oversized batches and packet lines; serial configuration stays application-owned | @@ -101,9 +101,9 @@ silently weakening packet parsing. For reconnecting services, keep session ownership in the application. The compile-tested `crates/aprs-transport-aprs-is/examples/session_reconnect.rs` -example shows an APRS-IS login line, bounded reader helper, retry loop, backoff, -and `Engine::process_event()` integration without adding networking behavior to -the parser core. +example shows a profile-validated APRS-IS login line, bounded reader helper, +retry loop, backoff, and `Engine::process_event()` integration without adding +networking behavior to the parser core. ## TCP With Caller-Owned Timeouts diff --git a/examples/downstream-smoke/src/main.rs b/examples/downstream-smoke/src/main.rs index f1bfb28..908df40 100644 --- a/examples/downstream-smoke/src/main.rs +++ b/examples/downstream-smoke/src/main.rs @@ -30,7 +30,7 @@ fn main() -> Result<(), Box> { software: "libaprs-engine-downstream-smoke 3.0.0-rc.1", filter: Some("r/49/-72/50"), }; - assert!(aprs_is_login.line()?.ends_with("\r\n")); + assert!(aprs_is_login.profile_line()?.ends_with("\r\n")); let aprs_is_packets = read_aprs_is_packet_lines(b"# banner\nN0CALL>APRS:>aprs-is\n"); assert_eq!(aprs_is_packets.len(), 1);