Skip to main content

max / makenotwork

server: structural scan fail-closed honesty, spool perms, oauth client binding (ultra-fuzz Run 2 Security) structural.rs (3 fixes): - The parse-error arm returned Skip, making the module's documented FailClosed ERROR_POLICY unreachable. It now fails closed (Error → held for review) ONLY when the file carries executable magic (PE/ELF/Mach-O, incl. byte-swapped and fat) but goblin can't parse it — a genuine non-binary download (zip/pdf/audio) still Skips, so legitimate uploads aren't held. - Fat/universal Mach-O analysis walked only the first arch slice; it now unions symbols across every Mach-O slice (a malicious arch can't hide behind a benign first one). - A Mach-O import-table parse error was swallowed to an empty symbol set (→ Pass); it now surfaces as Error (fail closed). spool.rs: scan spool tempfiles are created 0o600 and the spool dir is set 0o700, so the (possibly paid/private) upload bytes aren't world-readable during the scan window. oauth.rs: the refresh-token grant is now bound to its client_id (the auth-code path already was), so a stolen refresh token isn't redeemable under any client. is_localhost_redirect parses strictly (http scheme, exact loopback host, explicit non-zero port, no embedded credentials) instead of prefix-matching. Adds structural unit tests for the magic-gated fail-closed behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-23 17:04 UTC
Commit: cb8adae05c51e145fb2aa4bd8f3502b7aab20098
Parent: 8fb3d76
3 files changed, +130 insertions, -23 deletions
M server/src/routes/oauth.rs +28 -8
@@ -198,15 +198,25 @@ async fn issue_authorization_code(
198 198 ///
199 199 /// Non-localhost URIs must be registered in the app's `redirect_uris` column.
200 200 fn is_localhost_redirect(uri: &str) -> bool {
201 - for prefix in ["http://127.0.0.1:", "http://[::1]:", "http://localhost:"] {
202 - if let Some(rest) = uri.strip_prefix(prefix)
203 - && let Some(port_str) = rest.split('/').next()
204 - && port_str.parse::<u16>().is_ok()
205 - {
206 - return true;
207 - }
201 + // Parse strictly rather than prefix-match: require the http scheme, a host
202 + // exactly in the loopback set, an explicit non-zero port, and NO embedded
203 + // credentials (Run #2 Security MINOR — the old prefix check accepted port 0
204 + // and didn't reject userinfo). The host pin is the load-bearing property: a
205 + // native app's loopback listener is the only thing that can receive the code.
206 + let Ok(parsed) = url::Url::parse(uri) else {
207 + return false;
208 + };
209 + if parsed.scheme() != "http" {
210 + return false;
211 + }
212 + if !parsed.username().is_empty() || parsed.password().is_some() {
213 + return false;
208 214 }
209 - false
215 + match parsed.port() {
216 + Some(0) | None => return false,
217 + Some(_) => {}
218 + }
219 + matches!(parsed.host_str(), Some("127.0.0.1") | Some("[::1]") | Some("::1") | Some("localhost"))
210 220 }
211 221
212 222 async fn validate_redirect_uri(pool: &sqlx::PgPool, app_id: db::SyncAppId, uri: &str) -> Result<bool> {
@@ -749,6 +759,16 @@ async fn token_refresh(state: &AppState, secret: &str, req: TokenRequest) -> Res
749 759 db::oauth::RefreshRotateOutcome::Invalid => return Ok(oauth_error("invalid_grant")),
750 760 };
751 761
762 + // Bind the grant to its client: the presented client_id must own this refresh
763 + // lineage. The auth-code path enforces this; the refresh path did not, so a
764 + // stolen refresh token was redeemable under any client_id (Run #2 Security
765 + // MINOR). The token is already rotated above, so a mismatch leaves the stolen
766 + // token spent and the legit client's next refresh trips reuse-detection.
767 + let client_app = db::synckit::get_sync_app_by_api_key(&state.db, &req.client_id).await?;
768 + if client_app.map(|a| a.id) != Some(consumed.app_id) {
769 + return Ok(oauth_error("invalid_grant"));
770 + }
771 +
752 772 // Revocation + liveness: the same gates the access-token extractor applies,
753 773 // so password change / suspend / app-deactivate kill the refresh lineage.
754 774 let user = db::users::get_user_by_id(&state.db, consumed.user_id).await?;
M server/src/scanning/spool.rs +12
@@ -100,6 +100,14 @@ pub async fn download_into_tempfile(
100 100 .await
101 101 .map_err(|e| format!("create spool dir {}: {e}", spool_dir.display()))?;
102 102
103 + // Spooled uploads can be paid/private content; keep the dir owner-only so a
104 + // shared spool path can't be listed by other local users (Run #2 Security
105 + // MINOR). The per-file 0o600 below is the primary guard.
106 + {
107 + use std::os::unix::fs::PermissionsExt;
108 + let _ = tokio::fs::set_permissions(spool_dir, std::fs::Permissions::from_mode(0o700)).await;
109 + }
110 +
103 111 // `unique` (the scan job's UUID) makes the path collision-free even when two
104 112 // workers scan the same s3_key concurrently (an item and its version, or a
105 113 // re-queued job). Keying solely on pid+s3_key let the second `create_new`
@@ -109,9 +117,13 @@ pub async fn download_into_tempfile(
109 117 let suffix = s3_key.replace('/', "_");
110 118 let path = spool_dir.join(format!("scan-{}-{}-{}.tmp", std::process::id(), unique, suffix));
111 119
120 + // 0o600: the spooled file holds the (possibly paid/private) upload bytes for
121 + // the scan window; no other local user should be able to read it (Run #2
122 + // Security MINOR). `create_new` already blocks symlink pre-placement.
112 123 let mut file = OpenOptions::new()
113 124 .create_new(true)
114 125 .write(true)
126 + .mode(0o600)
115 127 .open(&path)
116 128 .await
117 129 .map_err(|e| format!("open spool tempfile {}: {e}", path.display()))?;
M server/src/scanning/structural.rs +90 -15
@@ -87,8 +87,21 @@ pub fn analyze_binary(data: &[u8], file_type: FileType) -> LayerResult {
87 87 detail: Some("Not an executable binary".to_string()),
88 88 }
89 89 }
90 + Err(_) if looks_like_executable(data) => {
91 + // The file CLAIMS to be a binary (executable magic header) but goblin
92 + // refused to parse it — an evasion signal, not "not applicable". Fail
93 + // closed for admin review, which is what this layer's ERROR_POLICY
94 + // already documents (Run #2 Security MINOR: the Skip arm made that
95 + // FailClosed posture unreachable from the buffered path).
96 + LayerResult {
97 + layer: "structural",
98 + verdict: LayerVerdict::Error,
99 + detail: Some("Executable magic header but unparseable binary".to_string()),
100 + }
101 + }
90 102 Err(_) => {
91 - // Not a recognized binary format — skip (not an error, just not applicable)
103 + // No executable magic — a genuine non-binary download (zip, pdf,
104 + // audio, sample pack). Structural analysis is not applicable; skip.
92 105 LayerResult {
93 106 layer: "structural",
94 107 verdict: LayerVerdict::Skip,
@@ -98,6 +111,25 @@ pub fn analyze_binary(data: &[u8], file_type: FileType) -> LayerResult {
98 111 }
99 112 }
100 113
114 + /// True if `data` begins with a known executable magic number (PE/ELF/Mach-O,
115 + /// including byte-swapped and fat/universal). Used so the layer fails closed
116 + /// ONLY when a file claims to be a binary but goblin can't parse it — a
117 + /// non-binary download has no such magic and is correctly skipped, so changing
118 + /// the parse-error arm doesn't hold every legitimate non-executable upload.
119 + fn looks_like_executable(data: &[u8]) -> bool {
120 + matches!(
121 + data,
122 + [0x4D, 0x5A, ..] // PE / DOS "MZ"
123 + | [0x7F, b'E', b'L', b'F', ..] // ELF
124 + | [0xFE, 0xED, 0xFA, 0xCE, ..] // Mach-O 32-bit, big-endian
125 + | [0xFE, 0xED, 0xFA, 0xCF, ..] // Mach-O 64-bit, big-endian
126 + | [0xCE, 0xFA, 0xED, 0xFE, ..] // Mach-O 32-bit, little-endian
127 + | [0xCF, 0xFA, 0xED, 0xFE, ..] // Mach-O 64-bit, little-endian
128 + | [0xCA, 0xFE, 0xBA, 0xBE, ..] // fat/universal (big-endian)
129 + | [0xBE, 0xBA, 0xFE, 0xCA, ..] // fat/universal (little-endian)
130 + )
131 + }
132 +
101 133 fn analyze_pe(pe: &goblin::pe::PE) -> LayerResult {
102 134 let import_names: Vec<&str> = pe.imports.iter().map(|i| i.name.as_ref()).collect();
103 135 let section_names: Vec<String> = pe
@@ -189,17 +221,26 @@ fn check_elf_warnings(symbol_names: &[&str]) -> Vec<String> {
189 221 }
190 222
191 223 fn analyze_mach(mach: &goblin::mach::Mach) -> LayerResult {
224 + // Collect symbols across EVERY Mach-O slice, not just the first — a fat
225 + // binary can hide a malicious arch behind a benign one (Run #2 Security
226 + // MINOR). An import-table parse error is surfaced as Error (fail closed)
227 + // rather than swallowed to an empty symbol set that would Pass.
192 228 let symbols: Vec<String> = match mach {
193 - goblin::mach::Mach::Binary(macho) => collect_macho_symbols(macho),
229 + goblin::mach::Mach::Binary(macho) => match collect_macho_symbols(macho) {
230 + Ok(s) => s,
231 + Err(e) => return mach_import_error(e),
232 + },
194 233 goblin::mach::Mach::Fat(fat) => {
195 - // Check first Mach-O arch in a fat binary
196 - fat.into_iter()
197 - .filter_map(|res| res.ok())
198 - .find_map(|arch| match arch {
199 - goblin::mach::SingleArch::MachO(macho) => Some(collect_macho_symbols(&macho)),
200 - _ => None,
201 - })
202 - .unwrap_or_default()
234 + let mut all = Vec::new();
235 + for arch in fat.into_iter().filter_map(|res| res.ok()) {
236 + if let goblin::mach::SingleArch::MachO(macho) = arch {
237 + match collect_macho_symbols(&macho) {
238 + Ok(s) => all.extend(s),
239 + Err(e) => return mach_import_error(e),
240 + }
241 + }
242 + }
243 + all
203 244 }
204 245 };
205 246
@@ -221,13 +262,20 @@ fn analyze_mach(mach: &goblin::mach::Mach) -> LayerResult {
221 262 }
222 263 }
223 264
224 - fn collect_macho_symbols(macho: &goblin::mach::MachO) -> Vec<String> {
225 - macho
226 - .imports()
227 - .unwrap_or_default()
265 + fn mach_import_error(e: goblin::error::Error) -> LayerResult {
266 + LayerResult {
267 + layer: "structural",
268 + verdict: LayerVerdict::Error,
269 + detail: Some(format!("Mach-O import table failed to parse: {e}")),
270 + }
271 + }
272 +
273 + fn collect_macho_symbols(macho: &goblin::mach::MachO) -> Result<Vec<String>, goblin::error::Error> {
274 + Ok(macho
275 + .imports()?
228 276 .iter()
229 277 .map(|imp| imp.name.to_string())
230 - .collect()
278 + .collect())
231 279 }
232 280
233 281 /// Check Mach-O import names for suspicious patterns.
@@ -270,6 +318,33 @@ mod tests {
270 318 assert_eq!(result.verdict, LayerVerdict::Skip);
271 319 }
272 320
321 + #[test]
322 + fn executable_magic_but_unparseable_fails_closed() {
323 + // "MZ" claims to be a PE but the rest is garbage goblin can't parse.
324 + // The layer's ERROR_POLICY is FailClosed, so this must hold for review,
325 + // not silently Skip (Run #2 Security MINOR).
326 + let mut data = vec![0x4D, 0x5A]; // "MZ"
327 + data.extend_from_slice(&[0xFFu8; 64]);
328 + let result = analyze_binary(&data, FileType::Download);
329 + assert_eq!(result.verdict, LayerVerdict::Error);
330 + }
331 +
332 + #[test]
333 + fn elf_magic_but_unparseable_fails_closed() {
334 + let mut data = vec![0x7F, b'E', b'L', b'F'];
335 + data.extend_from_slice(&[0x00u8; 8]); // truncated/garbage ELF
336 + let result = analyze_binary(&data, FileType::Download);
337 + assert_eq!(result.verdict, LayerVerdict::Error);
338 + }
339 +
340 + #[test]
341 + fn non_executable_magic_still_skipped() {
342 + // A ZIP (no executable magic) is a legitimate non-binary download and
343 + // must NOT be held for review by the structural layer.
344 + let result = analyze_binary(&[0x50, 0x4B, 0x03, 0x04, 0x00, 0x00], FileType::Download);
345 + assert_eq!(result.verdict, LayerVerdict::Skip);
346 + }
347 +
273 348 // -- PE import detection --
274 349
275 350 #[test]