-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Input validation and bounds-checking hardening #30681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -668,7 +668,16 @@ fn parse_nested_block<T>( | |
| let saved_stop_before = parser.stop_before; | ||
| parser.stop_before = closing_delimiter; | ||
| parser.at_start_of = None; | ||
| let result = parser.parse_entirely((), |(), p| parsefn(p)); | ||
| // Bound recursive descent so deeply nested blocks (e.g. `calc(calc(...))`) | ||
| // can't overflow the native stack; the error path leaves the parser recoverable. | ||
| let result = if parser.nesting_depth >= MAX_NESTING_DEPTH { | ||
| Err(parser.new_custom_error(ParserError::maximum_nesting_depth)) | ||
| } else { | ||
| parser.nesting_depth += 1; | ||
| let r = parser.parse_entirely((), |(), p| parsefn(p)); | ||
| parser.nesting_depth -= 1; | ||
| r | ||
| }; | ||
| if let Some(block_type2) = parser.at_start_of.take() { | ||
| consume_until_end_of_block(block_type2, &mut parser.input.tokenizer); | ||
| } | ||
|
|
@@ -3439,8 +3448,14 @@ pub struct Parser<'a> { | |
| /// materialises a fresh short-lived `&mut` instead. | ||
| pub import_records: Option<core::ptr::NonNull<Vec<ImportRecord>>>, | ||
| pub extra: Option<&'a mut ParserExtra>, | ||
| /// Current nested-block depth; bounded by [`MAX_NESTING_DEPTH`] to prevent | ||
| /// stack overflow from deeply nested input. | ||
| pub nesting_depth: u32, | ||
| } | ||
|
|
||
| /// Hard cap on nested-block depth (matches WebKit's CSS tokenizer limit). | ||
| pub const MAX_NESTING_DEPTH: u32 = 128; | ||
|
Comment on lines
+3451
to
+3457
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C2 --type rust '\bMAX_NESTING_DEPTH\b|\bnesting_depth\b' -g '!src/css/css_parser.rs'Repository: oven-sh/bun Length of output: 2313 Narrow the visibility of nesting guard internals to The 🤖 Prompt for AI Agents |
||
|
|
||
| impl<'a> Parser<'a> { | ||
| pub fn add_symbol_for_name( | ||
| &mut self, | ||
|
|
@@ -3563,6 +3578,7 @@ impl<'a> Parser<'a> { | |
| flags, | ||
| import_records, | ||
| extra, | ||
| nesting_depth: 0, | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -310,16 +310,21 @@ impl<const SSL: bool> WebSocket<SSL> { | |
| // null is well-defined (BoringSSL returns null). | ||
| let servername = | ||
| unsafe { boringssl::c::SSL_get_servername(ssl_ptr, TLSEXT_NAMETYPE_HOST_NAME) }; | ||
| if !servername.is_null() { | ||
| // SNI may be null (e.g. IP literal); with no hostname there is nothing | ||
| // to verify the certificate against, so reject rather than skip verification. | ||
| let hostname: &[u8] = if !servername.is_null() { | ||
| // SAFETY: servername is a NUL-terminated C string owned by the SSL session. | ||
| let hostname = unsafe { bun_core::ffi::cstr(servername) }.to_bytes(); | ||
| // SAFETY: ssl_ptr is non-null (connected SSL socket on the handshake path). | ||
| 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); | ||
| } | ||
| unsafe { bun_core::ffi::cstr(servername) }.to_bytes() | ||
| } else { | ||
| b"" | ||
| }; | ||
| if ssl_ptr.is_null() | ||
| || hostname.is_empty() | ||
| // SAFETY: ssl_ptr is non-null (checked above) for the open socket's lifetime. | ||
| || !boringssl::check_server_identity(unsafe { &mut *ssl_ptr }, hostname) | ||
| { | ||
| self.outgoing_websocket = None; | ||
| ws_ref.did_abrupt_close(ErrorCode::FailedToConnect); | ||
|
Comment on lines
+313
to
+327
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes, in both BoringSSL and OpenSSL, SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name) returns NULL when the client connects to an IP literal or otherwise does not send an SNI extension [1][2][3][4][5][6]. In BoringSSL source code, if no SNI extension is present in ClientHello, ssl->s3->hostname remains nullptr, and SSL_get_servername returns it [7][6]. OpenSSL documentation explicitly states it returns NULL if no servername was requested by the client [2][3][4][5]. When connecting to an IP literal, clients typically do not send SNI (as SNI must be a hostname per RFC 6066, not IP [8]), so NULL is returned [1][8]. Yes, certificate identity verification should use the original requested host/IP (including checking IP SANs) rather than the (possibly absent or different) negotiated SNI value [9][10][11]. OpenSSL's built-in verification via X509_check_host or SSL_CTX_set_verify uses the application-provided reference identity from functions like SSL_add1_host or X509_VERIFY_PARAM_set1_host, which is set to the originally requested endpoint (host or IP string) [9][10]. This checks against the peer certificate's CN/SAN DNS names (for hostnames) or IP SANs (for IPs) [9]. It does not use the SNI value, which is a separate server-side extension for virtual hosting and unavailable without client SNI [9][11]. Custom verification callbacks receive the original connection details independently of SNI [2]. Citations:
🏁 Script executed: # First, let's find the exact location and full context of the handle_handshake method
cd src/http_jsc && grep -n "fn handle_handshake" websocket_client.rsRepository: oven-sh/bun Length of output: 88 🏁 Script executed: # Let's examine the WebSocket struct definition and fields to see if it stores the original URL/host
rg "struct WebSocket" src/http_jsc/ -A 20Repository: oven-sh/bun Length of output: 5485 🏁 Script executed: # Check the full handle_handshake method signature and available context
sed -n '280,330p' src/http_jsc/websocket_client.rsRepository: oven-sh/bun Length of output: 2642 🏁 Script executed: # Check the Socket<SSL> definition to see if it stores the original host
rg "pub struct Socket" src/http_jsc/ -A 10Repository: oven-sh/bun Length of output: 37 🏁 Script executed: # Look for where handle_handshake is called and what context is available
rg "handle_handshake" src/http_jsc/ -B 3 -A 1Repository: oven-sh/bun Length of output: 1763 🏁 Script executed: # Check WebSocket initialization to see if original URL/host is stored
sed -n '1,100p' src/http_jsc/websocket_client.rs | head -60Repository: oven-sh/bun Length of output: 2318 🏁 Script executed: # Check if CppWebSocket stores the original host/URL
rg "impl CppWebSocket" src/http_jsc/ -A 30 | head -60Repository: oven-sh/bun Length of output: 5254 🏁 Script executed: # Look for WebSocket construction and initialization
rg "fn new\|fn init\|pub fn create" src/http_jsc/websocket_client.rs -A 5Repository: oven-sh/bun Length of output: 37 🏁 Script executed: # Search for where the WebSocket URL/host originates from
rg "class WebSocket\|new WebSocket" src/ -t cpp -t h --max-count 20Repository: oven-sh/bun Length of output: 37 Verify against the originally requested host, not the negotiated SNI value.
🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| // If reject_unauthorized is false, we accept the connection regardless of SSL errors | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -737,6 +737,17 @@ pub fn normalized_bin_name(name: &[u8]) -> &[u8] { | |
| name | ||
| } | ||
|
|
||
| /// True when `abs_target` is contained in `package_dir`. `bin` targets are | ||
| /// untrusted package.json values; a `../../..` target would be symlinked + chmod 0o777. | ||
| fn bin_target_within_package_dir(abs_target: &[u8], package_dir: &[u8]) -> bool { | ||
| debug_assert!(matches!(package_dir.last(), Some(&b'/') | Some(&b'\\'))); | ||
| let dir = strings::without_trailing_slash(package_dir); | ||
| if !strings::has_prefix(abs_target, dir) { | ||
| return false; | ||
| } | ||
| matches!(abs_target.get(dir.len()), None | Some(&b'/') | Some(&b'\\')) | ||
| } | ||
|
Comment on lines
+740
to
+749
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lexical containment still allows symlink escapes.
🤖 Prompt for AI Agents |
||
|
|
||
| pub struct Linker<'a> { | ||
| pub bin: Bin, | ||
|
|
||
|
|
@@ -1436,6 +1447,11 @@ impl<'a> Linker<'a> { | |
| target, | ||
| unscoped_package_name, | ||
| ); | ||
| // SECURITY: refuse `bin` targets that traverse outside the | ||
| // package directory (would otherwise be symlinked + chmod 0o777). | ||
| if !bin_target_within_package_dir(r.as_bytes(), package_dir) { | ||
| return; | ||
| } | ||
| // SAFETY: `resolve_bin_target` writes into the thread-local | ||
| // `PARSER_JOIN_INPUT_BUFFER` (via `join_abs_string_z`); the | ||
| // returned slice does not actually borrow `self` or | ||
|
|
@@ -1476,6 +1492,11 @@ impl<'a> Linker<'a> { | |
| target, | ||
| normalized_name, | ||
| ); | ||
| // SECURITY: refuse `bin` targets that traverse outside the | ||
| // package directory (would otherwise be symlinked + chmod 0o777). | ||
| if !bin_target_within_package_dir(r.as_bytes(), package_dir) { | ||
| return; | ||
| } | ||
| // SAFETY: thread-local buffer; see Tag::File above. | ||
| ZStr::from_raw(r.as_bytes().as_ptr(), r.len()) | ||
| }; | ||
|
|
@@ -1521,6 +1542,12 @@ impl<'a> Linker<'a> { | |
| bin_target, | ||
| normalized_bin_dest, | ||
| ); | ||
| // SECURITY: refuse `bin` targets that traverse outside the | ||
| // package directory (would otherwise be symlinked + chmod 0o777). | ||
| if !bin_target_within_package_dir(r.as_bytes(), package_dir) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| // SAFETY: thread-local buffer; see Tag::File above. | ||
| ZStr::from_raw(r.as_bytes().as_ptr(), r.len()) | ||
| }; | ||
|
|
@@ -1551,6 +1578,11 @@ impl<'a> Linker<'a> { | |
| let package_dir = &self.abs_target_buf[0..package_dir_len]; | ||
| let r = | ||
| resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target]); | ||
| // SECURITY: refuse `directories.bin` targets that traverse | ||
| // outside the package directory. | ||
| if !bin_target_within_package_dir(r.as_bytes(), package_dir) { | ||
| return; | ||
| } | ||
| // SAFETY: `join_abs_string_z` writes into the thread-local | ||
| // `PARSER_JOIN_INPUT_BUFFER`; result does not borrow | ||
| // `package_dir`. Detached so `abs_target_buf` can be | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 280
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 91
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 3499
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 2582
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 107
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 5476
Use RAII (
scopeguard::defer!) for nesting_depth cleanup to match the pattern already used in this file.The manual decrement-after-parse pattern is error-prone; the same issue with
composes_refscleanup is solved at line 2165 usingscopeguard::defer!. This ensuresnesting_depthis decremented on all exit paths (including early returns fromparser.parse_entirely) and aligns with the coding guideline to preferDrop-based cleanup.Suggested change
let result = if parser.nesting_depth >= MAX_NESTING_DEPTH { Err(parser.new_custom_error(ParserError::maximum_nesting_depth)) } else { parser.nesting_depth += 1; + scopeguard::defer! { + parser.nesting_depth -= 1; + } - let r = parser.parse_entirely((), |(), p| parsefn(p)); - parser.nesting_depth -= 1; - r + parser.parse_entirely((), |(), p| parsefn(p)) };🤖 Prompt for AI Agents