Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/dotenv/env_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is full of magic numbers +2 +5 +7.
Can be replaced with meaningful constants?

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() {
Expand Down
38 changes: 36 additions & 2 deletions src/http/HTTPContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ pub struct PooledSocket<const SSL: bool> {
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<ssl_config::SharedPtr>,
Expand Down Expand Up @@ -606,6 +612,7 @@ impl<const SSL: bool> HTTPContext<SSL> {
&mut self,
socket: HTTPSocket<SSL>,
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>,
Expand Down Expand Up @@ -660,6 +667,7 @@ impl<const SSL: bool> HTTPContext<SSL> {
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(),
Expand Down Expand Up @@ -786,6 +794,17 @@ impl<const SSL: bool> HTTPContext<SSL> {
{
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(
Expand Down Expand Up @@ -904,14 +923,28 @@ impl<const SSL: bool> HTTPContext<SSL> {
// 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)
{
Comment thread
coderabbitai[bot] marked this conversation as resolved.
s.adopt(client);
return Ok(None);
}
Comment thread
claude[bot] marked this conversation as resolved.
}
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());
Expand Down Expand Up @@ -1036,6 +1069,7 @@ impl<const SSL: bool> HTTPContext<SSL> {
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
Expand Down
6 changes: 6 additions & 0 deletions src/http/h2_client/ClientSession.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ pub struct ClientSession {
pub port: u16,
pub ssl_config: Option<ssl_config::SharedPtr>,
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.
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down
7 changes: 7 additions & 0 deletions src/http/h2_client/PendingConnect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<NonNull<SSLConfig>>,
/// 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<NonNull<HTTPClient<'static>>>,
}
Expand All @@ -27,6 +33,7 @@ impl Default for PendingConnect {
hostname: Box::default(),
port: 0,
ssl_config: None,
reject_unauthorized: false,
waiters: Vec::new(),
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: These three release_socket call sites pass self.flags.reject_unauthorized (the current request's flag) as established_with_reject_unauthorized, so a strict-established socket that gets reused by a lax client and re-pooled is downgraded to established_with_reject_unauthorized=false — a subsequent strict client is then needlessly blocked at HTTPContext.rs:805 from reusing a socket whose hostname was validated. Contrast ClientSession.rs:903, which correctly carries self.established_with_reject_unauthorized (set once at create). This errs conservative (extra TLS handshake, not a security hole), so it's just a keepalive-efficiency nit; the proper fix is to thread the original handshake's flag through ExistingSocketHTTPClient and pass it back here instead of re-deriving from the current request.

Extended reasoning...

What the bug is

Fix #83 adds an established_with_reject_unauthorized: bool field to PooledSocket so that existing_socket() can refuse to hand a strict (rejectUnauthorized: true) caller a socket whose TLS handshake never ran checkServerIdentity. The field is populated from the new release_socket() parameter. ClientSession (h2) does this correctly: it stores established_with_reject_unauthorized once at creation time from the establishing client (ClientSession.rs:241) and passes that stored value back to release_socket (ClientSession.rs:903). But the three HTTP/1.1 release_socket() call sites in lib.rs (lines 2318, 3595, 3817) instead pass self.flags.reject_unauthorized — the current request's flag, not the original handshake's strictness.

The code path that triggers it

For a fresh connection, the current request is the establishing request, so self.flags.reject_unauthorized happens to be correct. The divergence appears on the second pool round-trip:

  1. Strict client A establishes a TLS socket. on_handshake runs check_server_identity. A finishes and calls release_socket(..., established_with_reject_unauthorized = A.flags.reject_unauthorized = true, ...). Pooled with the flag = true. ✓
  2. Lax client B (rejectUnauthorized: false) calls existing_socket(). The guard at HTTPContext.rs:797–808 only blocks strict→lax-socket, not lax→strict-socket, so B is handed the socket via ExistingSocket { socket, tunnel, h2_session }.
  3. ExistingSocket (HTTPContext.rs:271–278) carries only { socket, tunnel, h2_session } — it does not carry established_with_reject_unauthorized out of the pool. The provenance is dropped on the floor.
  4. B finishes and calls release_socket(..., established_with_reject_unauthorized = B.flags.reject_unauthorized = **false**, ...). The PooledSocket is now marked as if it were lax-established.
  5. Strict client C calls existing_socket(). The guard at HTTPContext.rs:805 sees reject_unauthorized && !socket.established_with_reject_unauthorizedcontinue. C opens a fresh TLS connection even though the pooled socket's hostname was validated in step 1.

Why existing code doesn't prevent it

There is no field on HTTPClient that records the strictness of the handshake that produced the socket it's currently using; flags.reject_unauthorized is the request's policy, not the connection's provenance. And ExistingSocket doesn't carry the flag through the pool→client→pool round-trip, so once a lax client touches the socket the original strictness is unrecoverable. Contrast ClientSession, which is a per-connection object and stores the flag once at create().

Step-by-step proof

Consider a sequence of three fetch() calls to https://api.example.com over HTTP/1.1 keepalive:

  1. fetch(url, { tls: { rejectUnauthorized: true } }) — fresh connect, checkServerIdentity runs, request completes, release_socket at lib.rs:3595 pools it with established_with_reject_unauthorized = self.flags.reject_unauthorized = true.
  2. fetch(url, { tls: { rejectUnauthorized: false } })existing_socket() finds the pooled entry; reject_unauthorized=false so the guard at line 805 is skipped; socket reused. Request completes, release_socket at lib.rs:3595 pools it with established_with_reject_unauthorized = self.flags.reject_unauthorized = **false**.
  3. fetch(url, { tls: { rejectUnauthorized: true } })existing_socket() finds the pooled entry; reject_unauthorized=true && !socket.established_with_reject_unauthorizedcontinue. The socket is left in the pool and a fresh TLS handshake is performed, even though the pooled socket's certificate was fully validated in step 1.

Impact

This errs on the safe side — it never lets a strict caller reuse an actually-unverified socket, so it is not a security hole. It is a keepalive-efficiency regression introduced by fix #83: in workloads that interleave strict and lax requests to the same origin (e.g. a default-strict app that occasionally probes with rejectUnauthorized: false), a strictly-validated socket is permanently "downgraded" after any lax reuse and can never serve a strict request again, costing one extra TLS handshake per such interleaving. This is a narrow scenario and the cost is bounded, hence nit severity — not a blocker for this PR.

How to fix

Thread the provenance through the reuse cycle the same way ClientSession does:

  • Add established_with_reject_unauthorized: bool to ExistingSocket and populate it from the PooledSocket in existing_socket() before pending_sockets.put().
  • Carry it on HTTPClient (or in state) when an existing socket is adopted; for a fresh connect, set it from self.flags.reject_unauthorized at handshake time (matching what ClientSession::create does at line 241).
  • Pass that stored value, not self.flags.reject_unauthorized, at the three lib.rs release_socket call sites.

This is more invasive than the rest of fix #83 and is fine to defer to a follow-up.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the analysis — this can downgrade a strictly-handshaked pooled socket after a lax client touches it, costing a redundant TLS handshake on the next strict reuse. It never lets a strict caller reuse an unverified socket, so it stays conservative.

Threading the original handshake's flag through ExistingSocketHTTPClient and back to release_socket (mirroring ClientSession's established_with_reject_unauthorized field) is the right shape, but it touches the pool/client/release surface across three files. Deferring to a follow-up so this PR stays focused on the security-relevant pieces.

self.connected_url.hostname,
self.connected_url.get_port_auto(),
self.tls_props.as_ref(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down
18 changes: 12 additions & 6 deletions src/http_jsc/websocket_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,15 +287,13 @@ impl<const SSL: bool> WebSocket<SSL> {
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;
}

Expand All @@ -317,8 +315,8 @@ impl<const SSL: bool> WebSocket<SSL> {
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;
}
}
}
Expand Down Expand Up @@ -1322,6 +1320,10 @@ impl<const SSL: bool> WebSocket<SSL> {
}

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);
Expand Down Expand Up @@ -1667,6 +1669,10 @@ impl<const SSL: bool> WebSocket<SSL> {
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);
Expand Down
51 changes: 47 additions & 4 deletions src/install/bin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

Expand Down Expand Up @@ -1269,7 +1276,7 @@ impl<'a> Linker<'a> {
fn chmod_on_ok(err: &Option<Error>, 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));
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
self.abs_dest_buf[dest_off..dest_off + unscoped_package_name.len()]
.copy_from_slice(unscoped_package_name);
dest_off += unscoped_package_name.len();
Expand All @@ -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);
Expand All @@ -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()]
Expand Down Expand Up @@ -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);
Expand Down
19 changes: 16 additions & 3 deletions src/install/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand Down
Loading
Loading