From 00e9c8fad08accfd5ca4e4f5e5edb7de5b60f070 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 28 Jul 2025 10:41:00 -0700 Subject: [PATCH 1/2] fix: use NgxStr in NgxListIterator to prevent UTF-8 panics ngx::http::NgxListIterator ngx_str_t items are now NgxStr, not str. The ngx_str_items in the header name and value are often untrusted input, and may not have utf-8 contents. The use of ngx_str_t::to_str in this iterator will panic when the contents are not utf-8. So, instead of yielding a pair of strs here, yield a pair of &NgxStr, which is like ngx_str_t but with more methods. --- examples/awssig.rs | 17 ++++++++++++----- src/http/request.rs | 9 +++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/examples/awssig.rs b/examples/awssig.rs index 5b903dc..594b1cd 100644 --- a/examples/awssig.rs +++ b/examples/awssig.rs @@ -286,8 +286,16 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { // Copy only headers that will be used to sign the request let mut headers = HeaderMap::new(); for (name, value) in request.headers_in_iterator() { - if name.to_lowercase() == "host" { - headers.insert(http::header::HOST, value.parse().unwrap()); + if let Ok(name) = name.to_str() { + if name.to_lowercase() == "host" { + if let Ok(value) = http::HeaderValue::from_bytes(value.as_bytes()) { + headers.insert(http::header::HOST, value); + } else { + return core::Status::NGX_DECLINED; + } + } + } else { + return core::Status::NGX_DECLINED; } } headers.insert("X-Amz-Date", datetime_now.parse().unwrap()); @@ -313,12 +321,11 @@ http_request_handler!(awssigv4_header_handler, |request: &mut Request| { request.add_header_in("authorization", signature.as_str()); request.add_header_in("X-Amz-Date", datetime_now.as_str()); - // done signing, let's print values we have in request.headers_out, request.headers_in for (name, value) in request.headers_out_iterator() { - ngx_log_debug_http!(request, "headers_out {}: {}", name, value); + ngx_log_debug_http!(request, "headers_out {name}: {value}",); } for (name, value) in request.headers_in_iterator() { - ngx_log_debug_http!(request, "headers_in {}: {}", name, value); + ngx_log_debug_http!(request, "headers_in {name}: {value}",); } core::Status::NGX_OK diff --git a/src/http/request.rs b/src/http/request.rs index f8dead2..3671717 100644 --- a/src/http/request.rs +++ b/src/http/request.rs @@ -462,7 +462,7 @@ impl<'a> Iterator for NgxListIterator<'a> { // something like pub struct Header(ngx_table_elt_t); // then header would have key and value - type Item = (&'a str, &'a str); + type Item = (&'a NgxStr, &'a NgxStr); fn next(&mut self) -> Option { let part = self.part.as_mut()?; @@ -478,7 +478,12 @@ impl<'a> Iterator for NgxListIterator<'a> { } let header = &part.arr[self.i]; self.i += 1; - Some((header.key.to_str(), header.value.to_str())) + unsafe { + Some(( + NgxStr::from_ngx_str(header.key), + NgxStr::from_ngx_str(header.value), + )) + } } } From 11bdc2087a7793a1b56832c46336834d2ddfab4b Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 28 Jul 2025 10:45:32 -0700 Subject: [PATCH 2/2] fix(sys): replace panicking ngx_str_t::to_str with fallible variant ngx_str_t::to_str should not contain an unwrap. In general, we should avoid exposing methods that panic when its reasonable to return a Result instead. This particular method was used, likely without considering that it may panic or validating that the contents are utf-8, in ngx::http::NgxListIterator's Iterator impl, which made it very easy to panic based on untrusted input. --- examples/async.rs | 12 +++++++++--- examples/awssig.rs | 16 +++++++++++++--- examples/curl.rs | 12 +++++++++--- nginx-sys/src/string.rs | 13 ++++++------- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/examples/async.rs b/examples/async.rs index c158d02..47f5de8 100644 --- a/examples/async.rs +++ b/examples/async.rs @@ -9,11 +9,11 @@ use ngx::ffi::{ ngx_array_push, ngx_command_t, ngx_conf_t, ngx_connection_t, ngx_event_t, ngx_http_handler_pt, ngx_http_module_t, ngx_http_phases_NGX_HTTP_ACCESS_PHASE, ngx_int_t, ngx_module_t, ngx_post_event, ngx_posted_events, ngx_posted_next_events, ngx_str_t, ngx_uint_t, - NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, + NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, NGX_LOG_EMERG, }; use ngx::http::{self, HttpModule, MergeConfigError}; use ngx::http::{HttpModuleLocationConf, HttpModuleMainConf, NgxHttpCoreModule}; -use ngx::{http_request_handler, ngx_log_debug_http, ngx_string}; +use ngx::{http_request_handler, ngx_conf_log_error, ngx_log_debug_http, ngx_string}; use tokio::runtime::Runtime; struct Module; @@ -205,7 +205,13 @@ extern "C" fn ngx_http_async_commands_set_enable( unsafe { let conf = &mut *(conf as *mut ModuleConfig); let args: &[ngx_str_t] = (*(*cf).args).as_slice(); - let val = args[1].to_str(); + let val = match args[1].to_str() { + Ok(s) => s, + Err(_) => { + ngx_conf_log_error!(NGX_LOG_EMERG, cf, "`async` argument is not utf-8 encoded"); + return ngx::core::NGX_CONF_ERROR; + } + }; // set default value optionally conf.enable = false; diff --git a/examples/awssig.rs b/examples/awssig.rs index 594b1cd..cbbea10 100644 --- a/examples/awssig.rs +++ b/examples/awssig.rs @@ -6,10 +6,10 @@ use ngx::ffi::{ ngx_array_push, ngx_command_t, ngx_conf_t, ngx_http_handler_pt, ngx_http_module_t, ngx_http_phases_NGX_HTTP_PRECONTENT_PHASE, ngx_int_t, ngx_module_t, ngx_str_t, ngx_uint_t, NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, - NGX_HTTP_SRV_CONF, + NGX_HTTP_SRV_CONF, NGX_LOG_EMERG, }; use ngx::http::*; -use ngx::{http_request_handler, ngx_log_debug_http, ngx_string}; +use ngx::{http_request_handler, ngx_conf_log_error, ngx_log_debug_http, ngx_string}; struct Module; @@ -176,7 +176,17 @@ extern "C" fn ngx_http_awssigv4_commands_set_enable( unsafe { let conf = &mut *(conf as *mut ModuleConfig); let args: &[ngx_str_t] = (*(*cf).args).as_slice(); - let val = args[1].to_str(); + let val = match args[1].to_str() { + Ok(s) => s, + Err(_) => { + ngx_conf_log_error!( + NGX_LOG_EMERG, + cf, + "`awssigv4` argument is not utf-8 encoded" + ); + return ngx::core::NGX_CONF_ERROR; + } + }; // set default value optionally conf.enable = false; diff --git a/examples/curl.rs b/examples/curl.rs index 74b4613..3d07002 100644 --- a/examples/curl.rs +++ b/examples/curl.rs @@ -4,11 +4,11 @@ use ngx::core; use ngx::ffi::{ ngx_array_push, ngx_command_t, ngx_conf_t, ngx_http_handler_pt, ngx_http_module_t, ngx_http_phases_NGX_HTTP_ACCESS_PHASE, ngx_int_t, ngx_module_t, ngx_str_t, ngx_uint_t, - NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, + NGX_CONF_TAKE1, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, NGX_HTTP_MODULE, NGX_LOG_EMERG, }; use ngx::http::{self, HttpModule, MergeConfigError}; use ngx::http::{HttpModuleLocationConf, HttpModuleMainConf, NgxHttpCoreModule}; -use ngx::{http_request_handler, ngx_log_debug_http, ngx_string}; +use ngx::{http_request_handler, ngx_conf_log_error, ngx_log_debug_http, ngx_string}; struct Module; @@ -119,7 +119,13 @@ extern "C" fn ngx_http_curl_commands_set_enable( let conf = &mut *(conf as *mut ModuleConfig); let args: &[ngx_str_t] = (*(*cf).args).as_slice(); - let val = args[1].to_str(); + let val = match args[1].to_str() { + Ok(s) => s, + Err(_) => { + ngx_conf_log_error!(NGX_LOG_EMERG, cf, "`curl` argument is not utf-8 encoded"); + return ngx::core::NGX_CONF_ERROR; + } + }; // set default value optionally conf.enable = false; diff --git a/nginx-sys/src/string.rs b/nginx-sys/src/string.rs index c386fb6..d007667 100644 --- a/nginx-sys/src/string.rs +++ b/nginx-sys/src/string.rs @@ -40,15 +40,14 @@ impl ngx_str_t { self.len == 0 } - /// Convert the nginx string to a string slice (`&str`). - /// - /// # Panics - /// This function panics if the `ngx_str_t` is not valid UTF-8. + /// Returns the contents of this `ngx_str_t` a string slice (`&str`) if + /// the contents are utf-8 encoded. /// /// # Returns - /// A string slice (`&str`) representing the nginx string. - pub fn to_str(&self) -> &str { - str::from_utf8(self.as_bytes()).unwrap() + /// A string slice (`&str`) representing the nginx string, or else a + /// Utf8Error. + pub fn to_str(&self) -> Result<&str, str::Utf8Error> { + str::from_utf8(self.as_bytes()) } /// Creates an empty `ngx_str_t` instance.