From 75f8857db71f128fb521e77135744c97453a6e4b Mon Sep 17 00:00:00 2001
From: Joseph Birr-Pixton <jpixton@gmail.com>
Date: Fri, 29 Mar 2024 09:20:24 +0000
Subject: [PATCH] Ignore server_name extension containing IP address

This works around quality-of-implementation issues in OpenSSL and
Apple SecureTransport: they send `server_name` extensions containing
IP addresses.  RFC6066 specifically disallows that.

It is a similar work-around to that adopted by LibreSSL: ignore
SNI contents if they can be parsed as an IP address.
---
 rustls/src/msgs/handshake.rs    | 31 ++++++++++++++++++++++++-------
 rustls/src/msgs/message_test.rs |  4 ++--
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/rustls/src/msgs/handshake.rs b/rustls/src/msgs/handshake.rs
index f8be007ed5..f14e41d84d 100644
--- a/rustls/src/msgs/handshake.rs
+++ b/rustls/src/msgs/handshake.rs
@@ -16,6 +16,8 @@ use crate::verify::DigitallySignedStruct;
 
 use std::collections;
 use std::fmt;
+use std::net::IpAddr;
+use std::str::FromStr;
 
 /// Create a newtype wrapper around a given type.
 ///
@@ -211,6 +213,7 @@ impl TlsListElement for SignatureScheme {
 #[derive(Clone, Debug)]
 pub enum ServerNamePayload {
     HostName(DnsName),
+    IpAddress(PayloadU16),
     Unknown(Payload),
 }
 
@@ -221,15 +224,12 @@ impl ServerNamePayload {
 
     fn read_hostname(r: &mut Reader) -> Result<Self, InvalidMessage> {
         let raw = PayloadU16::read(r)?;
-
         match DnsName::try_from_ascii(&raw.0) {
             Ok(dns_name) => Ok(Self::HostName(dns_name)),
             Err(_) => {
-                warn!(
-                    "Illegal SNI hostname received {:?}",
-                    String::from_utf8_lossy(&raw.0)
-                );
-                Err(InvalidMessage::InvalidServerName)
+                let _ = IpAddr::from_str(&String::from_utf8_lossy(&raw.0))
+                    .map_err(|_| InvalidMessage::InvalidServerName)?;
+                Ok(Self::IpAddress(raw))
             }
         }
     }
@@ -240,6 +240,7 @@ impl ServerNamePayload {
                 (name.as_ref().len() as u16).encode(bytes);
                 bytes.extend_from_slice(name.as_ref().as_bytes());
             }
+            Self::IpAddress(ref r) => r.encode(bytes),
             Self::Unknown(ref r) => r.encode(bytes),
         }
     }
@@ -897,7 +898,23 @@ impl ClientHelloPayload {
     pub fn get_sni_extension(&self) -> Option<&[ServerName]> {
         let ext = self.find_extension(ExtensionType::ServerName)?;
         match *ext {
-            ClientExtension::ServerName(ref req) => Some(req),
+            // Does this comply with RFC6066?
+            //
+            // [RFC6066][] specifies that literal IP addresses are illegal in
+            // `ServerName`s with a `name_type` of `host_name`.
+            //
+            // Some clients incorrectly send such extensions: we choose to
+            // successfully parse these (into `ServerNamePayload::IpAddress`)
+            // but then act like the client sent no `server_name` extension.
+            //
+            // [RFC6066]: https://datatracker.ietf.org/doc/html/rfc6066#section-3
+            ClientExtension::ServerName(ref req)
+                if !req
+                    .iter()
+                    .any(|name| matches!(name.payload, ServerNamePayload::IpAddress(_))) =>
+            {
+                Some(req)
+            }
             _ => None,
         }
     }
diff --git a/rustls/src/msgs/message_test.rs b/rustls/src/msgs/message_test.rs
index 5ba8f120bc..ba3c6e4863 100644
--- a/rustls/src/msgs/message_test.rs
+++ b/rustls/src/msgs/message_test.rs
@@ -48,7 +48,7 @@ fn test_read_fuzz_corpus() {
 }
 
 #[test]
-fn can_read_safari_client_hello() {
+fn can_read_safari_client_hello_with_ip_address_in_sni_extension() {
     let _ = env_logger::Builder::new()
         .filter(None, log::LevelFilter::Trace)
         .try_init();
@@ -72,7 +72,7 @@ fn can_read_safari_client_hello() {
     let mut rd = Reader::init(bytes);
     let m = OpaqueMessage::read(&mut rd).unwrap();
     println!("m = {:?}", m);
-    assert!(Message::try_from(m.into_plain_message()).is_err());
+    Message::try_from(m.into_plain_message()).unwrap();
 }
 
 #[test]