diff --git a/src/dotenv/env_loader.rs b/src/dotenv/env_loader.rs index b7a3754729f..774ee87792a 100644 --- a/src/dotenv/env_loader.rs +++ b/src/dotenv/env_loader.rs @@ -1448,12 +1448,18 @@ impl Map { let mut i: usize = 0; let mut it = self.map.iterator(); while let Some(pair) = it.next() { + if i + pair.key_ptr.len() + 7 >= result.len() { + return Err(bun_core::Error::from_name("TooManyEnvironmentVariables")); + } i += strings::convert_utf8_to_utf16_in_buffer(&mut result[i..], pair.key_ptr).len(); if i + 7 >= result.len() { return Err(bun_core::Error::from_name("TooManyEnvironmentVariables")); } result[i] = b'=' as u16; i += 1; + if i + pair.value_ptr.value.len() + 5 >= result.len() { + return Err(bun_core::Error::from_name("TooManyEnvironmentVariables")); + } i += strings::convert_utf8_to_utf16_in_buffer(&mut result[i..], &pair.value_ptr.value) .len(); if i + 5 >= result.len() { diff --git a/src/http/HTTPContext.rs b/src/http/HTTPContext.rs index 2bc05f5ff68..a42a2ef2d20 100644 --- a/src/http/HTTPContext.rs +++ b/src/http/HTTPContext.rs @@ -170,6 +170,12 @@ pub struct PooledSocket { pub port: u16, /// If you set `rejectUnauthorized` to `false`, the connection fails to verify, pub did_have_handshaking_error_while_reject_unauthorized_is_false: bool, + /// True if the TLS handshake for this socket ran with + /// `rejectUnauthorized=true` (i.e. `checkServerIdentity` was enforced). + /// A socket established with `rejectUnauthorized=false` never validated the + /// peer hostname, so a strict caller must not reuse it even when the chain + /// itself was CA-valid (`did_have_handshaking_error` stays false). + pub established_with_reject_unauthorized: bool, /// The interned SSLConfig this socket was created with (None = default context). /// Owns a strong ref while the socket is in the keepalive pool. pub ssl_config: Option, @@ -606,6 +612,7 @@ impl HTTPContext { &mut self, socket: HTTPSocket, did_have_handshaking_error_while_reject_unauthorized_is_false: bool, + established_with_reject_unauthorized: bool, hostname: &[u8], port: u16, ssl_config: Option<&ssl_config::SharedPtr>, @@ -660,6 +667,7 @@ impl HTTPContext { hostname_len: hostname.len() as u8, // @truncate port, did_have_handshaking_error_while_reject_unauthorized_is_false, + established_with_reject_unauthorized, // Clone a strong ref for the keepalive pool; the caller retains // its own ref via HTTPClient.tls_props. ssl_config: ssl_config.cloned(), @@ -786,6 +794,17 @@ impl HTTPContext { { continue; } + } else if SSL + // Same failure mode as the tunnel branch above, for direct + // HTTPS sockets: a socket established with + // reject_unauthorized=false never ran checkServerIdentity, so + // a CA-valid wrong-hostname cert leaves + // did_have_handshaking_error=false and the outer guard passes. + // Block a strict caller from reusing it. + && reject_unauthorized + && !socket.established_with_reject_unauthorized + { + continue; } if strings::eql_long( @@ -904,14 +923,28 @@ impl HTTPContext { // the centralised [`h2_session_as_mut`] accessor — same // strong-ref-held invariant as the pool/found-slot cases. let s = h2_session_as_mut(NonNull::new(session)).unwrap(); - if s.has_headroom() && s.matches(hostname, port, cfg) { + if s.has_headroom() + && s.matches(hostname, port, cfg) + // Same guard as the pool path: a session whose TLS + // handshake ran with reject_unauthorized=false never + // validated the peer hostname, so a strict caller + // must not multiplex onto it. + && (!client.flags.reject_unauthorized + || s.established_with_reject_unauthorized) + { s.adopt(client); return Ok(None); } } let cfg_nn = cfg.and_then(|p| NonNull::new(p.cast_mut())); for pc in &mut self.pending_h2_connects { - if pc.matches(hostname, port, cfg_nn) { + // Same strictness guard as the active-session loop above: a + // strict caller must not coalesce onto an in-flight connect + // that was initiated with reject_unauthorized=false, since + // the resulting session won't have validated the peer. + if pc.matches(hostname, port, cfg_nn) + && (!client.flags.reject_unauthorized || pc.reject_unauthorized) + { // client outlives the pending connect (resolved before // its terminal callback fires). pc.waiters.push(client.as_erased_ptr()); @@ -1036,6 +1069,7 @@ impl HTTPContext { hostname: Box::<[u8]>::from(hostname), port, ssl_config: cfg, + reject_unauthorized: client.flags.reject_unauthorized, ..Default::default() }); // `client.pending_h2 = pc` stores a *borrowed* backref into the diff --git a/src/http/h2_client/ClientSession.rs b/src/http/h2_client/ClientSession.rs index 569987640ae..4fb4c94973d 100644 --- a/src/http/h2_client/ClientSession.rs +++ b/src/http/h2_client/ClientSession.rs @@ -49,6 +49,10 @@ pub struct ClientSession { pub port: u16, pub ssl_config: Option, pub did_have_handshaking_error: bool, + /// True if the TLS handshake ran with `rejectUnauthorized=true`. Carried + /// into the keepalive pool so a strict caller never reuses a session whose + /// hostname was never validated. + pub established_with_reject_unauthorized: bool, /// Queued bytes for the socket; whole frames are written here and /// `flush()` drains as much as the socket accepts. @@ -234,6 +238,7 @@ impl ClientSession { port: client.connected_url.get_port_auto(), ssl_config: client.tls_props.clone(), did_have_handshaking_error: client.flags.did_have_handshaking_error, + established_with_reject_unauthorized: client.flags.reject_unauthorized, write_buffer: bun_io::StreamBuffer::default(), read_buffer: Vec::new(), streams: ArrayHashMap::default(), @@ -895,6 +900,7 @@ impl ClientSession { HTTPClient::ssl_ctx_mut(self.ctx).release_socket( self.socket, self.did_have_handshaking_error, + self.established_with_reject_unauthorized, &self.hostname, self.port, self.ssl_config.as_ref(), diff --git a/src/http/h2_client/PendingConnect.rs b/src/http/h2_client/PendingConnect.rs index 403751de57f..706308fc41c 100644 --- a/src/http/h2_client/PendingConnect.rs +++ b/src/http/h2_client/PendingConnect.rs @@ -17,6 +17,12 @@ pub struct PendingConnect { pub port: u16, // TODO(port): lifetime — compared by pointer identity only, never derefed/freed here pub ssl_config: Option>, + /// Whether the client that initiated this in-flight TLS connect requested + /// `rejectUnauthorized`. The eventual `ClientSession` records this as + /// `established_with_reject_unauthorized`; mirroring it here lets the + /// coalescing path apply the same strictness guard *before* the session + /// exists, so a strict caller never waits on a connect started by a lax one. + pub reject_unauthorized: bool, // BACKREF: waiters are borrowed HTTP clients owned elsewhere; lifetime-erased. pub waiters: Vec>>, } @@ -27,6 +33,7 @@ impl Default for PendingConnect { hostname: Box::default(), port: 0, ssl_config: None, + reject_unauthorized: false, waiters: Vec::new(), } } diff --git a/src/http/lib.rs b/src/http/lib.rs index faa73e950f5..66d67c6ab99 100644 --- a/src/http/lib.rs +++ b/src/http/lib.rs @@ -2315,6 +2315,7 @@ impl<'a> HTTPClient<'a> { Self::ssl_ctx_mut(ctx).release_socket( socket, self.flags.did_have_handshaking_error && !self.flags.reject_unauthorized, + self.flags.reject_unauthorized, self.connected_url.hostname, self.connected_url.get_port_auto(), self.tls_props.as_ref(), @@ -3591,6 +3592,7 @@ impl<'a> HTTPClient<'a> { Self::ssl_ctx_mut(ctx).release_socket( socket, self.flags.did_have_handshaking_error && !self.flags.reject_unauthorized, + self.flags.reject_unauthorized, self.connected_url.hostname, self.connected_url.get_port_auto(), self.tls_props.as_ref(), @@ -3812,6 +3814,7 @@ impl<'a> HTTPClient<'a> { Self::ssl_ctx_mut(ctx).release_socket( socket, self.flags.did_have_handshaking_error && !self.flags.reject_unauthorized, + self.flags.reject_unauthorized, self.url.hostname, self.url.get_port_auto(), self.tls_props.as_ref(), diff --git a/src/http_jsc/websocket_client.rs b/src/http_jsc/websocket_client.rs index 9701b48fbee..bb158280a11 100644 --- a/src/http_jsc/websocket_client.rs +++ b/src/http_jsc/websocket_client.rs @@ -287,15 +287,13 @@ impl WebSocket { if reject_unauthorized { // Check for SSL errors if ssl_error.error_no != 0 { - self.outgoing_websocket = None; - ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); + self.fail(ErrorCode::FailedToConnect); return; } // Check authorization status if !authorized { - self.outgoing_websocket = None; - ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); + self.fail(ErrorCode::FailedToConnect); return; } @@ -317,8 +315,8 @@ impl WebSocket { if !ssl_ptr.is_null() && !boringssl::check_server_identity(unsafe { &mut *ssl_ptr }, hostname) { - self.outgoing_websocket = None; - ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); + self.fail(ErrorCode::FailedToConnect); + return; } } } @@ -1322,6 +1320,10 @@ impl WebSocket { } fn send_close_with_body(&mut self, code: u16, body: Option<&mut [u8; 125]>, body_len: usize) { + // RFC 6455 §5.5: control-frame payloads are capped at 125 bytes total, + // and a close-frame payload starts with the 2-byte status code, so the + // reason text is limited to 123 bytes. + let body_len = body_len.min(123); log!("Sending close with code {}", code); if !self.has_tcp() { self.dispatch_abrupt_close(ErrorCode::Ended); @@ -1667,6 +1669,10 @@ impl WebSocket { cursor.set_position((pos + result.written as usize) as u64); } let wrote_len = cursor.position() as usize; + // 125-byte close-frame payload budget minus the 2-byte status code. + if wrote_len > 123 { + break 'inner; + } // SAFETY: close_reason_buf has 128 bytes; reinterpret first 125 as fixed array let buf_ptr = close_reason_buf.as_mut_ptr().cast::<[u8; 125]>(); this.send_close_with_body(code, Some(unsafe { &mut *buf_ptr }), wrote_len); diff --git a/src/install/bin.rs b/src/install/bin.rs index 5b3250f2b74..4f013443f06 100644 --- a/src/install/bin.rs +++ b/src/install/bin.rs @@ -781,9 +781,16 @@ static HAS_SET_UMASK: AtomicBool = AtomicBool::new(false); impl<'a> Linker<'a> { pub fn ensure_umask() { - if !HAS_SET_UMASK.load(Ordering::Acquire) { - HAS_SET_UMASK.store(true, Ordering::Release); - UMASK.store(sys::umask(0) as u32, Ordering::Release); + // Single-winner gate: only the thread that flips false->true performs + // the temporary umask(0)/umask(prev) probe. A bare load+store would let + // two threads interleave the probe and leave the process umask wrong. + if HAS_SET_UMASK + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + let prev = sys::umask(0); + sys::umask(prev); + UMASK.store(prev as u32, Ordering::Release); } } @@ -1269,7 +1276,7 @@ impl<'a> Linker<'a> { fn chmod_on_ok(err: &Option, abs_target: &ZStr) { // PORT NOTE: hoisted from `defer` block in create_symlink if err.is_none() { - let _ = sys::chmod(abs_target, UMASK.load(Ordering::Acquire) as Mode | 0o777); + let _ = sys::chmod(abs_target, 0o777 & !(UMASK.load(Ordering::Acquire) as Mode)); } } @@ -1444,6 +1451,12 @@ impl<'a> Linker<'a> { ZStr::from_raw(r.as_bytes().as_ptr(), r.len()) }; + if unscoped_package_name.len() + >= self.abs_dest_buf.len().saturating_sub(dest_off) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } self.abs_dest_buf[dest_off..dest_off + unscoped_package_name.len()] .copy_from_slice(unscoped_package_name); dest_off += unscoped_package_name.len(); @@ -1596,6 +1609,12 @@ impl<'a> Linker<'a> { ZStr::from_raw(r.as_bytes().as_ptr(), r.len()) }; + if entry_name.len() + >= self.abs_dest_buf.len().saturating_sub(abs_dest_dir_end) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } dest_off = abs_dest_dir_end; self.abs_dest_buf[dest_off..dest_off + entry_name.len()] .copy_from_slice(entry_name); @@ -1633,6 +1652,12 @@ impl<'a> Linker<'a> { Tag::File => { let unscoped_package_name = Dependency::unscoped_package_name(self.package_name.slice()); + if unscoped_package_name.len() + >= self.abs_dest_buf.len().saturating_sub(dest_off) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } self.abs_dest_buf[dest_off..dest_off + unscoped_package_name.len()] .copy_from_slice(unscoped_package_name); dest_off += unscoped_package_name.len(); @@ -1649,6 +1674,12 @@ impl<'a> Linker<'a> { if normalized_name.is_empty() { return; } + if normalized_name.len() + >= self.abs_dest_buf.len().saturating_sub(dest_off) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } self.abs_dest_buf[dest_off..dest_off + normalized_name.len()] .copy_from_slice(normalized_name); @@ -1672,6 +1703,12 @@ impl<'a> Linker<'a> { i += 2; continue; } + if normalized_bin_dest.len() + >= self.abs_dest_buf.len().saturating_sub(abs_dest_dir_end) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } dest_off = abs_dest_dir_end; self.abs_dest_buf[dest_off..dest_off + normalized_bin_dest.len()] @@ -1714,6 +1751,12 @@ impl<'a> Linker<'a> { match entry.kind { sys::EntryKind::SymLink | sys::EntryKind::File => { let entry_name = entry.name.slice_u8(); + if entry_name.len() + >= self.abs_dest_buf.len().saturating_sub(abs_dest_dir_end) + { + self.err = Some(bun_core::err!("NameTooLong")); + return; + } dest_off = abs_dest_dir_end; self.abs_dest_buf[dest_off..dest_off + entry_name.len()] .copy_from_slice(entry_name); diff --git a/src/install/dependency.rs b/src/install/dependency.rs index 7b532ae7f65..017e19ee8f5 100644 --- a/src/install/dependency.rs +++ b/src/install/dependency.rs @@ -680,9 +680,15 @@ impl VersionExt for Version { let slice = String { bytes: bytes[1..9].try_into().expect("infallible: size matches"), }; + if !slice.is_inline() { + let ptr = slice.ptr(); + if (ptr.off as usize).saturating_add(ptr.len as usize) > ctx.buffer.len() { + return Version::default(); + } + } // bytes[0] was written by `to_external` from a valid `Tag`; decode by - // exhaustive match so a corrupt lockfile byte traps instead of - // producing an invalid discriminant. + // exhaustive match so a corrupt lockfile byte degrades to an + // uninitialized version (and a logged error) instead of aborting. let tag: Tag = match bytes[0] { 0 => Tag::Uninitialized, 1 => Tag::Npm, @@ -694,7 +700,14 @@ impl VersionExt for Version { 7 => Tag::Git, 8 => Tag::Github, 9 => Tag::Catalog, - n => unreachable!("invalid Dependency.Version.Tag {n}"), + n => { + ctx.log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!("Corrupt lockfile: invalid dependency version tag {n}"), + ); + Tag::Uninitialized + } }; let sliced = slice.sliced(ctx.buffer); parse_with_tag( diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index b188ca63635..223d2af3d9f 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -3463,7 +3463,29 @@ pub mod serializer { // TODO(port): assert_no_uninitialized_padding once a typed accessor lands. let end_pos = stream.pos + bytes.len(); if end_pos as u64 <= end_at { - bytes.copy_from_slice(&stream.buffer[stream.pos..stream.pos + bytes.len()]); + let src = &stream.buffer[stream.pos..stream.pos + bytes.len()]; + if matches!(field, PackageField::Resolution) { + // Validate the tag discriminant on the *raw stream bytes* + // before they are copied into the typed column. `ResolutionTag` + // is a `#[repr(u8)]` enum with non-contiguous discriminants + // (0,1,2,4,8,16,32,64,72,80,100); copying an out-of-range byte + // into `ResolutionType.tag` and then reading it would be + // immediate UB, and a `matches!` over all 11 typed variants is + // provably exhaustive and would be optimized away. Check the + // raw u8 here. Layout: `ResolutionType` is `#[repr(C)] + // { tag: Tag, _padding: [u8; 7], value: ... }`, so the + // discriminant is the first byte of each element. + let stride = mem::size_of::>(); + debug_assert!(stride != 0 && src.len() % stride == 0); + for raw in src.chunks_exact(stride) { + if !matches!(raw[0], 0 | 1 | 2 | 4 | 8 | 16 | 32 | 64 | 72 | 80 | 100) { + return Err(bun_core::err!( + "Lockfile validation failed: invalid resolution tag" + )); + } + } + } + bytes.copy_from_slice(src); stream.pos = end_pos; if matches!(field, PackageField::Meta) { // need to check if any values were created from an older version of bun diff --git a/src/install/npm.rs b/src/install/npm.rs index 04024fd8d86..a41044e0610 100644 --- a/src/install/npm.rs +++ b/src/install/npm.rs @@ -547,7 +547,10 @@ pub mod registry { 429 => return Err(err!("TooManyRequests")), 404 => return Ok(PackageVersionResponse::NotFound), 500..=599 => return Err(err!("HTTPInternalServerError")), - 304 => return Ok(PackageVersionResponse::Cached(loaded_manifest.unwrap())), + 304 => match loaded_manifest { + Some(manifest) => return Ok(PackageVersionResponse::Cached(manifest)), + None => return Err(err!("UnexpectedNotModified")), + }, _ => {} } @@ -659,6 +662,14 @@ pub struct PackageVersion { // Splitting this into it's own array ends up increasing the final size a little bit. pub integrity: Integrity, + // Explicit padding so this struct has no implicit (uninitialized) padding + // bytes — `Serializer::write_array` reinterprets the whole slice as `&[u8]`, + // and reading uninitialized padding as `u8` is UB. With explicit `[u8; N]` + // fields, `Default` zero-fills them and every byte of the struct is + // initialized. Layout (size=240, align=8) is unchanged; see the + // `offset_of!` asserts below and `padding_checker.rs` for the contract. + pub _padding_after_integrity: [u8; 3], + /// "dependencies"` in [package.json](https://docs.npmjs.com/cli/v8/configuring-npm/package-json#dependencies) pub dependencies: ExternalStringMap, @@ -685,6 +696,7 @@ pub struct PackageVersion { /// `"peerDependenciesMeta"` in [package.json](https://docs.npmjs.com/cli/v8/configuring-npm/package-json#peerdependenciesmeta) /// if `non_optional_peer_dependencies_start` is > 0, then instead of alphabetical, the first N items of `peer_dependencies` are optional pub non_optional_peer_dependencies_start: u32, + pub _padding_before_man_dir: [u8; 4], pub man_dir: ExternalString, @@ -705,6 +717,7 @@ pub struct PackageVersion { /// `hasInstallScript` field in registry API. pub has_install_script: bool, + pub _padding_tail: [u8; 2], /// Unix timestamp when this version was published (0 if unknown) pub publish_timestamp_ms: f64, @@ -714,6 +727,7 @@ impl Default for PackageVersion { fn default() -> Self { Self { integrity: Integrity::default(), + _padding_after_integrity: [0; 3], dependencies: ExternalStringMap::default(), optional_dependencies: ExternalStringMap::default(), peer_dependencies: ExternalStringMap::default(), @@ -722,6 +736,7 @@ impl Default for PackageVersion { bin: Bin::default(), engines: ExternalStringMap::default(), non_optional_peer_dependencies_start: 0, + _padding_before_man_dir: [0; 4], man_dir: ExternalString::default(), tarball_url: ExternalString::default(), unpacked_size: 0, @@ -730,6 +745,7 @@ impl Default for PackageVersion { cpu: Architecture::ALL, libc: Libc::NONE, has_install_script: false, + _padding_tail: [0; 2], publish_timestamp_ms: 0.0, } } @@ -765,6 +781,41 @@ const _: () = assert!( bump PackageManifest::Serializer::VERSION if intentional", ); +// Compile-time proof that the explicit `_padding_*` fields above leave no +// implicit padding gaps in `PackageVersion` (so `&[PackageVersion] as &[u8]` +// in `Serializer::write_array` reads only initialized bytes). Mirrors the +// `NpmPackage` checks below and `padding_checker.rs`. +const _: () = { + use core::mem::{offset_of, size_of}; + // gap between `integrity` (size 65, align 1) and `dependencies` (align 4 → 68) + assert!( + offset_of!(PackageVersion, _padding_after_integrity) + == offset_of!(PackageVersion, integrity) + size_of::() + ); + assert!( + offset_of!(PackageVersion, dependencies) + == offset_of!(PackageVersion, _padding_after_integrity) + 3 + ); + // gap between `non_optional_peer_dependencies_start` (u32, ends at 180) and `man_dir` (align 8 → 184) + assert!( + offset_of!(PackageVersion, _padding_before_man_dir) + == offset_of!(PackageVersion, non_optional_peer_dependencies_start) + size_of::() + ); + assert!( + offset_of!(PackageVersion, man_dir) + == offset_of!(PackageVersion, _padding_before_man_dir) + 4 + ); + // gap between `has_install_script` (bool, ends at 230) and `publish_timestamp_ms` (align 8 → 232) + assert!( + offset_of!(PackageVersion, _padding_tail) + == offset_of!(PackageVersion, has_install_script) + size_of::() + ); + assert!( + offset_of!(PackageVersion, publish_timestamp_ms) + == offset_of!(PackageVersion, _padding_tail) + 2 + ); +}; + // ────────────────────────────────────────────────────────────────────────── #[repr(C)] diff --git a/src/install/windows-shim/bun_shim_impl.rs b/src/install/windows-shim/bun_shim_impl.rs index 61e7e9967ac..6f895844ad6 100644 --- a/src/install/windows-shim/bun_shim_impl.rs +++ b/src/install/windows-shim/bun_shim_impl.rs @@ -992,6 +992,13 @@ fn launcher(bun_ctx: Ctx) -> LauncherRet // BUF1: '\??"C:\Users\chloe\project\node_modules\my-cli\src\app.js" --flag!!!!!' let argument_start_ptr: *mut u8 = read_ptr.cast::().wrapping_sub(2 * 1 /* "\x00".len */); + if (argument_start_ptr as usize) - (buf1_u8 as usize) + + user_arguments_u8.len() + + 2 /* "\x00".len */ + > BUF1_LEN * 2 + { + return LauncherMode::fail(MODE, FailReason::InvalidShimBounds); + } if !user_arguments_u8.is_empty() { // SAFETY: argument_start_ptr is within buf1 with room for user_arguments_u8. unsafe { @@ -1055,7 +1062,15 @@ fn launcher(bun_ctx: Ctx) -> LauncherRet const VALIDATION_LENGTH_OFFSET: u64 = 14; // very careful here to not overflow u32, so that we properly error if you hijack the file + // + // Both lengths are byte counts of UTF-16 data written by + // BinLinkingShim.zig and must be even. An odd value (corrupt or + // tampered shim) would later misalign `write_ptr` (used to store a + // u16 NUL terminator) and break the even-length invariant relied on + // by `bytemuck::cast_slice`. Reject up front. if shebang_arg_len_u8 == 0 + || (shebang_arg_len_u8 & 1) != 0 + || (shebang_bin_path_len_bytes & 1) != 0 || (shebang_arg_len_u8 as u64).saturating_add(shebang_bin_path_len_bytes as u64) + VALIDATION_LENGTH_OFFSET != read_len as u64 @@ -1114,7 +1129,29 @@ fn launcher(bun_ctx: Ctx) -> LauncherRet .cast::() .wrapping_sub(shebang_arg_len_u8 as usize) .cast::(); - // SAFETY: copying shebang_arg_len_u8 bytes from buf1 into buf2; both in bounds. + + // Compute the filename length and perform the BUF2 capacity check + // *before* the first write into buf2 so a hostile metadata file can + // never overrun it (the lengths feeding `advance` below come from + // the untrusted shim, not from anything we control). + // + // BUF1: '\??\C:\Users\chloe\project\node_modules\my-cli\src\app.js"#node #####!!!!!!!!!!' + // ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ ^ read_ptr + let length_of_filename_u8 = (read_ptr as usize) + - (buf1_u8 as usize) + - 2 * (NT_OBJECT_PREFIX.len() + 1/* "\x00".len */); + if shebang_arg_len_u8 as usize + + 2 /* "\"".len */ + + length_of_filename_u8 + + user_arguments_u8.len() + + 2 /* "\x00".len */ + > BUF2_U16_LEN * 2 + { + return LauncherMode::fail(MODE, FailReason::InvalidShimBounds); + } + + // SAFETY: copying shebang_arg_len_u8 bytes from buf1 into buf2; both in bounds + // (capacity validated above). unsafe { core::ptr::copy_nonoverlapping( read_ptr.cast::(), @@ -1136,9 +1173,6 @@ fn launcher(bun_ctx: Ctx) -> LauncherRet // BUF1: '\??\C:\Users\chloe\project\node_modules\my-cli\src\app.js"#node #####!!!!!!!!!!' // ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ ^ read_ptr // BUF2: 'node "C:\Users\chloe\project\node_modules\my-cli\src\app.js"!!!!!!!!!!!!!!!!!!!!' - let length_of_filename_u8 = (read_ptr as usize) - - (buf1_u8 as usize) - - 2 * (NT_OBJECT_PREFIX.len() + 1/* "\x00".len */); // SAFETY: slice within buf1. let filename: &[u8] = unsafe { bun_core::ffi::slice( @@ -1218,8 +1252,11 @@ fn launcher(bun_ctx: Ctx) -> LauncherRet // BUF2: 'node "C:\Users\chloe\project\node_modules\my-cli\src\app.js" --flags#!!!!!!!!!!' // ^ null terminator - // SAFETY: write_ptr is within buf2. - unsafe { *write_ptr = 0 }; + // SAFETY: write_ptr is within buf2 (capacity validated above). Use an + // unaligned store as defense in depth: alignment is guaranteed by the + // even-parity metadata checks, but this avoids UB if that invariant + // ever regresses. + unsafe { write_ptr.write_unaligned(0) }; break 'spawn_command_line buf2_u16; } diff --git a/src/libarchive/lib.rs b/src/libarchive/lib.rs index 6651e49ea3c..dfa3bdb94fa 100644 --- a/src/libarchive/lib.rs +++ b/src/libarchive/lib.rs @@ -1316,6 +1316,30 @@ fn is_symlink_target_safe( resolved.starts_with(fake_root) } +/// Returns true if any leading component of `path` (including the full path) +/// matches a symlink already created by this extraction. `is_symlink_target_safe` +/// is purely lexical, so once a symlink is on disk the kernel will follow it +/// during later `mkdirat`/`openat`/`symlinkat` calls — such entries must be +/// rejected rather than resolved. +#[cfg(unix)] +fn path_traverses_created_symlink(path: &[u8], created_symlinks: &[Vec]) -> bool { + if created_symlinks.is_empty() { + return false; + } + let sep = b'/'; + let mut end = 0usize; + while end <= path.len() { + if end == path.len() || path[end] == sep { + let prefix = &path[..end]; + if !prefix.is_empty() && created_symlinks.iter().any(|s| s.as_slice() == prefix) { + return true; + } + } + end += 1; + } + false +} + /// Port of `bun.MakePath.makePath(u16, dir, sub_path)` (bun.zig:2481) — the /// Windows arm calls `makeOpenPathAccessMaskW`, which component-iterates the /// wide path and `NtCreateFile`s each prefix with `FILE_OPEN_IF`, walking back @@ -1617,6 +1641,9 @@ impl Archiver { let mut symlink_join_buf: Option = None; // (guard Drop puts the buffer back to the pool) + #[cfg(unix)] + let mut created_symlinks: Vec> = Vec::new(); + let mut normalized_buf = OSPathBuffer::uninit(); let mut use_pwrite = cfg!(unix); let mut use_lseek = true; @@ -1778,6 +1805,17 @@ impl Archiver { let path_slice: &[OSPathChar] = &path[..]; + #[cfg(unix)] + if path_traverses_created_symlink(path_slice, &created_symlinks) { + if options.log { + Output::warn(&format_args!( + "Skipping entry that traverses a previously extracted symlink: {}\n", + bun_core::fmt::fmt_os_path(path_slice, Default::default()), + )); + } + continue; + } + if options.log { bun_core::prettyln!( " {}", @@ -1889,6 +1927,7 @@ impl Archiver { _ => return Err(err.into()), }, } + created_symlinks.push(path_slice.to_vec()); } #[cfg(not(unix))] { diff --git a/src/parsers/toml.rs b/src/parsers/toml.rs index 3f462d604e7..34e721d7d52 100644 --- a/src/parsers/toml.rs +++ b/src/parsers/toml.rs @@ -356,12 +356,24 @@ impl<'a> TOML<'a> { let head: *mut Rope = rope; let mut rope: *mut Rope = rope; + // Hard cap on dotted-key segments. The rope is consumed by `set_rope`, + // `get_or_put_array`, and `get_or_put_object`, each of which recurses + // once per `rope.next` link with no stack guard of their own. + const MAX_DOTTED_KEY_SEGMENTS: usize = 512; + let mut segments: usize = 1; + while self.lexer.token == T::t_dot { self.lexer.next()?; let Some(seg) = self.parse_key_segment()? else { break; }; + segments += 1; + if segments > MAX_DOTTED_KEY_SEGMENTS { + self.lexer + .add_default_error(b"Dotted key has too many segments")?; + return Err(bun_core::err!("SyntaxError")); + } // SAFETY: `rope` points into `bump` and is live for this call; we are // the sole mutator. Raw pointers used to avoid stacked &mut reborrows. // PORT NOTE: reshaped for borrowck diff --git a/src/parsers/yaml.rs b/src/parsers/yaml.rs index e9fb12489b3..d234ddc322d 100644 --- a/src/parsers/yaml.rs +++ b/src/parsers/yaml.rs @@ -1407,9 +1407,9 @@ impl<'i, Enc: Encoding> ScalarResolverCtx<'i, Enc> { _ => {} }, FirstChar::Negative | FirstChar::Positive => { - // PORT NOTE: Zig `a == b and c or d` parses as `(a == b and c) or d`. - if Enc::wide(parser!().next()) == 0x2E && Enc::wide(parser!().peek(1)) == 0x69 - || Enc::wide(parser!().peek(1)) == 0x49 + if Enc::wide(parser!().next()) == 0x2E + && (Enc::wide(parser!().peek(1)) == 0x69 + || Enc::wide(parser!().peek(1)) == 0x49) { self.append_source(Enc::ch(b'.'), parser!().pos)?; parser!().inc(1); diff --git a/src/runtime/api/BunObject.rs b/src/runtime/api/BunObject.rs index 33566197028..a90cb009c95 100644 --- a/src/runtime/api/BunObject.rs +++ b/src/runtime/api/BunObject.rs @@ -532,6 +532,16 @@ pub fn braces( return bun_string_jsc::to_js_array(global, &[brace_str]); } + // Hard cap before preallocation: `calculate_expanded_amount` saturates to + // `u32::MAX`, so a tiny nested input can otherwise request a huge `Vec`. + const MAX_BRACE_EXPANSIONS: u32 = 65536; + if expansion_count > MAX_BRACE_EXPANSIONS { + return Err(global.throw_pretty(format_args!( + "Too many brace expansions ({} > {})", + expansion_count, MAX_BRACE_EXPANSIONS + ))); + } + // Non-AST crate: result containers use plain Vec (arena is only for Braces::* internals). let expansion_count = expansion_count as usize; let mut expanded_strings: Vec> = Vec::with_capacity(expansion_count); @@ -548,7 +558,9 @@ pub fn braces( Ok(()) => {} Err(Braces::ParserError::OutOfMemory) => return Err(jsc::JsError::OutOfMemory), Err(Braces::ParserError::UnexpectedToken) => { - return Err(global.throw_pretty(format_args!("Unexpected token while expanding braces"))); + return Err( + global.throw_pretty(format_args!("Unexpected token while expanding braces")) + ); } } @@ -1030,7 +1042,8 @@ pub fn open_in_editor(global_this: &JSGlobalObject, callframe: &CallFrame) -> Js // directly into the persistent EditorContext (latent // UAF in spec). Own the bytes in `name_storage` and // hand back a thread-lifetime borrow. - slot.name_storage = sliced.slice().to_vec(); + let prev_storage = + core::mem::replace(&mut slot.name_storage, sliced.slice().to_vec()); // SAFETY: `name_storage` lives in a thread_local that // outlives any caller; we never reallocate it while // `edit.name` is observed (single-threaded JS VM). @@ -1039,6 +1052,7 @@ pub fn open_in_editor(global_this: &JSGlobalObject, callframe: &CallFrame) -> Js edit.detect_editor(env); editor_choice = edit.editor; if editor_choice.is_none() { + slot.name_storage = prev_storage; *edit = prev; return Err(global_this.throw(format_args!( "Could not find editor \"{}\"", diff --git a/src/runtime/cli/bunx_command.rs b/src/runtime/cli/bunx_command.rs index 21f16743f46..434da147388 100644 --- a/src/runtime/cli/bunx_command.rs +++ b/src/runtime/cli/bunx_command.rs @@ -526,6 +526,48 @@ impl BunxCommand { } } + /// Refuse to execute a binary resolved from inside the bunx cache unless + /// it is owned by the current user. + /// + /// The bunx cache lives under the world-writable temp dir at a predictable + /// path. Another local user could pre-create that path. Bun's bin linker + /// creates `.bin/` entries as *symlinks* on Unix + /// (`Linker::create_symlink`), so a regular-file-only check would mark every + /// legitimate cache hit as untrusted and reinstall on every invocation. + /// Accept either a symlink or a regular file owned by the current uid; for + /// symlinks, also follow once and require the target to be a uid-owned + /// regular file so an attacker-planted, uid-matching link can't redirect + /// execution outside the cache. + /// + /// On non-Unix targets there is no comparable shared world-writable temp + /// dir / uid model, so the check is a no-op there. + #[cfg(unix)] + fn is_trusted_cached_binary(destination: &ZStr, uid: libc::uid_t) -> bool { + let lstat_ok = |st: &bun_sys::Stat| { + let kind = st.st_mode & libc::S_IFMT; + st.st_uid == uid && (kind == libc::S_IFREG || kind == libc::S_IFLNK) + }; + let stat_ok = |st: &bun_sys::Stat| { + st.st_uid == uid && (st.st_mode & libc::S_IFMT) == libc::S_IFREG + }; + match bun_sys::lstat(destination) { + Ok(st) if lstat_ok(&st) => { + if (st.st_mode & libc::S_IFMT) == libc::S_IFLNK { + matches!(bun_sys::stat(destination), Ok(target) if stat_ok(&target)) + } else { + true + } + } + _ => false, + } + } + + #[cfg(not(unix))] + #[inline(always)] + fn is_trusted_cached_binary(_destination: &ZStr, _uid: u32) -> bool { + true + } + fn exit_with_usage() -> ! { crate::cli::command::tag_print_help(Command::Tag::BunxCommand, false); Global::exit(1); @@ -923,6 +965,19 @@ impl BunxCommand { // If this directory was installed by bunx, we want to perform cache invalidation on it // this way running `bunx hello` will update hello automatically to the latest version if strings::has_prefix(out, bunx_cache_dir) { + // Refuse to execute a cached binary that wasn't created by the + // current user (another local user could have pre-created the + // path); fall through to a fresh install instead. See + // `is_trusted_cached_binary` for the full rationale. + if !Self::is_trusted_cached_binary(destination, uid) { + bun_output::scoped_log!( + bunx, + "refusing untrusted cached binary: {}", + BStr::new(out) + ); + do_cache_bust = true; + break 'try_run_existing; + } let is_stale: bool = 'is_stale: { #[cfg(windows)] { @@ -1078,6 +1133,22 @@ impl BunxCommand { }; if let Some(destination) = dest_or_cache2 { let out: &[u8] = destination.as_bytes(); + // Same hardening as the first cache probe: this path + // resolves the package's *real* bin name (which may + // differ from the package name), so it is just as + // reachable for a binary planted by another local user + // in the world-writable bunx cache. + if strings::has_prefix(out, bunx_cache_dir) + && !Self::is_trusted_cached_binary(destination, uid) + { + bun_output::scoped_log!( + bunx, + "refusing untrusted cached binary: {}", + BStr::new(out) + ); + do_cache_bust = true; + break 'try_run_existing; + } let stored = fs.dirname_store.append_slice(out)?; Run::run_binary( ctx, @@ -1325,17 +1396,29 @@ impl BunxCommand { absolute_in_cache_dir, ) { let out: &[u8] = destination.as_bytes(); - let stored = fs.dirname_store.append_slice(out)?; - Run::run_binary( - ctx, - stored, - destination, - top_level_dir, - env_loader, - passthrough, - None, - )?; - // run_binary is noreturn + // The install we just ran should have created this symlink as the + // current user, but the cache lives in a world-writable temp dir; an + // attacker can race the install and plant a uid-mismatched entry. + // Bail out to the generic error rather than execute it. + if Self::is_trusted_cached_binary(destination, uid) { + let stored = fs.dirname_store.append_slice(out)?; + Run::run_binary( + ctx, + stored, + destination, + top_level_dir, + env_loader, + passthrough, + None, + )?; + // run_binary is noreturn + } else { + bun_output::scoped_log!( + bunx, + "refusing untrusted cached binary: {}", + BStr::new(out) + ); + } } // 2. The "bin" is possibly not the same as the package name, so we load the package.json to figure out what "bin" to use @@ -1376,17 +1459,26 @@ impl BunxCommand { absolute_in_cache_dir, ) { let out: &[u8] = destination.as_bytes(); - let stored = fs.dirname_store.append_slice(out)?; - Run::run_binary( - ctx, - stored, - destination, - top_level_dir, - env_loader, - passthrough, - None, - )?; - // run_binary is noreturn + // Same TOCTOU hardening as the post-install probe above. + if Self::is_trusted_cached_binary(destination, uid) { + let stored = fs.dirname_store.append_slice(out)?; + Run::run_binary( + ctx, + stored, + destination, + top_level_dir, + env_loader, + passthrough, + None, + )?; + // run_binary is noreturn + } else { + bun_output::scoped_log!( + bunx, + "refusing untrusted cached binary: {}", + BStr::new(out) + ); + } } } } diff --git a/src/runtime/crypto/PBKDF2.rs b/src/runtime/crypto/PBKDF2.rs index 42ddb2aae4f..03bf9dfb390 100644 --- a/src/runtime/crypto/PBKDF2.rs +++ b/src/runtime/crypto/PBKDF2.rs @@ -162,6 +162,7 @@ impl PBKDF2 { match evp::lookup_ignore_case(slice.slice()) { Some(alg) => match alg { Algorithm::Shake128 | Algorithm::Shake256 => break 'invalid, + other if other.md().is_none() => break 'invalid, other => break 'brk other, }, None => break 'invalid, diff --git a/src/runtime/node/node_crypto_binding.rs b/src/runtime/node/node_crypto_binding.rs index 4e24df04289..b6f5f691444 100644 --- a/src/runtime/node/node_crypto_binding.rs +++ b/src/runtime/node/node_crypto_binding.rs @@ -244,6 +244,13 @@ pub mod random { pub bytes: *mut u8, pub offset: u32, pub length: usize, + // Worker-owned destination for user-supplied buffers (`randomFill`). + // The user can detach (`transfer()`) or shrink (`resize()`) the backing + // store between scheduling and the WorkPool write, so the worker fills + // this scratch and `run_from_js` re-validates + copies on the JS thread. + // `randomBytes` allocates its own buffer (unreachable from JS until the + // callback fires) and leaves this `None`. + pub scratch: Option>, pub result: (), // void } @@ -263,8 +270,14 @@ pub mod random { } fn run_task(&mut self) { + if let Some(scratch) = &mut self.scratch { + bun_core::csprng(scratch); + return; + } // SAFETY: `bytes` points into an ArrayBuffer kept alive by `self.value` // (protected in `init`); offset+length were range-checked by callers. + // This branch is only used for internally-allocated buffers that JS + // cannot reach (and therefore cannot detach/resize) until `run_from_js`. let slice = unsafe { core::slice::from_raw_parts_mut(self.bytes.add(self.offset as usize), self.length) }; @@ -272,6 +285,22 @@ pub mod random { } fn run_from_js(&mut self, global: &JSGlobalObject, callback: JSValue) { + if let Some(scratch) = self.scratch.take() { + // Re-fetch the buffer on the JS thread and re-validate bounds: + // the user may have detached or resized it while the WorkPool + // task ran. On mismatch, drop the random bytes rather than + // write through a stale pointer. + if let Some(mut buf) = self.value.as_array_buffer(global) { + let off = self.offset as usize; + let dst = buf.slice_mut(); + match off.checked_add(scratch.len()) { + Some(end) if end <= dst.len() => { + dst[off..end].copy_from_slice(&scratch); + } + _ => {} + } + } + } // `bun_vm()` is the audited safe `&'static VirtualMachine` accessor; // `event_loop_mut()` is the audited safe `&mut EventLoop` accessor. global.bun_vm().event_loop_mut().run_callback( @@ -533,6 +562,7 @@ pub mod random { bytes: bytes.as_mut_ptr(), offset: 0, length: size as usize, + scratch: None, result: (), }; crypto_job_init_and_schedule(global, callback, ctx)?; @@ -592,7 +622,7 @@ pub mod random { let [buf_value, mut offset_value, mut size_value, mut callback] = call_frame.arguments_as_array::<4>(); - let Some(mut buf) = buf_value.as_array_buffer(global) else { + let Some(buf) = buf_value.as_array_buffer(global) else { return Err(global.throw_invalid_argument_type_value( b"buf", b"ArrayBuffer or ArrayBufferView", @@ -636,11 +666,22 @@ pub mod random { return Ok(JSValue::UNDEFINED); } + // `vec![0u8; size]` aborts the process on OOM. The 3-arg overload + // `randomFill(buf, offset, cb)` defaults `size` to the full + // remaining buffer length, which can exceed allocator limits for a + // multi-GiB ArrayBuffer — surface that as a JS error instead. + let mut scratch = Vec::new(); + if scratch.try_reserve_exact(size).is_err() { + return Err(global.throw_out_of_memory()); + } + scratch.resize(size, 0); + let ctx = JobCtx { value: buf_value, - bytes: buf.slice_mut().as_mut_ptr(), + bytes: core::ptr::null_mut(), offset, length: size, + scratch: Some(scratch), result: (), }; crypto_job_init_and_schedule(global, callback, ctx)?; diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index e3077db6a65..c94e3b882e8 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3454,16 +3454,23 @@ where if !basename.is_empty() { let mut filename_buf = [0u8; 1024]; let truncated = &basename[..basename.len().min(1024 - 32)]; - let header_value = { - let mut w = &mut filename_buf[..]; - if write!(w, "filename=\"{}\"", bstr::BStr::new(truncated)).is_ok() { - let written = 1024 - w.len(); - &filename_buf[..written] - } else { - &b""[..] + if !truncated + .iter() + .any(|&b| matches!(b, b'\r' | b'\n' | 0 | b'"')) + { + let header_value = { + let mut w = &mut filename_buf[..]; + if write!(w, "filename=\"{}\"", bstr::BStr::new(truncated)).is_ok() { + let written = 1024 - w.len(); + &filename_buf[..written] + } else { + &b""[..] + } + }; + if !header_value.is_empty() { + resp.write_header(b"content-disposition", header_value); } - }; - resp.write_header(b"content-disposition", header_value); + } } } } diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 42f2795a062..b23a04ae358 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -3533,7 +3533,7 @@ where } // IPv6 loopback addresses if address.ip.starts_with(b"::ffff:127.") - || address.ip.starts_with(b"::1") + || address.ip == b"::1" || address.ip == b"0:0:0:0:0:0:0:1" { break 'brk true; diff --git a/src/runtime/shell/states/Expansion.rs b/src/runtime/shell/states/Expansion.rs index 5ddaed9d4e4..d732bc1d442 100644 --- a/src/runtime/shell/states/Expansion.rs +++ b/src/runtime/shell/states/Expansion.rs @@ -265,7 +265,16 @@ impl Expansion { braces::NewLexer::<{ braces::StringEncoding::Wtf8 }>::tokenize(brace_str), ) }; - let count = braces::calculate_expanded_amount(&lexer_output.tokens[..]) as usize; + let count = braces::calculate_expanded_amount(&lexer_output.tokens[..]); + // Hard cap before preallocation: `calculate_expanded_amount` saturates + // to `u32::MAX`, so a tiny nested input can otherwise request a huge `Vec`. + const MAX_BRACE_EXPANSIONS: u32 = 65536; + if count > MAX_BRACE_EXPANSIONS { + let msg = format!("too many brace expansions ({count} > {MAX_BRACE_EXPANSIONS})"); + me.state = ExpansionState::Err(ShellErr::Custom(msg.into_bytes().into())); + return; + } + let count = count as usize; let mut expanded: Vec> = (0..count).map(|_| Vec::new()).collect(); let arena = bun_alloc::Arena::new(); diff --git a/src/runtime/socket/Listener.rs b/src/runtime/socket/Listener.rs index 93a2a628933..01fc91c9b55 100644 --- a/src/runtime/socket/Listener.rs +++ b/src/runtime/socket/Listener.rs @@ -1086,8 +1086,12 @@ impl Listener { // SAFETY: caller passes a live TLSSocket let prev = unsafe { &*prev_ptr }; if let Some(prev_handlers) = prev.handlers.get() { - // SAFETY: prev_handlers was heap-allocated - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + // SAFETY: prev_handlers was heap-allocated; shared + // reborrow is scoped to this expression. + if unsafe { (*prev_handlers.as_ptr()).active_connections.get() } == 0 { + // SAFETY: prev_handlers was heap-allocated and unreferenced. + unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + } } debug_assert!(!prev.this_value.get().is_empty()); prev.handlers.set(NonNull::new(handlers_ptr)); @@ -1175,8 +1179,12 @@ impl Listener { let prev = unsafe { &*prev_ptr }; debug_assert!(!prev.this_value.get().is_empty()); if let Some(prev_handlers) = prev.handlers.get() { - // SAFETY: prev_handlers was heap-allocated - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + // SAFETY: prev_handlers was heap-allocated; shared + // reborrow is scoped to this expression. + if unsafe { (*prev_handlers.as_ptr()).active_connections.get() } == 0 { + // SAFETY: prev_handlers was heap-allocated and unreferenced. + unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + } } prev.handlers.set(NonNull::new(handlers_ptr)); debug_assert!(matches!( @@ -1417,11 +1425,19 @@ fn connect_finish( let prev = unsafe { &*prev_ptr }; // TODO(port): `JsRef::is_not_empty` — assert non-empty wrapper. if let Some(prev_handlers) = prev.handlers.get() { - // SAFETY: prev_handlers was heap-allocated - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + // Only free the previous Handlers when no callback scope is still + // holding it. If a `data`/`close` handler synchronously re-entered + // `connect`, `Scope::exit` (via `Handlers::mark_inactive`) frees it + // once the in-flight callback unwinds; freeing here would be a UAF. + // SAFETY: prev_handlers was heap-allocated; shared reborrow is + // scoped to this expression. + if unsafe { (*prev_handlers.as_ptr()).active_connections.get() } == 0 { + // SAFETY: prev_handlers was heap-allocated and unreferenced. + unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; + } } prev.handlers.set(NonNull::new(handlers_ptr)); - // TODO(port): debug_assert!(matches!(prev.socket.get().socket, InternalSocket::Detached)) + debug_assert!(prev.socket.get().is_detached()); // Free old resources before reassignment to prevent memory leaks // when sockets are reused for reconnection (common with MongoDB driver) prev.connection.set(Some(connection)); @@ -1555,8 +1571,7 @@ fn is_valid_pipe_name(pipe_name: &[u8]) -> bool { fn normalize_pipe_name<'a>(pipe_name: &[u8], buffer: &'a mut [u8]) -> Option<&'a [u8]> { #[cfg(windows)] { - debug_assert!(pipe_name.len() < buffer.len()); - if !is_valid_pipe_name(pipe_name) { + if pipe_name.len() > buffer.len() || !is_valid_pipe_name(pipe_name) { return None; } // normalize pipe name with can have mixed slashes diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index d6a822904d2..3f7cd60917b 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -892,16 +892,31 @@ impl NewSocket { // moved to a guard so all early-returns run them. The outer // `_outer_deref` above owns the final `deref()`; LIFO drop order // (cleanup → _outer_deref) mirrors Zig's three defers exactly. - let cleanup = scopeguard::guard((this.as_ctx_ptr(), needs_deref), |(p, nd)| { - // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. - unsafe { - // Zig defer order (reverse-declaration): needs_deref → markInactive. - if nd { - (*p).deref(); + // + // The deferred `mark_inactive()` is gated on the `Handlers` pointer + // captured before the user callback runs: `onConnectError` can + // synchronously re-enter `connect()` and — via `do_connect()`'s + // `UnixOrHost::Fd` branch — reach `on_open()`/`mark_active()` for a + // *fresh* `Handlers` allocation before this guard drops. Without the + // gate the deferred `mark_inactive()` would tear down that newly + // activated connection. When no reconnect happened the socket never + // opened, so `IS_ACTIVE` is unset and the call is a no-op either way. + let pre_callback_handlers = handlers.as_ptr(); + let cleanup = scopeguard::guard( + (this.as_ctx_ptr(), needs_deref, pre_callback_handlers), + |(p, nd, h)| { + // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. + unsafe { + // Zig defer order (reverse-declaration): needs_deref → markInactive. + if nd { + (*p).deref(); + } + if (*p).handlers.get().map(|n| n.as_ptr()) == Some(h) { + (*p).mark_inactive(); + } } - (*p).mark_inactive(); - } - }); + }, + ); if vm.is_shutting_down() { drop(cleanup); @@ -944,21 +959,33 @@ impl NewSocket { // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can + let captured_handlers = handlers.as_ptr(); let scope = Handlers::enter_ref(handlers); // PORT NOTE: `let _ = guard` would drop *immediately* (end of // statement, not end of scope) and run `scope.exit()` before the // user's onConnectError callback. Bind to a named `_`-prefixed // local so it lives to end of scope like Zig's `defer`. - let _scope_guard = scopeguard::guard((this.as_ctx_ptr(), scope), |(p, mut sc)| { - if sc.exit() { - // Connection never opened (`is_active == false`), so the - // scope's decrement is what brings client handlers to zero - // and frees them. Null the field so a retry via - // `connectInner` doesn't double-free. - // SAFETY: `p` is the live `*mut Self`. - unsafe { (*p).handlers.set(None) }; - } - }); + let _scope_guard = + scopeguard::guard((this.as_ctx_ptr(), scope, captured_handlers), |(p, mut sc, h)| { + if sc.exit() { + // Connection never opened (`is_active == false`), so the + // scope's decrement is what brings client handlers to zero + // and frees them. Null the field so a retry via + // `connectInner` doesn't double-free — but only if the + // cell still points at the Handlers `exit()` just freed: + // the `connectError` callback may have synchronously + // re-entered `connect()` (node:net `autoSelectFamily` + // retries) which already repointed the cell at a fresh + // allocation, and nulling it here would orphan the new + // Handlers and make the retry's `on_open` panic. + // SAFETY: `p` is the live `*mut Self`. + unsafe { + if (*p).handlers.get().map(|n| n.as_ptr()) == Some(h) { + (*p).handlers.set(None); + } + } + } + }); if callback.is_empty() { // Connection failed before open; allow the wrapper to be GC'd @@ -1491,10 +1518,49 @@ impl NewSocket { unsafe { Self::on_close(raw, socket, err, reason).ok() }; } // PORT NOTE: reshaped for borrowck — `defer this.deref()` + `defer markInactive()`. - let cleanup = scopeguard::guard(this.as_ctx_ptr(), |p| { + // Capture the `Handlers` pointer that was paired with the + // `IS_ACTIVE` flag *before* the user's close callback runs. The + // callback may synchronously re-enter `Bun.connect({ socket: this })` + // (the documented MongoDB-driver reconnect path), which makes + // `connect_finish` repoint `self.handlers` at a fresh allocation + // while keeping the old one alive for the in-flight `Scope`. If the + // deferred `mark_inactive()` re-read the cell at that point it would + // (a) underflow the new `Handlers`' counter (created with + // `active_connections == 0`) and (b) leak the old one with its + // `protect()`'d JS callbacks orphaned at count 1. + let captured_handlers = handlers.as_ptr(); + let cleanup = scopeguard::guard((this.as_ctx_ptr(), captured_handlers), |(p, h)| { // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. unsafe { - (*p).mark_inactive(); + let this_ref = &*p; + if this_ref.handlers.get().map(|n| n.as_ptr()) == Some(h) { + // Normal close: the cell still points at the Handlers we + // captured; do the full idle teardown. + this_ref.mark_inactive(); + } else if this_ref.flags.get().contains(Flags::IS_ACTIVE) { + // The close callback synchronously reconnected. The + // socket is not going idle — `connect_finish` already + // re-upgraded `this_value` and re-armed `poll_ref` for + // the in-flight connect — so skip the idle teardown. + // Just clear `IS_ACTIVE` (the next `on_open` re-arms it + // against the new Handlers) and release the lifecycle + // ref `mark_active` took on the *captured* Handlers so + // it can reach zero in `Scope::exit` instead of leaking. + this_ref.update_flags(|f| f.remove(Flags::IS_ACTIVE)); + let vm = VirtualMachine::get(); + // SAFETY: VM singleton is always live once initialized. + if !(*vm).is_shutting_down() { + // SAFETY: `h` is still live. The lifecycle ref + // released here is the one `mark_active` took at + // `on_open`, so `active_connections >= 1` until this + // call. `connect_finish` saw a non-zero count and + // therefore kept `h` allocated; `scope.exit()` (which + // ran just before this guard dropped) decremented the + // `enter_ref` ref but cannot have freed `h` while this + // lifecycle ref is outstanding. + let _ = Handlers::mark_inactive(h); + } + } (*p).deref(); } }); @@ -1538,7 +1604,14 @@ impl NewSocket { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(e)]); } if scope.exit() { - this.handlers.set(None); + // Only null if the cell still points at the Handlers `exit()` + // just freed — a synchronous reconnect from inside the close + // callback already repointed it at a fresh allocation, and + // nulling it here would orphan the new Handlers and make the + // pending `on_open`'s `get_handlers()` panic on `None`. + if this.handlers.get().map(|n| n.as_ptr()) == Some(captured_handlers) { + this.handlers.set(None); + } } drop(cleanup); Ok(()) diff --git a/src/runtime/socket/udp_socket.rs b/src/runtime/socket/udp_socket.rs index 745d826d902..6ae8e62d365 100644 --- a/src/runtime/socket/udp_socket.rs +++ b/src/runtime/socket/udp_socket.rs @@ -1636,10 +1636,11 @@ impl UDPSocket { if this.closed.get() { return JSValue::UNDEFINED; } + let Some(socket) = this.socket.get() else { + return JSValue::UNDEFINED; + }; // `Socket` is an `opaque_ffi!` ZST — `opaque_mut` is the safe deref. - JSValue::js_number( - uws::udp::Socket::opaque_mut(this.socket.get().unwrap()).bound_port() as f64, - ) + JSValue::js_number(uws::udp::Socket::opaque_mut(socket).bound_port() as f64) } fn create_sock_addr(global_this: &JSGlobalObject, address_bytes: &[u8], port: u16) -> JSValue { @@ -1655,10 +1656,13 @@ impl UDPSocket { if this.closed.get() { return JSValue::UNDEFINED; } + let Some(socket) = this.socket.get() else { + return JSValue::UNDEFINED; + }; let mut buf = [0u8; 64]; let mut length: i32 = 64; // `Socket` is an `opaque_ffi!` ZST — `opaque_mut` is the safe deref. - let socket = uws::udp::Socket::opaque_mut(this.socket.get().unwrap()); + let socket = uws::udp::Socket::opaque_mut(socket); socket.bound_ip(buf.as_mut_ptr(), &mut length); let address_bytes = &buf[..usize::try_from(length).expect("int cast")]; @@ -1678,11 +1682,13 @@ impl UDPSocket { let Some(connect_info) = this.connect_info.get() else { return JSValue::UNDEFINED; }; + let Some(socket) = this.socket.get() else { + return JSValue::UNDEFINED; + }; let mut buf = [0u8; 64]; let mut length: i32 = 64; // `Socket` is an `opaque_ffi!` ZST — `opaque_mut` is the safe deref. - uws::udp::Socket::opaque_mut(this.socket.get().unwrap()) - .remote_ip(buf.as_mut_ptr(), &mut length); + uws::udp::Socket::opaque_mut(socket).remote_ip(buf.as_mut_ptr(), &mut length); let address_bytes = &buf[..usize::try_from(length).expect("int cast")]; Self::create_sock_addr(global_this, address_bytes, connect_info.port) diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 1faa42251b1..bc2e43c7a48 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -1729,7 +1729,11 @@ impl BlobExt for Blob { let proxy_url: Option> = unsafe { (*global_this.bun_vm().as_mut().transpiler.env).get_http_proxy(true, None, None) }; - let proxy = proxy_url.as_ref().map(|p| p.href); + // Copy the href out of the env map before any reentrant JS (the + // `get_truthy`/credential getters below) can mutate `process.env` + // and free the backing allocation. + let proxy_owned: Option> = proxy_url.as_ref().map(|p| p.href.to_vec()); + let proxy = proxy_owned.as_deref(); if has_args && arg0.is_object() { let options = arg0; diff --git a/src/runtime/webcore/Crypto.rs b/src/runtime/webcore/Crypto.rs index 272d96aad04..e21e66c49d2 100644 --- a/src/runtime/webcore/Crypto.rs +++ b/src/runtime/webcore/Crypto.rs @@ -1,5 +1,3 @@ -use core::slice; - use bun_core::String as BunString; use bun_jsc::uuid::{self, UUID, UUID5, UUID7}; use bun_jsc::{ @@ -243,8 +241,9 @@ impl Crypto { // SAFETY: a_ptr/b_ptr are valid for `len` bytes (just obtained from JSUint8Array; // `JSUint8Array::slice()` needs `&mut self`, so reconstruct the slices here). - let a = unsafe { slice::from_raw_parts(a_ptr, len) }; - let b = unsafe { slice::from_raw_parts(b_ptr, len) }; + // `ffi::slice` tolerates `(null, 0)` for detached/empty arrays. + let a = unsafe { bun_core::ffi::slice(a_ptr, len) }; + let b = unsafe { bun_core::ffi::slice(b_ptr, len) }; JSValue::from(bun_boringssl_sys::constant_time_eq(a, b)) } diff --git a/src/runtime/webcore/fetch.rs b/src/runtime/webcore/fetch.rs index 07e6a7abf61..b50bcc9d739 100644 --- a/src/runtime/webcore/fetch.rs +++ b/src/runtime/webcore/fetch.rs @@ -1643,16 +1643,11 @@ fn fetch_impl( // `vm.node_fs()` accessor is gated behind a jsc↔runtime cycle and // the buffer is just NUL-termination scratch). let mut open_path_buf = PathBuffer::uninit(); - let pathlike_is_path: bool; let opened_fd_res: bun_sys::Result = { let store = body.store().expect("needs_to_read_file implies store"); match &store.data.as_file().pathlike { - PathOrFileDescriptor::Fd(fd) => { - pathlike_is_path = false; - bun_sys::dup(*fd) - } + PathOrFileDescriptor::Fd(fd) => bun_sys::dup(*fd), PathOrFileDescriptor::Path(path) => { - pathlike_is_path = true; let zpath = path.slice_z(&mut open_path_buf); let flags = if cfg!(windows) { bun_sys::O::RDONLY @@ -1733,6 +1728,14 @@ fn fetch_impl( } } + // The sendfile path above moves `opened_fd` into `SendFile` (which + // owns its lifecycle and breaks out of `'prepare_body`). On this + // read-file path we are the sole owner of the fresh fd: `read_file` + // is handed it as an `Fd` and never takes ownership. Wrap it in an + // RAII guard so any future early return between here and the read + // can't leak it. + let opened_fd = scopeguard::guard(opened_fd, |fd| fd.close()); + // TODO: make this async + lazy let blob_offset = body.any_blob().blob().offset.get(); let blob_size = body.any_blob().blob().size.get(); @@ -1744,14 +1747,14 @@ fn fetch_impl( // `ReadFile` has `Drop`; can't use FRU `..Default::default()`. let mut rf_args = node::fs::args::ReadFile::default(); rf_args.encoding = Encoding::Buffer; - rf_args.path = PathOrFileDescriptor::Fd(opened_fd); + rf_args.path = PathOrFileDescriptor::Fd(*opened_fd); rf_args.offset = blob_offset; rf_args.max_size = Some(blob_size); let res = node_fs.read_file(&rf_args, node::fs::Flavor::Sync); - if pathlike_is_path { - opened_fd.close(); - } + // Eagerly close before constructing the (potentially large) JS + // result. Dropping the guard runs the close exactly once. + drop(opened_fd); match res { Err(err) => { diff --git a/src/s3_signing/credentials.rs b/src/s3_signing/credentials.rs index a7d95be0d75..89a38c55828 100644 --- a/src/s3_signing/credentials.rs +++ b/src/s3_signing/credentials.rs @@ -429,6 +429,12 @@ impl S3Credentials { if bucket.is_empty() { return Err(SignError::InvalidEndpoint); } + // The bucket is interpolated into the host; a `/` (or `\`, + // which encode_uri_component normalizes to `/`) would let a + // crafted bucket redirect the signed request to another host. + if bucket.contains(&b'/') { + return Err(SignError::InvalidEndpoint); + } // default to https://.s3..amazonaws.com/ let mut v = Vec::new(); write!( @@ -461,7 +467,12 @@ impl S3Credentials { } else { break 'brk buf_print( &mut normalized_path_buffer, - format_args!("{}/{}/{}", BStr::new(extra_path), BStr::new(bucket), BStr::new(path)), + format_args!( + "{}/{}/{}", + BStr::new(extra_path), + BStr::new(bucket), + BStr::new(path) + ), ) .map_err(|_| SignError::InvalidPath)?; } @@ -502,7 +513,12 @@ impl S3Credentials { let sig_date_region_service_req: [u8; DIGESTED_HMAC_256_LEN] = 'brk_sign: { let key = buf_print( &mut tmp_buffer, - format_args!("{}{}{}", BStr::new(region), service_name, BStr::new(&self.secret_access_key)), + format_args!( + "{}{}{}", + BStr::new(region), + service_name, + BStr::new(&self.secret_access_key) + ), ) .map_err(|_| SignError::NoSpaceLeft)?; // PORT NOTE: was `bun_jsc::VirtualMachine::get*().rare_data().aws_cache()`. @@ -556,7 +572,12 @@ impl S3Credentials { // TODO(port): fix the overwritten-key bug in credentials.zig as well. let key = buf_print( &mut tmp_buffer, - format_args!("{}{}{}", BStr::new(region), service_name, BStr::new(&self.secret_access_key)), + format_args!( + "{}{}{}", + BStr::new(region), + service_name, + BStr::new(&self.secret_access_key) + ), ) .map_err(|_| SignError::NoSpaceLeft)?; aws_cache_set(date_result.numeric_day, key, digest); @@ -847,6 +868,9 @@ impl S3Credentials { || content_disposition.is_some_and(contains_newline_or_cr) || content_encoding.is_some_and(contains_newline_or_cr) || session_token.is_some_and(contains_newline_or_cr) + || contains_newline_or_cr(region) + || contains_newline_or_cr(&self.access_key_id) + || contains_newline_or_cr(&host) { return Err(SignError::InvalidHeaderValue); } diff --git a/src/shell_parser/parse.rs b/src/shell_parser/parse.rs index afe48fafc35..1ab4ff86edc 100644 --- a/src/shell_parser/parse.rs +++ b/src/shell_parser/parse.rs @@ -1211,7 +1211,10 @@ impl<'bump> Parser<'bump> { fn is_if_clause_text_token(&mut self, if_clause_token: IfClauseTok) -> bool { match self.peek() { - Token::Text(range) => self.is_if_clause_text_token_impl(range, if_clause_token), + Token::Text(range) => { + self.delimits(self.peek_n(1)) + && self.is_if_clause_text_token_impl(range, if_clause_token) + } _ => false, } } @@ -2064,6 +2067,13 @@ impl<'bump> Parser<'bump> { let Token::Text(range) = peektok else { return false; }; + // A keyword like `fi` only terminates the if body when it is followed + // by a delimiter. `fi$x`, `else$x`, etc. are ordinary command text and + // must keep the body loop running so they go through the normal + // command/atom path instead of the panicking `expect_if_clause_text_token`. + if !self.delimits(self.peek_n(1)) { + return false; + } let txt = self.text(range); for &tag in toktags { if txt == <&'static str>::from(tag).as_bytes() { @@ -2183,9 +2193,16 @@ pub enum IfClauseTok { } impl IfClauseTok { + /// Classify the *current peeked* token as an if-clause keyword. + /// + /// `tok` must be `p.peek()`: like `match_if_clausetok` and + /// `is_if_clause_text_token`, this only treats the text as a keyword when + /// the *next* token delimits it, so `fi$x` / `else$x` / `elif$x` are not + /// misclassified and routed into the panicking + /// `expect_if_clause_text_token`. pub fn from_tok(p: &Parser<'_>, tok: Token) -> Option { match tok { - Token::Text(range) => Self::from_text(p.text(range)), + Token::Text(range) if p.delimits(p.peek_n(1)) => Self::from_text(p.text(range)), _ => None, } } diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index 3ba36191647..22a1f263ea0 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -821,7 +821,7 @@ impl MySQLConnection { Auth::caching_sha2_password::FastAuthStatus::CONTINUE_AUTH => { bun_core::scoped_log!(MySQLConnection, "continue auth"); - if self.ssl_mode == SSLMode::Disable { + if self.tls_status != TLSStatus::SslOk { // we are in plain TCP so we need to request the public key self.set_status(ConnectionState::AuthenticationAwaitingPk); bun_core::scoped_log!( diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 462cebaa5d4..11cfe65ecec 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -876,7 +876,8 @@ impl PostgresSQLConnection { .get_native_handle() .map_or(core::ptr::null_mut(), |p| p.cast()); // SAFETY: `servername` is a NUL-terminated C string owned by `tls_config`. - let hostname = unsafe { bun_core::ffi::cstr(servername) }.to_bytes(); + let hostname = + unsafe { bun_core::ffi::cstr(servername) }.to_bytes(); // SAFETY: `ssl_ptr` is the live SSL* of a connected TLS socket. !ssl_ptr.is_null() && BoringSSL::check_server_identity( @@ -2715,13 +2716,44 @@ impl PostgresSQLConnection { debug!("SASLContinue"); let iteration_count = cont.iteration_count().map_err(pg_err)?; + // RFC 7677 §4: SCRAM-SHA-256 requires a minimum of 4096 + // iterations. Cap the upper bound to avoid a CPU-burn DoS + // from a malicious/MITM'd server sending i ≈ u32::MAX. + if !(4096..=10_000_000).contains(&iteration_count) { + debug!( + "SASLContinue iteration count out of range: {}", + iteration_count + ); + return Err(AnyPostgresError::InvalidMessage); + } - let server_salt_decoded_base64 = bun_base64::decode_alloc(cont.s.slice()) + // Bound the *encoded* length before allocating: a + // malicious/MITM'd server can otherwise force an + // arbitrarily large decode_alloc here. 1024 decoded + // bytes ≈ 1368 base64 chars; round up generously. + let server_salt_b64 = cont.s.slice(); + if server_salt_b64.is_empty() || server_salt_b64.len() > 2048 { + debug!( + "SASLContinue encoded salt length out of range: {}", + server_salt_b64.len() + ); + return Err(AnyPostgresError::InvalidMessage); + } + let server_salt_decoded_base64 = bun_base64::decode_alloc(server_salt_b64) .map_err(|e| match e { bun_base64::DecodeAllocError::DecodingFailed => { AnyPostgresError::SASL_SIGNATURE_INVALID_BASE64 } })?; + if server_salt_decoded_base64.is_empty() + || server_salt_decoded_base64.len() > 1024 + { + debug!( + "SASLContinue salt length out of range: {}", + server_salt_decoded_base64.len() + ); + return Err(AnyPostgresError::InvalidMessage); + } sasl.compute_salted_password( &server_salt_decoded_base64, iteration_count,