security: harden x509 ffi
ober
620e7dc8ef85d6b7936929b632e2a1079df61889
--- a/docs/ffi-audit.md +++ b/docs/ffi-audit.md @@ -40,10 +40,10 @@ Latest summary from `tools/ffi-audit-report.ss`: (rust-file-count 29) (c-file-count 5) (no-mangle-exports 196) - (unsafe-sites 407) - (unsafe-sites-with-nearby-safety-comment 203) - (unsafe-sites-without-nearby-safety-comment 204) - (unsafe-comment-review-sites 204) + (unsafe-sites 400) + (unsafe-sites-with-nearby-safety-comment 211) + (unsafe-sites-without-nearby-safety-comment 189) + (unsafe-comment-review-sites 189) (export-review-sites 196)) (vendor (jsqlite (path "vendor/jsqlite") (status accepted-risk-cve-gated))) @@ -132,8 +132,12 @@ funnels path, SQL, text, and blob inputs through checked C ABI slice helpers, converts poisoned database/statement/result store locks into normal FFI errors, rejects null result and output slots, checks result indexes before converting signed values to `usize`, and centralizes output-buffer writes with bounded -copy invariants. The generated inventory now reports 203 annotated native -unsafe sites and 204 remaining unsafe review sites. +copy invariants. `x509.rs` now funnels certificate SAN/path/PEM inputs through +checked C ABI UTF-8 helpers, centralizes PEM/fingerprint output-buffer writes, +documents atomic file-publication syscall invariants, and has regressions for +null/nonzero input and output-slot failures. The generated inventory now +reports 211 annotated native unsafe sites and 189 remaining unsafe review +sites. Remaining work before closing K3-P1-01: --- a/docs/kimi3-security-recommmendations.md +++ b/docs/kimi3-security-recommmendations.md @@ -675,9 +675,11 @@ not started." with overflow-safe helpers, and has Rust regressions for invalid C ABI inputs. `duckdb_native.rs` now validates path, SQL, text, blob, result-slot, result-index, and output-buffer inputs through shared helpers and converts - poisoned handle-store locks into normal FFI errors. The generated report now - shows 203 annotated native unsafe sites and 204 remaining unsafe review - sites. + poisoned handle-store locks into normal FFI errors. `x509.rs` now validates + SAN/path/PEM inputs through checked UTF-8 helpers, centralizes PEM and + fingerprint output writes, and documents atomic file-publication syscalls. + The generated report now shows 211 annotated native unsafe sites and 189 + remaining unsafe review sites. Remaining work: continue unsafe invariant comments across the rest of `jerboa-native-rs`. --- a/docs/reviews/2026-07-27-native-export-review.sexp +++ b/docs/reviews/2026-07-27-native-export-review.sexp @@ -192,7 +192,7 @@ (export (symbol "jerboa_wasm_module_free") (file "jerboa-native-rs/src/wasm.rs") (line 224) (scheme-callers 1 (caller (file "lib/std/wasm/sandbox.ss") (line 76)))) (export (symbol "jerboa_wasm_module_new") (file "jerboa-native-rs/src/wasm.rs") (line 183) (scheme-callers 1 (caller (file "lib/std/wasm/sandbox.ss") (line 70)))) (export (symbol "jerboa_writev2") (file "jerboa-native-rs/src/http_parse.rs") (line 110) (scheme-callers 1 (caller (file "lib/std/net/io.ss") (line 81)))) - (export (symbol "jerboa_x509_cert_fingerprint") (file "jerboa-native-rs/src/x509.rs") (line 562) (scheme-callers 1 (caller (file "lib/std/crypto/x509.ss") (line 31)))) - (export (symbol "jerboa_x509_generate_self_signed") (file "jerboa-native-rs/src/x509.rs") (line 124) (scheme-callers 1 (caller (file "lib/std/crypto/x509.ss") (line 27)))) + (export (symbol "jerboa_x509_cert_fingerprint") (file "jerboa-native-rs/src/x509.rs") (line 607) (scheme-callers 1 (caller (file "lib/std/crypto/x509.ss") (line 31)))) + (export (symbol "jerboa_x509_generate_self_signed") (file "jerboa-native-rs/src/x509.rs") (line 234) (scheme-callers 1 (caller (file "lib/std/crypto/x509.ss") (line 27)))) ) ) --- 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, antidebug ABI, SOCKS5 server ABI, PostgreSQL native ABI, Landlock native ABI, wasmi sandbox ABI, and DuckDB native ABI now have nearby `SAFETY:` comments or equivalent checked FFI invariants, with the generated unannotated unsafe-site count at 204. | 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, SOCKS5 server ABI, PostgreSQL native ABI, Landlock native ABI, wasmi sandbox ABI, DuckDB native ABI, and X.509 native ABI now have nearby `SAFETY:` comments or equivalent checked FFI invariants, with the generated unannotated unsafe-site count at 189. | 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/x509.rs +++ b/jerboa-native-rs/src/x509.rs @@ -10,6 +10,98 @@ use std::net::IpAddr; use std::path::Path; use time::{Duration, OffsetDateTime}; +const MAX_C_ABI_SLICE_LEN: usize = isize::MAX as usize; + +fn ffi_bytes<'a>(ptr: *const u8, len: usize, label: &str) -> Result<&'a [u8], ()> { + if len > MAX_C_ABI_SLICE_LEN { + set_last_error(format!("{label} too large")); + return Err(()); + } + if ptr.is_null() && len != 0 { + set_last_error(format!("null {label}")); + return Err(()); + } + if len == 0 { + return Ok(&[]); + } + // SAFETY: `len` is bounded to `isize::MAX`, and null pointers paired with + // nonzero length were rejected above. The borrowed slice is parsed or + // copied before returning across the C ABI. + Ok(unsafe { std::slice::from_raw_parts(ptr, len) }) +} + +fn ffi_utf8(ptr: *const u8, len: usize, label: &str) -> Result<String, ()> { + let bytes = ffi_bytes(ptr, len, label)?; + std::str::from_utf8(bytes).map(str::to_owned).map_err(|e| { + set_last_error(format!("{label} is not valid UTF-8: {e}")); + }) +} + +fn copy_exact_output( + bytes: &[u8], + output: *mut u8, + output_max: usize, + output_len: *mut usize, + label: &str, +) -> i32 { + if output_len.is_null() { + set_last_error(format!("null {label} length output")); + return -1; + } + if output.is_null() && output_max != 0 { + set_last_error(format!("null {label} output")); + return -1; + } + if output_max > MAX_C_ABI_SLICE_LEN { + set_last_error(format!("{label} output too large")); + return -1; + } + if bytes.len() > output_max { + set_last_error(format!( + "{label} buffer too small: need {} have {}", + bytes.len(), + output_max + )); + return -1; + } + if !bytes.is_empty() { + // SAFETY: null output with nonzero capacity was rejected, `output_max` + // is bounded to `isize::MAX`, and the earlier length check ensures the + // destination can hold the whole PEM payload. + let out = unsafe { std::slice::from_raw_parts_mut(output, output_max) }; + out[..bytes.len()].copy_from_slice(bytes); + } + // SAFETY: `output_len` was checked non-null and points to caller-owned + // storage for the C ABI `usize` result length. + unsafe { + *output_len = bytes.len(); + } + 0 +} + +fn copy_fixed_output(bytes: &[u8], output: *mut u8, output_len: usize, label: &str) -> i32 { + if output.is_null() { + set_last_error(format!("null {label} output")); + return -1; + } + if output_len > MAX_C_ABI_SLICE_LEN { + set_last_error(format!("{label} output too large")); + return -1; + } + if output_len < bytes.len() { + set_last_error(format!( + "{label} output buffer too small (need {} bytes)", + bytes.len() + )); + return -1; + } + // SAFETY: `output` is non-null, `output_len` is bounded to `isize::MAX`, + // and the capacity check above ensures `bytes` fits in the caller buffer. + let out = unsafe { std::slice::from_raw_parts_mut(output, output_len) }; + out[..bytes.len()].copy_from_slice(bytes); + bytes.len() as i32 +} + #[cfg(unix)] fn write_new_file_atomic(path: &Path, data: &[u8], mode: u32) -> io::Result<()> { use ring::rand::{SecureRandom, SystemRandom}; @@ -47,6 +139,9 @@ fn write_new_file_atomic(path: &Path, data: &[u8], mode: u32) -> io::Result<()> temp_name.push_str(".tmp"); let temp = CString::new(temp_name).expect("generated name has no NUL"); + // SAFETY: `dirfd` comes from an open directory handle, `temp` is a + // NUL-free generated filename, and `openat` is used with no-follow and + // exclusive-create flags so the temporary file is private. let fd = unsafe { libc::openat( dirfd, @@ -63,17 +158,25 @@ fn write_new_file_atomic(path: &Path, data: &[u8], mode: u32) -> io::Result<()> return Err(error); } + // SAFETY: `fd` is a live descriptor returned by `openat`; the mode is + // caller-selected file permissions narrowed to the platform mode type. if unsafe { libc::fchmod(fd, mode as libc::mode_t) } != 0 { let error = io::Error::last_os_error(); + // SAFETY: `fd` is still owned here because `File::from_raw_fd` has + // not been called, and `temp` names the private file under `dirfd`. unsafe { libc::close(fd); libc::unlinkat(dirfd, temp.as_ptr(), 0); } return Err(error); } + // SAFETY: ownership of the live descriptor transfers exactly once to + // `File`, which closes it on drop on every following path. let mut file = unsafe { std::fs::File::from_raw_fd(fd) }; if let Err(error) = file.write_all(data).and_then(|_| file.sync_all()) { drop(file); + // SAFETY: `temp` names the private file under `dirfd`; the file + // descriptor has been closed by dropping `file`. unsafe { libc::unlinkat(dirfd, temp.as_ptr(), 0); } @@ -83,15 +186,22 @@ fn write_new_file_atomic(path: &Path, data: &[u8], mode: u32) -> io::Result<()> // linkat is an atomic no-replace publication: an existing regular // file or symlink at the destination makes the operation fail. + // SAFETY: `dirfd`, `temp`, and `destination` are valid directory/file + // descriptors or NUL-terminated names, and both paths are relative to + // the opened parent directory. let link_result = unsafe { libc::linkat(dirfd, temp.as_ptr(), dirfd, destination.as_ptr(), 0) }; let link_error = io::Error::last_os_error(); + // SAFETY: `temp` names the private temporary file under `dirfd`; after + // a successful hard-link publication this removes only the temp name. unsafe { libc::unlinkat(dirfd, temp.as_ptr(), 0); } if link_result != 0 { return Err(link_error); } + // SAFETY: `dirfd` remains valid for the lifetime of `directory`; fsync + // flushes the parent directory entry for the published file. if unsafe { libc::fsync(dirfd) } != 0 { return Err(io::Error::last_os_error()); } @@ -132,42 +242,23 @@ pub extern "C" fn jerboa_x509_generate_self_signed( ) -> i32 { ffi_wrap(|| { // Validate inputs - if ip_addrs_csv.is_null() || cert_path.is_null() || key_path.is_null() { - set_last_error("null pointer argument".to_string()); - return -1; - } if validity_days <= 0 { set_last_error("validity_days must be positive".to_string()); return -1; } // Parse strings from FFI - let ip_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(ip_addrs_csv, ip_addrs_len) - }) { + let ip_str = match ffi_utf8(ip_addrs_csv, ip_addrs_len, "SAN list") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 SAN list".into()); - return -1; - } + Err(()) => return -1, }; - let cert_path_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(cert_path, cert_path_len) - }) { + let cert_path_str = match ffi_utf8(cert_path, cert_path_len, "certificate path") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 certificate path".into()); - return -1; - } + Err(()) => return -1, }; - let key_path_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(key_path, key_path_len) - }) { + let key_path_str = match ffi_utf8(key_path, key_path_len, "private-key path") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 private-key path".into()); - return -1; - } + Err(()) => return -1, }; // Parse SANs — each entry is either an IP address or a DNS hostname @@ -228,7 +319,7 @@ pub extern "C" fn jerboa_x509_generate_self_signed( // Publish the private key first, with its final mode already applied. // Existing files and symlinks are rejected atomically. let key_pem = key_pair.serialize_pem(); - if let Err(e) = write_new_file_atomic(Path::new(key_path_str), key_pem.as_bytes(), 0o600) { + if let Err(e) = write_new_file_atomic(Path::new(&key_path_str), key_pem.as_bytes(), 0o600) { set_last_error(format!( "failed to create key file '{}': {}", key_path_str, e @@ -239,7 +330,7 @@ pub extern "C" fn jerboa_x509_generate_self_signed( // The certificate is public, but still uses no-follow/no-replace // publication so it cannot clobber an attacker-selected path. let cert_pem = cert.pem(); - if let Err(e) = write_new_file_atomic(Path::new(cert_path_str), cert_pem.as_bytes(), 0o644) + if let Err(e) = write_new_file_atomic(Path::new(&cert_path_str), cert_pem.as_bytes(), 0o644) { let _ = std::fs::remove_file(key_path_str); set_last_error(format!( @@ -271,28 +362,14 @@ pub extern "C" fn jerboa_x509_generate_self_signed_mem( key_out_len: *mut usize, ) -> i32 { ffi_wrap(|| { - if ip_addrs_csv.is_null() - || cert_out.is_null() - || key_out.is_null() - || cert_out_len.is_null() - || key_out_len.is_null() - { - set_last_error("null pointer argument".to_string()); - return -1; - } if validity_days <= 0 { set_last_error("validity_days must be positive".to_string()); return -1; } - let ip_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(ip_addrs_csv, ip_addrs_len) - }) { + let ip_str = match ffi_utf8(ip_addrs_csv, ip_addrs_len, "SAN list") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 SAN list".into()); - return -1; - } + Err(()) => return -1, }; // Parse SANs @@ -346,30 +423,27 @@ pub extern "C" fn jerboa_x509_generate_self_signed_mem( let cert_pem = cert.pem(); let key_pem = key_pair.serialize_pem(); - if cert_pem.len() > cert_out_max { - set_last_error(format!( - "cert buffer too small: need {} have {}", - cert_pem.len(), - cert_out_max - )); + if copy_exact_output( + cert_pem.as_bytes(), + cert_out, + cert_out_max, + cert_out_len, + "certificate", + ) != 0 + { return -1; } - if key_pem.len() > key_out_max { - set_last_error(format!( - "key buffer too small: need {} have {}", - key_pem.len(), - key_out_max - )); + if copy_exact_output( + key_pem.as_bytes(), + key_out, + key_out_max, + key_out_len, + "private-key", + ) != 0 + { return -1; } - unsafe { - std::ptr::copy_nonoverlapping(cert_pem.as_ptr(), cert_out, cert_pem.len()); - *cert_out_len = cert_pem.len(); - std::ptr::copy_nonoverlapping(key_pem.as_ptr(), key_out, key_pem.len()); - *key_out_len = key_pem.len(); - } - 0 }) } @@ -399,59 +473,33 @@ pub extern "C" fn jerboa_x509_generate_signed_by_ca_mem( key_out_len: *mut usize, ) -> i32 { ffi_wrap(|| { - if ca_cert_pem.is_null() - || ca_key_pem.is_null() - || ip_addrs_csv.is_null() - || cert_out.is_null() - || key_out.is_null() - || cert_out_len.is_null() - || key_out_len.is_null() - { - set_last_error("null pointer argument".to_string()); - return -1; - } if validity_days <= 0 { set_last_error("validity_days must be positive".to_string()); return -1; } - let ca_cert_str = unsafe { - match std::str::from_utf8(std::slice::from_raw_parts(ca_cert_pem, ca_cert_pem_len)) { - Ok(s) => s, - Err(e) => { - set_last_error(format!("ca cert is not valid UTF-8: {}", e)); - return -1; - } - } + let ca_cert_str = match ffi_utf8(ca_cert_pem, ca_cert_pem_len, "CA cert") { + Ok(value) => value, + Err(()) => return -1, }; - let ca_key_str = unsafe { - match std::str::from_utf8(std::slice::from_raw_parts(ca_key_pem, ca_key_pem_len)) { - Ok(s) => s, - Err(e) => { - set_last_error(format!("ca key is not valid UTF-8: {}", e)); - return -1; - } - } + let ca_key_str = match ffi_utf8(ca_key_pem, ca_key_pem_len, "CA key") { + Ok(value) => value, + Err(()) => return -1, }; - let ip_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(ip_addrs_csv, ip_addrs_len) - }) { + let ip_str = match ffi_utf8(ip_addrs_csv, ip_addrs_len, "SAN list") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 SAN list".into()); - return -1; - } + Err(()) => return -1, }; // Reconstruct the CA certificate + keypair from PEM. - let ca_params = match CertificateParams::from_ca_cert_pem(ca_cert_str) { + let ca_params = match CertificateParams::from_ca_cert_pem(&ca_cert_str) { Ok(p) => p, Err(e) => { set_last_error(format!("failed to parse CA cert PEM: {}", e)); return -1; } }; - let ca_kp = match KeyPair::from_pem(ca_key_str) { + let ca_kp = match KeyPair::from_pem(&ca_key_str) { Ok(k) => k, Err(e) => { set_last_error(format!("failed to parse CA key PEM: {}", e)); @@ -524,30 +572,27 @@ pub extern "C" fn jerboa_x509_generate_signed_by_ca_mem( let cert_pem = leaf_cert.pem(); let key_pem = leaf_kp.serialize_pem(); - if cert_pem.len() > cert_out_max { - set_last_error(format!( - "cert buffer too small: need {} have {}", - cert_pem.len(), - cert_out_max - )); + if copy_exact_output( + cert_pem.as_bytes(), + cert_out, + cert_out_max, + cert_out_len, + "certificate", + ) != 0 + { return -1; } - if key_pem.len() > key_out_max { - set_last_error(format!( - "key buffer too small: need {} have {}", - key_pem.len(), - key_out_max - )); + if copy_exact_output( + key_pem.as_bytes(), + key_out, + key_out_max, + key_out_len, + "private-key", + ) != 0 + { return -1; } - unsafe { - std::ptr::copy_nonoverlapping(cert_pem.as_ptr(), cert_out, cert_pem.len()); - *cert_out_len = cert_pem.len(); - std::ptr::copy_nonoverlapping(key_pem.as_ptr(), key_out, key_pem.len()); - *key_out_len = key_pem.len(); - } - 0 }) } @@ -566,27 +611,18 @@ pub extern "C" fn jerboa_x509_cert_fingerprint( output_len: usize, ) -> i32 { ffi_wrap(|| { - if cert_path.is_null() || output.is_null() { - set_last_error("null pointer argument".to_string()); - return -1; - } if output_len < 32 { set_last_error("output buffer too small (need 32 bytes)".to_string()); return -1; } - let path_str = match std::str::from_utf8(unsafe { - std::slice::from_raw_parts(cert_path, cert_path_len) - }) { + let path_str = match ffi_utf8(cert_path, cert_path_len, "certificate path") { Ok(value) => value, - Err(_) => { - set_last_error("invalid UTF-8 certificate path".into()); - return -1; - } + Err(()) => return -1, }; // Read and parse PEM file - let pem_data = match std::fs::read(path_str) { + let pem_data = match std::fs::read(&path_str) { Ok(d) => d, Err(e) => { set_last_error(format!("failed to read cert file '{}': {}", path_str, e)); @@ -610,10 +646,7 @@ pub extern "C" fn jerboa_x509_cert_fingerprint( // SHA-256 of the first certificate's DER encoding let hash = digest::digest(&digest::SHA256, certs[0].as_ref()); - unsafe { - std::ptr::copy_nonoverlapping(hash.as_ref().as_ptr(), output, 32); - } - 32 + copy_fixed_output(hash.as_ref(), output, output_len, "fingerprint") }) } @@ -734,4 +767,70 @@ mod security_tests { -1 ); } + + #[test] + fn null_nonzero_ffi_inputs_are_rejected() { + let name = b"127.0.0.1"; + let mut cert = vec![0_u8; 4096]; + let mut key = vec![0_u8; 4096]; + let mut cert_len = 0_usize; + let mut key_len = 0_usize; + + assert_eq!( + jerboa_x509_generate_self_signed_mem( + std::ptr::null(), + 1, + 1, + cert.as_mut_ptr(), + cert.len(), + &mut cert_len, + key.as_mut_ptr(), + key.len(), + &mut key_len, + ), + -1 + ); + assert_eq!( + jerboa_x509_generate_self_signed( + std::ptr::null(), + 1, + 1, + name.as_ptr(), + name.len(), + name.as_ptr(), + name.len(), + ), + -1 + ); + } + + #[test] + fn output_helpers_validate_slots_and_capacity() { + let mut len = 0_usize; + let mut out = [0_u8; 2]; + + assert_eq!( + copy_exact_output(b"abc", out.as_mut_ptr(), out.len(), &mut len, "test"), + -1 + ); + assert_eq!( + copy_exact_output(b"a", std::ptr::null_mut(), 1, &mut len, "test"), + -1 + ); + assert_eq!( + copy_exact_output( + b"a", + out.as_mut_ptr(), + out.len(), + std::ptr::null_mut(), + "test" + ), + -1 + ); + assert_eq!( + copy_fixed_output(b"abc", out.as_mut_ptr(), out.len(), "test"), + -1 + ); + assert_eq!(copy_fixed_output(b"a", std::ptr::null_mut(), 1, "test"), -1); + } }