security: harden socks5 ffi
ober
e5951bc5e8299f5e51ac2da10130914fa2073c16
--- a/docs/ffi-audit.md +++ b/docs/ffi-audit.md @@ -114,8 +114,11 @@ syscall and caller-output invariants, bounds caller-controlled event counts, checks output byte-size derivation, and treats empty nonblocking eventfd drains as normal. `antidebug.rs` now documents ptrace and breakpoint-probe invariants, reports ptrace OS errors, and saturates timing duration conversion instead of -truncating. The generated inventory now reports 184 annotated native unsafe -sites and 246 remaining unsafe review sites. +truncating. `socks5_server.rs` now rejects null pointers paired with nonzero +lengths before building FFI slices, bounds caller lengths, documents stats +buffer copies, and returns normal FFI errors for poisoned server-registry +locks. The generated inventory now reports 184 annotated native unsafe sites +and 246 remaining unsafe review sites. Remaining work before closing K3-P1-01: --- a/docs/kimi3-security-recommmendations.md +++ b/docs/kimi3-security-recommmendations.md @@ -659,8 +659,11 @@ not started." derivation, and treats empty nonblocking eventfd drains as normal. `antidebug.rs` now documents ptrace and breakpoint-probe invariants, reports ptrace OS errors, and saturates timing duration conversion instead of - truncating. The generated report now shows 184 annotated native unsafe sites and 246 - remaining unsafe review sites. + truncating. `socks5_server.rs` now rejects null pointers paired with nonzero + lengths before building FFI slices, bounds caller lengths, documents stats + buffer copies, and returns normal FFI errors for poisoned server-registry + locks. The generated report now shows 184 annotated native unsafe sites and + 246 remaining unsafe review sites. Remaining work: continue unsafe invariant comments across the rest of `jerboa-native-rs`. --- a/docs/status.md +++ b/docs/status.md @@ -25,7 +25,7 @@ release artifacts are built as Jerboa multicall binaries with `jerboa`, | Area | Current state | Remaining work | |---|---|---| | Kimi security handoff | [kimi3-security-recommmendations.md](kimi3-security-recommmendations.md) is the backlog. Dated evidence and review manifests live under [reviews/](reviews/). | Keep new security evidence in dated review records and summarize the current release state here. | -| FFI audit phase 5 | [ffi-audit.md](ffi-audit.md) records the scanner output, provisional Scheme binding verdicts, and native Rust export inventory. `make native-export-review-check` gates native export decisions. The worker-launch native path in `aproc.rs`/`seccomp.rs`, crypto FFI buffer path in `crypto.rs`, secure-memory region lifecycle in `secure_mem.rs`, Ed25519/X25519 key-agreement buffers, compression buffers, HTTP parse/writev boundary, embed-crypto ABI, integrity ABI, regex-native ABI, process-control ABI, pcap ABI, inotify ABI, epoll/eventfd ABI, and antidebug ABI now have nearby `SAFETY:` comments, with the generated unannotated unsafe-site count at 246. | Continue adding `SAFETY:` invariant comments near the remaining Rust unsafe sites. | +| FFI audit phase 5 | [ffi-audit.md](ffi-audit.md) records the scanner output, provisional Scheme binding verdicts, and native Rust export inventory. `make native-export-review-check` gates native export decisions. The worker-launch native path in `aproc.rs`/`seccomp.rs`, crypto FFI buffer path in `crypto.rs`, secure-memory region lifecycle in `secure_mem.rs`, Ed25519/X25519 key-agreement buffers, compression buffers, HTTP parse/writev boundary, embed-crypto ABI, integrity ABI, regex-native ABI, process-control ABI, pcap ABI, inotify ABI, epoll/eventfd ABI, antidebug ABI, and SOCKS5 server ABI now have nearby `SAFETY:` comments or equivalent checked FFI invariants, with the generated unannotated unsafe-site count at 246. | Continue adding `SAFETY:` invariant comments near the remaining Rust unsafe sites. | | Native Rust exports | The native export review now has 190 exported functions: 183 tracked Scheme references and 7 retained standalone C/binary helpers. The previous 35 no-Scheme-reference removal candidates no longer have C ABI export markers. | Re-run `make native-export-review-check` whenever adding or removing native exports. | | Confined worker | `(std security worker)` provides the facade, audit lifecycle, output caps, deadlines, process-group kill, memory rlimit pre-exec setup, Linux syscall/ptrace seccomp pre-exec setup for requested axes, explicit sandbox-axis refusal, and egress proxy env wiring. | Install native Landlock path/net rules in the worker pre-exec path and keep Linux/macOS/FreeBSD parity tests current. | | Safe surface | Direct scripts default to the safe prelude; raw access requires `--unsafe-prelude` or `(jerboa prelude unsafe)`. | Continue moving risky APIs behind explicit unsafe imports as new modules land. | --- a/jerboa-native-rs/src/socks5_server.rs +++ b/jerboa-native-rs/src/socks5_server.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::io::{Read, Write}; use std::net::{IpAddr, Shutdown, TcpListener, TcpStream}; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::sync::{Arc, Mutex, OnceLock}; +use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; use std::thread; use crate::panic::set_last_error; @@ -29,6 +29,16 @@ fn servers() -> &'static Mutex<HashMap<u64, Socks5Server>> { INSTANCE.get_or_init(|| Mutex::new(HashMap::new())) } +fn lock_servers() -> Option<MutexGuard<'static, HashMap<u64, Socks5Server>>> { + match servers().lock() { + Ok(guard) => Some(guard), + Err(_) => { + set_last_error("socks5 server registry lock poisoned".into()); + None + } + } +} + static NEXT_HANDLE: AtomicU64 = AtomicU64::new(1); static GLOBAL_ACTIVE_CONNECTIONS: AtomicU64 = AtomicU64::new(0); @@ -37,6 +47,7 @@ const MAX_CONNECTIONS_PER_CLIENT: u64 = 16; const MAX_CONNECTIONS_GLOBAL: u64 = 512; const HANDSHAKE_TIMEOUT_SECS: u64 = 10; const RELAY_IDLE_TIMEOUT_SECS: u64 = 120; +const MAX_C_ABI_SLICE_LEN: usize = isize::MAX as usize; fn next_handle() -> u64 { NEXT_HANDLE.fetch_add(1, Ordering::Relaxed) @@ -118,8 +129,19 @@ pub extern "C" fn jerboa_socks5_server_start( ) -> u64 { match std::panic::catch_unwind(|| { let addr_str = if bind_addr.is_null() || bind_addr_len == 0 { + if bind_addr.is_null() && bind_addr_len > 0 { + set_last_error("null bind address with nonzero length".into()); + return 0; + } "127.0.0.1".to_string() } else { + if bind_addr_len > MAX_C_ABI_SLICE_LEN { + set_last_error("bind address length exceeds platform limit".into()); + return 0; + } + // SAFETY: bind_addr is non-null, bind_addr_len is nonzero and + // bounded by isize::MAX, and the caller must keep the byte buffer + // live for this synchronous conversion. let slice = unsafe { std::slice::from_raw_parts(bind_addr, bind_addr_len) }; match std::str::from_utf8(slice) { Ok(s) => s.to_string(), @@ -131,10 +153,24 @@ pub extern "C" fn jerboa_socks5_server_start( }; let credentials = if !username.is_null() && username_len > 0 { + if username_len > MAX_C_ABI_SLICE_LEN || password_len > MAX_C_ABI_SLICE_LEN { + set_last_error("credential length exceeds platform limit".into()); + return 0; + } + if password.is_null() && password_len > 0 { + set_last_error("null password with nonzero length".into()); + return 0; + } + // SAFETY: username is non-null, username_len is nonzero and + // bounded by isize::MAX, and the caller keeps the bytes live for + // this synchronous UTF-8 validation/copy. let u = unsafe { std::slice::from_raw_parts(username, username_len) }; let p = if password.is_null() { &[] } else { + // SAFETY: password is non-null here, password_len is bounded + // by isize::MAX above, and the bytes are used only for this + // synchronous UTF-8 validation/copy. unsafe { std::slice::from_raw_parts(password, password_len) } }; match (std::str::from_utf8(u), std::str::from_utf8(p)) { @@ -144,6 +180,12 @@ pub extern "C" fn jerboa_socks5_server_start( return 0; } } + } else if username.is_null() && username_len > 0 { + set_last_error("null username with nonzero length".into()); + return 0; + } else if password_len > 0 { + set_last_error("password supplied without username".into()); + return 0; } else { None }; @@ -205,7 +247,10 @@ pub extern "C" fn jerboa_socks5_server_start( total_conns, }; - servers().lock().unwrap().insert(id, srv); + let Some(mut map) = lock_servers() else { + return 0; + }; + map.insert(id, srv); id }) { Ok(id) => id, @@ -220,7 +265,9 @@ pub extern "C" fn jerboa_socks5_server_start( /// Returns 0 on success, -1 on error. pub extern "C" fn jerboa_socks5_server_stop(handle: u64) -> i32 { match std::panic::catch_unwind(|| { - let mut map = servers().lock().unwrap(); + let Some(mut map) = lock_servers() else { + return -1; + }; match map.remove(&handle) { Some(srv) => { srv.stop.store(true, Ordering::Relaxed); @@ -246,7 +293,9 @@ pub extern "C" fn jerboa_socks5_server_stop(handle: u64) -> i32 { /// Returns port (>0) on success, 0 on error. pub extern "C" fn jerboa_socks5_server_port(handle: u64) -> u16 { match std::panic::catch_unwind(|| { - let map = servers().lock().unwrap(); + let Some(map) = lock_servers() else { + return 0; + }; match map.get(&handle) { Some(srv) => srv.port, None => { @@ -269,7 +318,13 @@ pub extern "C" fn jerboa_socks5_server_stats(handle: u64, buf: *mut u8, buf_len: set_last_error("null buffer".into()); return -1; } - let map = servers().lock().unwrap(); + if buf_len > MAX_C_ABI_SLICE_LEN { + set_last_error("stats buffer length exceeds platform limit".into()); + return -1; + } + let Some(map) = lock_servers() else { + return -1; + }; match map.get(&handle) { Some(srv) => { let active = srv.active_conns.load(Ordering::Relaxed); @@ -277,6 +332,9 @@ pub extern "C" fn jerboa_socks5_server_stats(handle: u64, buf: *mut u8, buf_len: let s = format!("active:{} total:{}", active, total); let bytes = s.as_bytes(); let n = bytes.len().min(buf_len); + // SAFETY: buf is non-null, buf_len is bounded by isize::MAX, + // and n is no larger than both the source string and caller + // output buffer. The destination pointer is not retained. unsafe { std::ptr::copy_nonoverlapping(bytes.as_ptr(), buf, n); } @@ -293,6 +351,67 @@ pub extern "C" fn jerboa_socks5_server_stats(handle: u64, buf: *mut u8, buf_len: } } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ffi_start_rejects_null_bind_with_nonzero_length() { + assert_eq!( + jerboa_socks5_server_start( + std::ptr::null(), + 4, + 0, + std::ptr::null(), + 0, + std::ptr::null(), + 0 + ), + 0 + ); + } + + #[test] + fn ffi_start_rejects_null_username_with_nonzero_length() { + let bind = b"127.0.0.1"; + assert_eq!( + jerboa_socks5_server_start( + bind.as_ptr(), + bind.len(), + 0, + std::ptr::null(), + 4, + std::ptr::null(), + 0 + ), + 0 + ); + } + + #[test] + fn ffi_start_rejects_null_password_with_nonzero_length() { + let bind = b"127.0.0.1"; + let username = b"user"; + assert_eq!( + jerboa_socks5_server_start( + bind.as_ptr(), + bind.len(), + 0, + username.as_ptr(), + username.len(), + std::ptr::null(), + 4, + ), + 0 + ); + } + + #[test] + fn ffi_stats_rejects_null_buffer() { + assert_eq!(jerboa_socks5_server_stats(1, std::ptr::null_mut(), 8), -1); + } +} + // ============================================================ // Server internals // ============================================================