Skip to content

Commit c9eae3d

Browse files
authored
SEC-010: Bound remote SSH output (#284)
## Summary Bound hostile SSH and remote-bootstrap output before it can cause unbounded memory retention or log amplification. The patch checks raw line and stream bytes before retained growth; separately caps protocol records, retained protocol bytes, diagnostic lines, redacted stderr tails, and tunnel stderr; and preserves the complete successful `listdir` envelope of `DIR` + 2,000 entries + `LIST-DONE`. Repository history makes lifecycle ownership part of the security boundary. Remote SSH (`3c234437`) deliberately uses a durable remote Goose daemon behind a disposable local loopback tunnel; later generation/incarnation and log-drain hardening (`baee7994`) established stale-work rejection, resumable reconnects, and native-speed bounded draining as constraints. This fix therefore leaves daemon lifetime and reconnect policy unchanged. Output readers only report bounded-read failures; the owner of the Tokio `Child` exclusively terminates and reaps, and reader tasks finish before lifecycle/reconnect disposition. The new established-supervisor regression exercises that same owner path after readiness. Remote SSH remains experiment-gated. The original feature excludes Windows remote hosts; that is distinct from using a supported Windows local Berd client. No current architectural law directly governs remote subprocess output (`LAWS/README.md`, `LAWS/AGENTS.md`, `LAWS/CHAT.md`). The registry's older generation-owned numeric-PID teardown for disconnect/app exit is not represented as solved here. Replacing that authority with owner cancellation or an identity-safe handle is separate lifecycle-hardening scope. ### Related issue N/A — no public issue was opened because the underlying report is security-sensitive. No duplicate public issue or PR was found during the pre-submission search. ### Testing Validated resulting tree `33e0ac4a6e1629ad979fad825b27139e921c8d11`, now exact head `49739fc01bf2081e9d355436429b7bf5c391a351`, on `origin/main` `5c31189a4d55437888bb8d0f921efada827c7d86`; combined binary-diff SHA-256 `7c3b744ce3f41b0b527ef64f408861aa46a5259874cd401970709d9c5eb05c17`. - `bin/just tauri-fmt-check` — passed - `git diff --check origin/main...HEAD` — passed - `bin/just tauri-check` — passed - `bin/just clippy` — passed across all four lanes - `bin/just tauri-test` — passed - Readiness-timeout, pre-readiness stderr-overflow, and established-supervisor stderr-overflow owner kill/reap/task-disposition regressions — passed - Three timing-sensitive shell-contract tests — 15/15 individual repetitions passed on the prior behavior-equivalent head and 15/15 on clean `origin/main` - History-led Zeg Squad review — VSec Bot, Geeky, and Vuln Buster independently returned PASS with no blocker on exact head `49739fc0` and reproduced the tree, base, and binary-diff fingerprint - Copilot findings addressed in the resulting tree — invalid protocol UTF-8 has a distinct classification; task failures identify pipe and read/write operation; an already-exited child wins over an elapsed wait deadline; a completed stderr rejection wins over simultaneous child exit; and the tunnel reader field reuses its task alias. Focused regressions cover each case; fresh exact-head Copilot review recommends approval with 0 new comments The lifecycle runtime regressions are Unix-only because their fixtures use `sh`, `sleep`, and `yes`. Windows receives Rust compile/test-lane coverage; this does not claim an equivalent native Windows runtime process-lifecycle test. The fully concurrent app-crate suite is not clean on the baseline: the prior behavior-equivalent head produced 957 passed / 10 failed, while clean `origin/main` produced 945 passed / 12 failed. Both runs showed the same unrelated layout-default and provider-config failures plus timing-sensitive shell-contract failures; clean main had additional failures. This is baseline-noise evidence, not a claim that the fully concurrent suite is clean. Signed-off-by: Olabode Olaoke <olabode@squareup.com>
1 parent 183542e commit c9eae3d

4 files changed

Lines changed: 880 additions & 74 deletions

File tree

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
//! Allocation-bounded byte-to-line parsing for untrusted SSH output.
2+
//!
3+
//! `tokio::io::AsyncBufReadExt::lines` buffers until a newline before returning,
4+
//! so a remote peer controls the size of that allocation. This module reads
5+
//! fixed chunks and checks every byte budget before extending retained state.
6+
7+
use tokio::io::{AsyncRead, AsyncReadExt};
8+
9+
const READ_CHUNK_BYTES: usize = 8 * 1024;
10+
11+
#[derive(Clone, Copy, Debug)]
12+
pub(crate) struct LineLimits {
13+
pub max_line_bytes: usize,
14+
pub max_stream_bytes: usize,
15+
}
16+
17+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
18+
pub(crate) enum LineLimitKind {
19+
LineBytes,
20+
StreamBytes,
21+
ProtocolRecords,
22+
RetainedBytes,
23+
ProtocolEncoding,
24+
}
25+
26+
#[derive(Debug)]
27+
pub(crate) enum BoundedLineError {
28+
Io(std::io::Error),
29+
Limit(LineLimitKind),
30+
}
31+
32+
impl std::fmt::Display for BoundedLineError {
33+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
34+
match self {
35+
Self::Io(error) => write!(f, "read failed: {error}"),
36+
Self::Limit(LineLimitKind::LineBytes) => write!(f, "line byte limit exceeded"),
37+
Self::Limit(LineLimitKind::StreamBytes) => write!(f, "stream byte limit exceeded"),
38+
Self::Limit(LineLimitKind::ProtocolRecords) => {
39+
write!(f, "protocol record limit exceeded")
40+
}
41+
Self::Limit(LineLimitKind::RetainedBytes) => {
42+
write!(f, "retained byte limit exceeded")
43+
}
44+
Self::Limit(LineLimitKind::ProtocolEncoding) => {
45+
write!(f, "protocol record is not valid UTF-8")
46+
}
47+
}
48+
}
49+
}
50+
51+
impl std::error::Error for BoundedLineError {}
52+
53+
/// Read `reader` as newline-delimited bytes and call `on_line` for each line.
54+
///
55+
/// Lines exclude the LF delimiter and one optional preceding CR. A final
56+
/// unterminated line is delivered at EOF. Input is not converted to UTF-8 until
57+
/// after both the per-line and cumulative stream budgets have been enforced.
58+
pub(crate) async fn read_bounded_lines<R, F>(
59+
mut reader: R,
60+
limits: LineLimits,
61+
mut on_line: F,
62+
) -> Result<(), BoundedLineError>
63+
where
64+
R: AsyncRead + Unpin,
65+
F: FnMut(&[u8]) -> Result<(), BoundedLineError>,
66+
{
67+
read_bounded_lines_with_chunk_size(&mut reader, limits, READ_CHUNK_BYTES, &mut on_line).await
68+
}
69+
70+
async fn read_bounded_lines_with_chunk_size<R, F>(
71+
reader: &mut R,
72+
limits: LineLimits,
73+
chunk_bytes: usize,
74+
on_line: &mut F,
75+
) -> Result<(), BoundedLineError>
76+
where
77+
R: AsyncRead + Unpin,
78+
F: FnMut(&[u8]) -> Result<(), BoundedLineError>,
79+
{
80+
let mut chunk = vec![0_u8; chunk_bytes.clamp(1, READ_CHUNK_BYTES)];
81+
let mut line = Vec::with_capacity(limits.max_line_bytes.min(READ_CHUNK_BYTES));
82+
let mut stream_bytes = 0_usize;
83+
84+
loop {
85+
let read = reader
86+
.read(&mut chunk)
87+
.await
88+
.map_err(BoundedLineError::Io)?;
89+
if read == 0 {
90+
if !line.is_empty() {
91+
on_line(strip_cr(&line))?;
92+
}
93+
return Ok(());
94+
}
95+
96+
stream_bytes = stream_bytes
97+
.checked_add(read)
98+
.filter(|total| *total <= limits.max_stream_bytes)
99+
.ok_or(BoundedLineError::Limit(LineLimitKind::StreamBytes))?;
100+
101+
let mut start = 0;
102+
for (index, byte) in chunk[..read].iter().enumerate() {
103+
if *byte != b'\n' {
104+
continue;
105+
}
106+
extend_line(&mut line, &chunk[start..index], limits.max_line_bytes)?;
107+
on_line(strip_cr(&line))?;
108+
line.clear();
109+
start = index + 1;
110+
}
111+
extend_line(&mut line, &chunk[start..read], limits.max_line_bytes)?;
112+
}
113+
}
114+
115+
fn extend_line(
116+
line: &mut Vec<u8>,
117+
bytes: &[u8],
118+
max_line_bytes: usize,
119+
) -> Result<(), BoundedLineError> {
120+
line.len()
121+
.checked_add(bytes.len())
122+
.filter(|total| *total <= max_line_bytes)
123+
.ok_or(BoundedLineError::Limit(LineLimitKind::LineBytes))?;
124+
line.extend_from_slice(bytes);
125+
Ok(())
126+
}
127+
128+
fn strip_cr(line: &[u8]) -> &[u8] {
129+
line.strip_suffix(b"\r").unwrap_or(line)
130+
}
131+
132+
#[cfg(test)]
133+
mod tests {
134+
use super::*;
135+
136+
#[tokio::test]
137+
async fn rejects_oversized_unterminated_line_before_growing_past_limit() {
138+
let input = vec![b'x'; 65];
139+
let mut delivered = Vec::new();
140+
let error = read_bounded_lines(
141+
input.as_slice(),
142+
LineLimits {
143+
max_line_bytes: 64,
144+
max_stream_bytes: 1_024,
145+
},
146+
|line| {
147+
delivered.push(line.to_vec());
148+
Ok(())
149+
},
150+
)
151+
.await
152+
.unwrap_err();
153+
154+
assert!(matches!(
155+
error,
156+
BoundedLineError::Limit(LineLimitKind::LineBytes)
157+
));
158+
assert!(delivered.is_empty());
159+
}
160+
161+
#[tokio::test]
162+
async fn rejects_many_small_lines_at_the_cumulative_stream_limit() {
163+
let input = b"a\nb\nc\nd\n";
164+
let mut delivered = 0;
165+
let error = read_bounded_lines(
166+
input.as_slice(),
167+
LineLimits {
168+
max_line_bytes: 8,
169+
max_stream_bytes: input.len() - 1,
170+
},
171+
|_| {
172+
delivered += 1;
173+
Ok(())
174+
},
175+
)
176+
.await
177+
.unwrap_err();
178+
179+
assert!(matches!(
180+
error,
181+
BoundedLineError::Limit(LineLimitKind::StreamBytes)
182+
));
183+
assert_eq!(delivered, 0, "the single read is rejected before parsing");
184+
}
185+
186+
#[tokio::test]
187+
async fn preserves_utf8_split_across_fixed_reads() {
188+
let input = b"a\xc3\xa9\n";
189+
let mut reader = input.as_slice();
190+
let mut lines = Vec::new();
191+
read_bounded_lines_with_chunk_size(
192+
&mut reader,
193+
LineLimits {
194+
max_line_bytes: 8,
195+
max_stream_bytes: 16,
196+
},
197+
2,
198+
&mut |line| {
199+
lines.push(String::from_utf8(line.to_vec()).unwrap());
200+
Ok(())
201+
},
202+
)
203+
.await
204+
.unwrap();
205+
206+
assert_eq!(lines, ["aé"]);
207+
}
208+
}

0 commit comments

Comments
 (0)