Reimplement DNS identifier validation.

.github/workflows/ci.yaml

@@ -61,11 +61,13 @@ jobs:
             build: aws-lc-static
 
         include:
+          # minimal supported nginx version
           - runner: ubuntu
             rust-version: stable
-            nginx-ref: stable-1.26
+            nginx-ref: stable-1.22
             build: debug
 
+          # minimal supported Rust version
           - runner: ubuntu
             rust-version: ${{ needs.rust-version.outputs.version }}
             nginx-ref: stable-1.28

README.md

@@ -28,7 +28,7 @@ The module implements following specifications:
 
 ### Requirements
 
-- NGINX sources, 1.25.0 or later.
+- NGINX sources, 1.22.0 or later.
 - Regular NGINX build dependencies: C compiler, make, PCRE2, Zlib
 - System-wide installation of OpenSSL 1.1.1 or later
 - Rust toolchain (1.81.0 or later)

src/acme/solvers/tls_alpn.rs

@@ -27,7 +27,7 @@ use core::ffi::{c_int, c_uint, c_void, CStr};
 use core::net::{Ipv4Addr, Ipv6Addr};
 use core::ptr;
 
-use nginx_sys::{ngx_conf_t, ngx_connection_t, ngx_http_validate_host, ngx_str_t, NGX_LOG_WARN};
+use nginx_sys::{ngx_conf_t, ngx_connection_t, ngx_str_t, NGX_LOG_WARN};
 use ngx::allocator::Allocator;
 use ngx::collections::RbTreeMap;
 use ngx::core::{NgxString, SlabPool, Status};
@@ -408,22 +408,16 @@ unsafe extern "C" fn acme_ssl_cert_cb(ssl: *mut SSL, arg: *mut c_void) -> c_int
         return 0;
     };
 
-    let name = unsafe { SSL_get_servername(ssl, openssl_sys::TLSEXT_NAMETYPE_host_name as _) };
-    if name.is_null() {
+    let Some(mut name) = (unsafe {
+        SSL_get_servername(ssl, openssl_sys::TLSEXT_NAMETYPE_host_name as _)
+            .as_ref()
+            .map(|x| CStr::from_ptr(x).to_bytes())
+            // Reallocate, because we will need to mutate the name in place.
+            .and_then(|x| ngx_str_t::from_bytes(c.pool, x))
+    }) else {
         return 0;
-    }
-
-    let mut name = ngx_str_t {
-        data: name.cast_mut().cast(),
-        len: unsafe { CStr::from_ptr(name).count_bytes() },
     };
 
-    // Validate `name` and reallocate on the connection pool.
-    if !Status(unsafe { ngx_http_validate_host(&mut name, c.pool, 1) }).is_ok() {
-        ngx_log_error!(NGX_LOG_WARN, c.log, "acme/tls-alpn-01: invalid server name");
-        return 0;
-    }
-
     let id = match acme_parse_ssl_server_name(&mut name) {
         Ok(x) => x,
         Err(err) => {
@@ -496,6 +490,8 @@ enum ParseIdentifierError {
     InvalidV4Ptr,
     #[error("invalid IPv6 reverse mapping")]
     InvalidV6Ptr,
+    #[error("invalid name")]
+    Name,
     #[error(transparent)]
     Utf8(#[from] core::str::Utf8Error),
 }
@@ -504,6 +500,8 @@ enum ParseIdentifierError {
 fn acme_parse_ssl_server_name(
     name: &mut ngx_str_t,
 ) -> Result<Identifier<&str>, ParseIdentifierError> {
+    name.as_bytes_mut().make_ascii_lowercase();
+
     if let Some(v4) = name.strip_suffix(".in-addr.arpa") {
         // RFC1035 § 3.5 encoded IPv4 address.
 
@@ -572,7 +570,12 @@ fn acme_parse_ssl_server_name(
 
         Ok(Identifier::Ip(name.to_str()?))
     } else {
-        Ok(Identifier::Dns(name.to_str()?))
+        let name = name.to_str()?;
+        if !name.is_ascii() {
+            return Err(ParseIdentifierError::Name);
+        }
+
+        Ok(Identifier::Dns(name))
     }
 }
 
@@ -676,7 +679,9 @@ mod tests {
 
         let pairs: &[(&str, Result<Identifier<&str>, _>)] = &[
             ("example.com", Ok(Identifier::Dns("example.com"))),
+            ("EXAMPLE.COM", Ok(Identifier::Dns("example.com"))),
             ("1.0.0.127.in-addr.arpa", Ok(Identifier::Ip("127.0.0.1"))),
+            ("1.0.0.127.IN-ADDR.ARPA", Ok(Identifier::Ip("127.0.0.1"))),
             ("1.0.0.0.127.in-addr.arpa", Err(Error::InvalidV4Ptr)),
             ("1.0..0.127.in-addr.arpa", Err(Error::InvalidV4Ptr)),
             ("256.0.0.127.in-addr.arpa", Err(Error::InvalidV4Ptr)),
@@ -685,6 +690,10 @@ mod tests {
                 "1.a.b.c.d.e.f.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6.arpa",
                 Ok(Identifier::Ip("fe80::fed:cba1")),
             ),
+            (
+                "1.A.B.C.D.E.F.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.E.F.IP6.ARPA",
+                Ok(Identifier::Ip("fe80::fed:cba1")),
+            ),
             (
                 "1.a.b.c.d.e.f.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6.arpa",
                 Err(Error::InvalidV6Ptr),

src/conf/order.rs

@@ -7,10 +7,10 @@ use core::fmt;
 use core::hash::{self, Hash, Hasher};
 use core::str::Utf8Error;
 
-use nginx_sys::{ngx_conf_t, ngx_http_server_name_t, ngx_str_t};
+use nginx_sys::{ngx_conf_t, ngx_http_server_name_t};
 use ngx::allocator::{AllocError, Allocator, TryCloneIn};
 use ngx::collections::Vec;
-use ngx::core::{NgxString, Pool, Status};
+use ngx::core::{NgxString, Pool};
 use ngx::ngx_log_error;
 use siphasher::sip::SipHasher;
 use thiserror::Error;
@@ -136,12 +136,10 @@ where
 pub enum IdentifierError {
     #[error("memory allocation failed")]
     Alloc(#[from] AllocError),
-    #[error("invalid server name")]
-    Invalid,
+    #[error("invalid server name: {0}")]
+    Invalid(#[from] NameError),
     #[error("invalid UTF-8 string")]
     Utf8(#[from] Utf8Error),
-    #[error("unsupported wildcard server name")]
-    Wildcard,
 }
 
 impl CertificateOrder<&'static str, Pool> {
@@ -193,37 +191,39 @@ impl CertificateOrder<&'static str, Pool> {
             return self.push(Identifier::Ip(addr)).map_err(Into::into);
         }
 
-        if value.contains('*') {
-            return Err(IdentifierError::Wildcard);
-        }
-
-        let host = validate_host(cf, value).map_err(|st| {
-            if st == Status::NGX_ERROR {
-                IdentifierError::Alloc(AllocError)
-            } else {
-                IdentifierError::Invalid
-            }
-        })?;
+        let realloc = validate_dns_identifier(value)?;
 
         /*
          * The only special syntax we want to support is a leading dot, which matches the domain
          * with "www." and without it.
          * See <https://nginx.org/en/docs/http/server_names.html>
          */
 
-        if let Some(host) = host.strip_prefix(".") {
+        if let Some(host) = value.strip_prefix(".") {
             let mut www = Vec::new_in(self.identifiers.allocator().clone());
             www.try_reserve_exact(host.len() + 4)
                 .map_err(|_| AllocError)?;
             www.extend_from_slice(b"www.");
             www.extend_from_slice(host.as_bytes());
+            www.make_ascii_lowercase();
+            // SAFETY: www is a pre-validated ASCII character slice.
             // The buffer is owned by ngx_pool_t and does not leak.
-            let www = core::str::from_utf8(www.leak())?;
+            let www = unsafe { core::str::from_utf8_unchecked(www.leak()) };
 
             self.push(Identifier::Dns(www))?;
-            self.push(Identifier::Dns(host))?;
+            self.push(Identifier::Dns(&www[4..]))?;
+        } else if realloc {
+            let mut out = Vec::new_in(self.identifiers.allocator().clone());
+            out.try_reserve_exact(value.len()).map_err(|_| AllocError)?;
+            out.extend_from_slice(value.as_bytes());
+            out.make_ascii_lowercase();
+            // SAFETY: out is a pre-validated ASCII character slice.
+            // The buffer is owned by ngx_pool_t and does not leak.
+            let out = unsafe { core::str::from_utf8_unchecked(out.leak()) };
+
+            self.push(Identifier::Dns(out))?;
         } else {
-            self.push(Identifier::Dns(host))?;
+            self.push(Identifier::Dns(value))?;
         }
 
         Ok(())
@@ -289,17 +289,119 @@ fn parse_ip_identifier(
     Ok(Some(out))
 }
 
-/// Checks if the value is a valid domain name and returns a canonical (lowercase) form,
-/// reallocated on the configuration pool if necessary.
-fn validate_host(cf: &ngx_conf_t, host: &'static str) -> Result<&'static str, Status> {
-    let mut host = ngx_str_t {
-        data: host.as_ptr().cast_mut(),
-        len: host.len(),
-    };
-    let rc = Status(unsafe { nginx_sys::ngx_http_validate_host(&mut host, cf.pool, 0) });
-    if rc != Status::NGX_OK {
-        return Err(rc);
+#[derive(Debug, Error, PartialEq)]
+pub enum NameError {
+    #[error("invalid character {0:x} at position {1}")]
+    Invalid(u8, usize),
+    #[error("unexpected character '{0}' at position {1}")]
+    Unexpected(char, usize),
+    #[error("does not end with a label")]
+    NotALabel,
+}
+
+/// Validates that the `name` can be used as a DNS identifier.
+///
+/// RFC8555 § 7.1.4 states that the "dns" identifier is a fully qualified domain name,
+/// encoded according to the rules in RFC5280 § 7.
+/// In practice, existing ACME servers and clients are rather relaxed about this requirement,
+/// trusting the user to specify valid values and the underlying protocol implementations (DNS or
+/// HTTP) to reject invalid ones.
+///
+/// Given that, the validation logic is loosely based on the `ngx_http_validate_host` as of 1.29.4,
+/// asserting that the name
+///
+///  - is a printable ASCII string, as required both by ACME and by NGINX[^1]
+///  - is a valid Host value accepted by NGINX
+///  - can have a leading dot or a single leading wildcard label
+///  - ends with a non-empty label
+///
+/// Returns true if the name needs to be reallocated and converted to lower case.
+///
+/// [^1]: https://nginx.org/en/docs/http/server_names.html
+fn validate_dns_identifier(name: &str) -> Result<bool, NameError> {
+    #[derive(PartialEq, Eq)]
+    enum State {
+        Start,
+        Label,
+        Dot,
+        Wildcard,
+    }
+
+    let mut alloc = false;
+    let mut state = State::Start;
+
+    for (i, ch) in name.bytes().enumerate() {
+        state = match ch {
+            // non-printable - handle separately for better error formatting
+            0x00..=0x20 | 0x7f.. => return Err(NameError::Invalid(ch, i)),
+
+            b'*' if state == State::Start => State::Wildcard,
+            b'.' if state != State::Dot => State::Dot,
+            // A wildcard domain name consists of a single asterisk character followed by a single
+            // full stop character ("*.") followed by a domain name (RFC8555 § 7.1.3)
+            _ if state == State::Wildcard => return Err(NameError::Unexpected(ch as _, i)),
+
+            // unreserved
+            b'A'..=b'Z' => {
+                alloc = true;
+                State::Label
+            }
+            b'0'..=b'9' | b'a'..=b'z' | b'-' | b'_' | b'~' => State::Label,
+
+            // pct-encoded
+            b'%' => State::Label,
+
+            // sub-delims
+            b'!' | b'$' | b'&' | b'\'' | b'(' | b')' | b'+' | b',' | b';' | b'=' => State::Label,
+
+            _ => return Err(NameError::Unexpected(ch as _, i)),
+        };
     }
 
-    unsafe { super::conf_value_to_str(&host) }.map_err(|_| Status::NGX_ERROR)
+    // We intentionally reject the trailing dot, as it should not appear in the TLS layer.
+
+    if state != State::Label {
+        return Err(NameError::NotALabel);
+    }
+
+    Ok(alloc)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_validate_dns_identifier() {
+        for (name, expect) in [
+            ("example", Ok(false)),
+            ("_example", Ok(false)),
+            ("exam_ple", Ok(false)),
+            ("Example", Ok(true)),
+            ("Example.", Err(NameError::NotALabel)),
+            ("E\x10ample", Err(NameError::Invalid(0x10, 1))),
+            ("00.example.test", Ok(false)),
+            ("www1.example.Test", Ok(true)),
+            ("www.example..test", Err(NameError::Unexpected('.', 12))),
+            ("www.example.test.", Err(NameError::NotALabel)),
+            ("пример.испытание", Err(NameError::Invalid(0xd0, 0))),
+            ("xn--e1afmkfd.xn--80akhbyknj4f", Ok(false)),
+            // wildcards
+            ("*", Err(NameError::NotALabel)),
+            ("*.example.test", Ok(false)),
+            ("*.xn--e1afmkfd.xn--80akhbyknj4f", Ok(false)),
+            ("*ww.example.test", Err(NameError::Unexpected('w', 1))),
+            ("ww*.example.test", Err(NameError::Unexpected('*', 2))),
+            ("www.example.*", Err(NameError::Unexpected('*', 12))),
+            // incorrect syntax
+            ("", Err(NameError::NotALabel)),
+            ("*.", Err(NameError::NotALabel)),
+        ] {
+            assert_eq!(
+                super::validate_dns_identifier(name),
+                expect,
+                "incorrect validation result for \"{name}\"",
+            );
+        }
+    }
 }

t/acme_conf_certificate.t

@@ -24,7 +24,7 @@ use Test::Nginx;
 select STDERR; $| = 1;
 select STDOUT; $| = 1;
 
-my $t = Test::Nginx->new()->has(qw/http http_ssl/)->plan(4);
+my $t = Test::Nginx->new()->has(qw/http http_ssl/)->plan(6);
 
 use constant TEMPLATE_CONF => <<'EOF';
 
@@ -80,6 +80,22 @@ acme_certificate example;
 EOF
 
 
+like(check($t, <<'EOF' ), qr/\[emerg].*invalid server name/, 'bad name - 1');
+
+server_name www.example.*;
+acme_certificate example;
+
+EOF
+
+
+like(check($t, <<'EOF' ), qr/\[emerg].*invalid server name/, 'bad name - 2');
+
+server_name .example..test;
+acme_certificate example;
+
+EOF
+
+
 like(check($t, <<'EOF' ), qr/\[emerg].*no identifiers/, 'no identifiers');
 
 acme_certificate example;