From 0ce2aa917a4fa11971d91315182e350577572478 Mon Sep 17 00:00:00 2001
From: Josh Matthews <josh@joshmatthews.net>
Date: Fri, 29 May 2020 11:56:17 -0400
Subject: [PATCH] net: Pass certs that fail the SSL handshake out of the
 network layer.

---
 components/net/connector.rs              | 104 ++++++++++++++++++-----
 components/net/http_loader.rs            |  18 +++-
 components/net/resource_thread.rs        |  36 +++++---
 components/net/websocket_loader.rs       |   5 +-
 components/net_traits/lib.rs             |   6 +-
 components/script/dom/servoparser/mod.rs |   8 +-
 resources/badcert.html                   |   3 +-
 7 files changed, 136 insertions(+), 44 deletions(-)

diff --git a/components/net/connector.rs b/components/net/connector.rs
index 4369e236501..20d58fee65e 100644
--- a/components/net/connector.rs
+++ b/components/net/connector.rs
@@ -10,9 +10,10 @@ use hyper::{Body, Client};
 use hyper_openssl::HttpsConnector;
 use openssl::ex_data::Index;
 use openssl::ssl::{
-    SslConnector, SslConnectorBuilder, SslContext, SslMethod, SslOptions, SslVerifyMode,
+    Ssl, SslConnector, SslConnectorBuilder, SslContext, SslMethod, SslOptions, SslVerifyMode,
 };
 use openssl::x509::{self, X509StoreContext};
+use std::collections::hash_map::{Entry, HashMap};
 use std::sync::{Arc, Mutex};
 use tokio::prelude::future::Executor;
 
@@ -34,6 +35,38 @@ const SIGNATURE_ALGORITHMS: &'static str = concat!(
     "RSA+SHA512:RSA+SHA384:RSA+SHA256"
 );
 
+#[derive(Clone)]
+pub struct ConnectionCerts {
+    certs: Arc<Mutex<HashMap<String, (Vec<u8>, u32)>>>,
+}
+
+impl ConnectionCerts {
+    pub(crate) fn new() -> Self {
+        Self {
+            certs: Arc::new(Mutex::new(HashMap::new())),
+        }
+    }
+
+    fn store(&self, host: String, cert_bytes: Vec<u8>) {
+        let mut certs = self.certs.lock().unwrap();
+        let entry = certs.entry(host).or_insert((cert_bytes, 0));
+        entry.1 += 1;
+    }
+
+    pub(crate) fn remove(&self, host: String) -> Option<Vec<u8>> {
+        match self.certs.lock().unwrap().entry(host) {
+            Entry::Vacant(_) => return None,
+            Entry::Occupied(mut e) => {
+                e.get_mut().1 -= 1;
+                if e.get().1 == 0 {
+                    return Some((e.remove_entry().1).0);
+                }
+                Some(e.get().0.clone())
+            },
+        }
+    }
+}
+
 pub struct HttpConnector {
     inner: HyperHttpConnector,
 }
@@ -65,7 +98,7 @@ pub type Connector = HttpsConnector<HttpConnector>;
 pub type TlsConfig = SslConnectorBuilder;
 
 #[derive(Clone)]
-pub(crate) struct ExtraCerts(pub Arc<Mutex<Vec<Vec<u8>>>>);
+pub struct ExtraCerts(pub Arc<Mutex<Vec<Vec<u8>>>>);
 
 impl ExtraCerts {
     pub(crate) fn new() -> Self {
@@ -73,11 +106,21 @@ impl ExtraCerts {
     }
 }
 
+struct Host(String);
+
 lazy_static! {
-    static ref INDEX: Index<SslContext, ExtraCerts> = SslContext::new_ex_index().unwrap();
+    static ref EXTRA_INDEX: Index<SslContext, ExtraCerts> = SslContext::new_ex_index().unwrap();
+    static ref CONNECTION_INDEX: Index<SslContext, ConnectionCerts> =
+        SslContext::new_ex_index().unwrap();
+    static ref HOST_INDEX: Index<Ssl, Host> = Ssl::new_ex_index().unwrap();
 }
 
-pub(crate) fn create_tls_config(certs: &str, alpn: &[u8], extra_certs: ExtraCerts) -> TlsConfig {
+pub(crate) fn create_tls_config(
+    certs: &str,
+    alpn: &[u8],
+    extra_certs: ExtraCerts,
+    connection_certs: ConnectionCerts,
+) -> TlsConfig {
     // certs include multiple certificates. We could add all of them at once,
     // but if any of them were already added, openssl would fail to insert all
     // of them.
@@ -121,30 +164,41 @@ pub(crate) fn create_tls_config(certs: &str, alpn: &[u8], extra_certs: ExtraCert
             SslOptions::NO_COMPRESSION,
     );
 
-    cfg.set_ex_data(*INDEX, extra_certs);
+    cfg.set_ex_data(*EXTRA_INDEX, extra_certs);
+    cfg.set_ex_data(*CONNECTION_INDEX, connection_certs);
     cfg.set_verify_callback(SslVerifyMode::PEER, |verified, x509_store_context| {
         if verified {
             return true;
         }
-        if let Some(cert) = x509_store_context.current_cert() {
-            match cert.to_pem() {
-                Ok(pem) => {
-                    let ssl_idx = X509StoreContext::ssl_idx().unwrap();
-                    let ssl = x509_store_context.ex_data(ssl_idx).unwrap();
-                    let ssl_context = ssl.ssl_context();
-                    let extra_certs = ssl_context.ex_data(*INDEX).unwrap();
-                    for cert in &*extra_certs.0.lock().unwrap() {
-                        if pem == *cert {
-                            return true;
-                        }
-                    }
-                    false
-                },
-                Err(_) => false,
+
+        let ssl_idx = X509StoreContext::ssl_idx().unwrap();
+        let ssl = x509_store_context.ex_data(ssl_idx).unwrap();
+
+        // Obtain the cert bytes for this connection.
+        let cert = match x509_store_context.current_cert() {
+            Some(cert) => cert,
+            None => return false,
+        };
+        let pem = match cert.to_pem() {
+            Ok(pem) => pem,
+            Err(_) => return false,
+        };
+
+        // Ensure there's an entry stored in the set of known connection certs for this connection.
+        let host = ssl.ex_data(*HOST_INDEX).unwrap();
+        let ssl_context = ssl.ssl_context();
+        let connection_certs = ssl_context.ex_data(*CONNECTION_INDEX).unwrap();
+
+        connection_certs.store((*host).0.clone(), pem.clone());
+
+        // Fall back to the dynamic set of allowed certs.
+        let extra_certs = ssl_context.ex_data(*EXTRA_INDEX).unwrap();
+        for cert in &*extra_certs.0.lock().unwrap() {
+            if pem == *cert {
+                return true;
             }
-        } else {
-            false
         }
+        false
     });
 
     cfg
@@ -154,7 +208,11 @@ pub fn create_http_client<E>(tls_config: TlsConfig, executor: E) -> Client<Conne
 where
     E: Executor<Box<dyn Future<Error = (), Item = ()> + Send + 'static>> + Sync + Send + 'static,
 {
-    let connector = HttpsConnector::with_connector(HttpConnector::new(), tls_config).unwrap();
+    let mut connector = HttpsConnector::with_connector(HttpConnector::new(), tls_config).unwrap();
+    connector.set_callback(|configuration, destination| {
+        configuration.set_ex_data(*HOST_INDEX, Host(destination.host().to_owned()));
+        Ok(())
+    });
 
     Client::builder()
         .http1_title_case_headers(true)
diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs
index 44b36c1bb4d..9f15dbcad72 100644
--- a/components/net/http_loader.rs
+++ b/components/net/http_loader.rs
@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-use crate::connector::{create_http_client, Connector, TlsConfig};
+use crate::connector::{create_http_client, ConnectionCerts, Connector, ExtraCerts, TlsConfig};
 use crate::cookie;
 use crate::cookie_storage::CookieStorage;
 use crate::decoder::Decoder;
@@ -89,6 +89,8 @@ pub struct HttpState {
     pub auth_cache: RwLock<AuthCache>,
     pub history_states: RwLock<HashMap<HistoryStateId, Vec<u8>>>,
     pub client: Client<Connector, Body>,
+    pub extra_certs: ExtraCerts,
+    pub connection_certs: ConnectionCerts,
 }
 
 impl HttpState {
@@ -104,6 +106,8 @@ impl HttpState {
                 tls_config,
                 HANDLE.lock().unwrap().as_ref().unwrap().executor(),
             ),
+            extra_certs: ExtraCerts::new(),
+            connection_certs: ConnectionCerts::new(),
         }
     }
 }
@@ -527,11 +531,19 @@ fn obtain_response(
     let method = method.clone();
     let send_start = precise_time_ms();
 
+    let host = request.uri().host().unwrap_or("").to_owned();
+    let host_clone = request.uri().host().unwrap_or("").to_owned();
+    let connection_certs = context.state.connection_certs.clone();
+    let connection_certs_clone = context.state.connection_certs.clone();
+
     let headers = headers.clone();
     Box::new(
         client
             .request(request)
             .and_then(move |res| {
+                // We no longer need to track the cert for this connection.
+                connection_certs.remove(host);
+
                 let send_end = precise_time_ms();
 
                 // TODO(#21271) response_start: immediately after receiving first byte of response
@@ -564,7 +576,9 @@ fn obtain_response(
                 };
                 Ok((Decoder::detect(res), msg))
             })
-            .map_err(move |e| NetworkError::from_hyper_error(&e)),
+            .map_err(move |e| {
+                NetworkError::from_hyper_error(&e, connection_certs_clone.remove(host_clone))
+            }),
     )
 }
 
diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs
index 10a83fbc04c..eaea61da7c5 100644
--- a/components/net/resource_thread.rs
+++ b/components/net/resource_thread.rs
@@ -4,7 +4,9 @@
 
 //! A thread that takes a URL and streams back the binary data.
 
-use crate::connector::{create_http_client, create_tls_config, ExtraCerts, ALPN_H2_H1};
+use crate::connector::{
+    create_http_client, create_tls_config, ConnectionCerts, ExtraCerts, ALPN_H2_H1,
+};
 use crate::cookie;
 use crate::cookie_storage::CookieStorage;
 use crate::fetch::cors_cache::CorsCache;
@@ -127,7 +129,7 @@ struct ResourceChannelManager {
 fn create_http_states(
     config_dir: Option<&Path>,
     certificate_path: Option<String>,
-) -> (Arc<HttpState>, Arc<HttpState>, ExtraCerts) {
+) -> (Arc<HttpState>, Arc<HttpState>) {
     let mut hsts_list = HstsList::from_servo_preload();
     let mut auth_cache = AuthCache::new();
     let http_cache = HttpCache::new();
@@ -144,6 +146,7 @@ fn create_http_states(
     };
 
     let extra_certs = ExtraCerts::new();
+    let connection_certs = ConnectionCerts::new();
 
     let http_state = HttpState {
         hsts_list: RwLock::new(hsts_list),
@@ -153,11 +156,21 @@ fn create_http_states(
         http_cache: RwLock::new(http_cache),
         http_cache_state: Mutex::new(HashMap::new()),
         client: create_http_client(
-            create_tls_config(&certs, ALPN_H2_H1, extra_certs.clone()),
+            create_tls_config(
+                &certs,
+                ALPN_H2_H1,
+                extra_certs.clone(),
+                connection_certs.clone(),
+            ),
             HANDLE.lock().unwrap().as_ref().unwrap().executor(),
         ),
+        extra_certs,
+        connection_certs,
     };
 
+    let extra_certs = ExtraCerts::new();
+    let connection_certs = ConnectionCerts::new();
+
     let private_http_state = HttpState {
         hsts_list: RwLock::new(HstsList::from_servo_preload()),
         cookie_jar: RwLock::new(CookieStorage::new(150)),
@@ -166,16 +179,19 @@ fn create_http_states(
         http_cache: RwLock::new(HttpCache::new()),
         http_cache_state: Mutex::new(HashMap::new()),
         client: create_http_client(
-            create_tls_config(&certs, ALPN_H2_H1, extra_certs.clone()),
+            create_tls_config(
+                &certs,
+                ALPN_H2_H1,
+                extra_certs.clone(),
+                connection_certs.clone(),
+            ),
             HANDLE.lock().unwrap().as_ref().unwrap().executor(),
         ),
+        extra_certs,
+        connection_certs,
     };
 
-    (
-        Arc::new(http_state),
-        Arc::new(private_http_state),
-        extra_certs,
-    )
+    (Arc::new(http_state), Arc::new(private_http_state))
 }
 
 impl ResourceChannelManager {
@@ -186,7 +202,7 @@ impl ResourceChannelManager {
         private_receiver: IpcReceiver<CoreResourceMsg>,
         memory_reporter: IpcReceiver<ReportsChan>,
     ) {
-        let (public_http_state, private_http_state, extra_certs) = create_http_states(
+        let (public_http_state, private_http_state) = create_http_states(
             self.config_dir.as_ref().map(Deref::deref),
             self.certificate_path.clone(),
         );
diff --git a/components/net/websocket_loader.rs b/components/net/websocket_loader.rs
index fee98bbd8d0..17e06705709 100644
--- a/components/net/websocket_loader.rs
+++ b/components/net/websocket_loader.rs
@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-use crate::connector::{create_tls_config, ExtraCerts, ALPN_H1};
+use crate::connector::{create_tls_config, ConnectionCerts, ExtraCerts, ALPN_H1};
 use crate::cookie::Cookie;
 use crate::fetch::methods::should_be_blocked_due_to_bad_port;
 use crate::hosts::replace_host;
@@ -167,7 +167,8 @@ impl<'a> Handler for Client<'a> {
                 WebSocketErrorKind::Protocol,
                 format!("Unable to parse domain from {}. Needed for SSL.", url),
             ))?;
-        let tls_config = create_tls_config(&certs, ALPN_H1, ExtraCerts::new());
+        let tls_config =
+            create_tls_config(&certs, ALPN_H1, ExtraCerts::new(), ConnectionCerts::new());
         tls_config
             .build()
             .connect(domain, stream)
diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs
index f2f88ba4f92..cd0f78013ba 100644
--- a/components/net_traits/lib.rs
+++ b/components/net_traits/lib.rs
@@ -712,14 +712,14 @@ pub enum NetworkError {
     Internal(String),
     LoadCancelled,
     /// SSL validation error that has to be handled in the HTML parser
-    SslValidation(String),
+    SslValidation(String, Vec<u8>),
 }
 
 impl NetworkError {
-    pub fn from_hyper_error(error: &HyperError) -> Self {
+    pub fn from_hyper_error(error: &HyperError, cert_bytes: Option<Vec<u8>>) -> Self {
         let s = error.to_string();
         if s.contains("the handshake failed") {
-            NetworkError::SslValidation(s)
+            NetworkError::SslValidation(s, cert_bytes.unwrap_or_default())
         } else {
             NetworkError::Internal(s)
         }
diff --git a/components/script/dom/servoparser/mod.rs b/components/script/dom/servoparser/mod.rs
index 9ed99e6b812..d2d1831c490 100644
--- a/components/script/dom/servoparser/mod.rs
+++ b/components/script/dom/servoparser/mod.rs
@@ -731,8 +731,8 @@ impl FetchResponseListener for ParserContext {
                 FetchMetadata::Unfiltered(m) => m,
                 FetchMetadata::Filtered { unsafe_, .. } => unsafe_,
             }),
-            Err(NetworkError::SslValidation(reason)) => {
-                ssl_error = Some(reason);
+            Err(NetworkError::SslValidation(reason, cert_bytes)) => {
+                ssl_error = Some((reason, cert_bytes));
                 let mut meta = Metadata::default(self.url.clone());
                 let mime: Option<Mime> = "text/html".parse().ok();
                 meta.set_content_type(mime.as_ref());
@@ -815,10 +815,12 @@ impl FetchResponseListener for ParserContext {
             },
             Some(ref mime) if mime.type_() == mime::TEXT && mime.subtype() == mime::HTML => {
                 // Handle text/html
-                if let Some(reason) = ssl_error {
+                if let Some((reason, bytes)) = ssl_error {
                     self.is_synthesized_document = true;
                     let page = resources::read_string(Resource::BadCertHTML);
                     let page = page.replace("${reason}", &reason);
+                    let page =
+                        page.replace("${bytes}", std::str::from_utf8(&bytes).unwrap_or_default());
                     parser.push_string_input_chunk(page);
                     parser.parse_sync();
                 }
diff --git a/resources/badcert.html b/resources/badcert.html
index 5a96eee377e..15fc425a376 100644
--- a/resources/badcert.html
+++ b/resources/badcert.html
@@ -3,6 +3,7 @@
 <title>Certificate error</title>
 </head>
 <body>
-    <p>${reason}</p>
+  <p>${reason}</p>
+  <pre>${bytes}</pre>
 </body>
 </html>