From b7a640b5172ebcdea73179c5a33486930ef35e96 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 27 May 2020 19:22:46 -0400 Subject: [PATCH 1/7] net: Treat SSL handshake errors differently from other hyper errors. --- components/net_traits/lib.rs | 9 +++++++-- components/script/dom/servoparser/mod.rs | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index 3222daceff0..f2f88ba4f92 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -712,12 +712,17 @@ pub enum NetworkError { Internal(String), LoadCancelled, /// SSL validation error that has to be handled in the HTML parser - SslValidation(ServoUrl, String), + SslValidation(String), } impl NetworkError { pub fn from_hyper_error(error: &HyperError) -> Self { - NetworkError::Internal(error.to_string()) + let s = error.to_string(); + if s.contains("the handshake failed") { + NetworkError::SslValidation(s) + } else { + NetworkError::Internal(s) + } } pub fn from_http_error(error: &HttpError) -> Self { diff --git a/components/script/dom/servoparser/mod.rs b/components/script/dom/servoparser/mod.rs index d1a4a18df18..9ed99e6b812 100644 --- a/components/script/dom/servoparser/mod.rs +++ b/components/script/dom/servoparser/mod.rs @@ -731,9 +731,9 @@ impl FetchResponseListener for ParserContext { FetchMetadata::Unfiltered(m) => m, FetchMetadata::Filtered { unsafe_, .. } => unsafe_, }), - Err(NetworkError::SslValidation(url, reason)) => { + Err(NetworkError::SslValidation(reason)) => { ssl_error = Some(reason); - let mut meta = Metadata::default(url); + let mut meta = Metadata::default(self.url.clone()); let mime: Option = "text/html".parse().ok(); meta.set_content_type(mime.as_ref()); Some(meta) From 1cdaf40eb24c26bc2d372a791ffb9a6a08d7a304 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 27 May 2020 19:24:06 -0400 Subject: [PATCH 2/7] net: Add an SSL verification callback to support checking a dynamic list of certs. --- components/net/connector.rs | 49 ++++++++++++++++++++++++++++-- components/net/resource_thread.rs | 18 +++++++---- components/net/websocket_loader.rs | 4 +-- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/components/net/connector.rs b/components/net/connector.rs index 058a27c47d6..4369e236501 100644 --- a/components/net/connector.rs +++ b/components/net/connector.rs @@ -8,8 +8,12 @@ use hyper::client::HttpConnector as HyperHttpConnector; use hyper::rt::Future; use hyper::{Body, Client}; use hyper_openssl::HttpsConnector; -use openssl::ssl::{SslConnector, SslConnectorBuilder, SslMethod, SslOptions}; -use openssl::x509; +use openssl::ex_data::Index; +use openssl::ssl::{ + SslConnector, SslConnectorBuilder, SslContext, SslMethod, SslOptions, SslVerifyMode, +}; +use openssl::x509::{self, X509StoreContext}; +use std::sync::{Arc, Mutex}; use tokio::prelude::future::Executor; pub const BUF_SIZE: usize = 32768; @@ -60,7 +64,20 @@ impl Connect for HttpConnector { pub type Connector = HttpsConnector; pub type TlsConfig = SslConnectorBuilder; -pub fn create_tls_config(certs: &str, alpn: &[u8]) -> TlsConfig { +#[derive(Clone)] +pub(crate) struct ExtraCerts(pub Arc>>>); + +impl ExtraCerts { + pub(crate) fn new() -> Self { + Self(Arc::new(Mutex::new(vec![]))) + } +} + +lazy_static! { + static ref INDEX: Index = SslContext::new_ex_index().unwrap(); +} + +pub(crate) fn create_tls_config(certs: &str, alpn: &[u8], extra_certs: ExtraCerts) -> 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. @@ -104,6 +121,32 @@ pub fn create_tls_config(certs: &str, alpn: &[u8]) -> TlsConfig { SslOptions::NO_COMPRESSION, ); + cfg.set_ex_data(*INDEX, extra_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, + } + } else { + false + } + }); + cfg } diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs index 28a397e78e6..10a83fbc04c 100644 --- a/components/net/resource_thread.rs +++ b/components/net/resource_thread.rs @@ -4,7 +4,7 @@ //! A thread that takes a URL and streams back the binary data. -use crate::connector::{create_http_client, create_tls_config, ALPN_H2_H1}; +use crate::connector::{create_http_client, create_tls_config, ExtraCerts, ALPN_H2_H1}; use crate::cookie; use crate::cookie_storage::CookieStorage; use crate::fetch::cors_cache::CorsCache; @@ -127,7 +127,7 @@ struct ResourceChannelManager { fn create_http_states( config_dir: Option<&Path>, certificate_path: Option, -) -> (Arc, Arc) { +) -> (Arc, Arc, ExtraCerts) { let mut hsts_list = HstsList::from_servo_preload(); let mut auth_cache = AuthCache::new(); let http_cache = HttpCache::new(); @@ -143,6 +143,8 @@ fn create_http_states( None => resources::read_string(Resource::SSLCertificates), }; + let extra_certs = ExtraCerts::new(); + let http_state = HttpState { hsts_list: RwLock::new(hsts_list), cookie_jar: RwLock::new(cookie_jar), @@ -151,7 +153,7 @@ 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), + create_tls_config(&certs, ALPN_H2_H1, extra_certs.clone()), HANDLE.lock().unwrap().as_ref().unwrap().executor(), ), }; @@ -164,12 +166,16 @@ 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), + create_tls_config(&certs, ALPN_H2_H1, extra_certs.clone()), HANDLE.lock().unwrap().as_ref().unwrap().executor(), ), }; - (Arc::new(http_state), Arc::new(private_http_state)) + ( + Arc::new(http_state), + Arc::new(private_http_state), + extra_certs, + ) } impl ResourceChannelManager { @@ -180,7 +186,7 @@ impl ResourceChannelManager { private_receiver: IpcReceiver, memory_reporter: IpcReceiver, ) { - let (public_http_state, private_http_state) = create_http_states( + let (public_http_state, private_http_state, extra_certs) = 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 69d3c430fcb..fee98bbd8d0 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, ALPN_H1}; +use crate::connector::{create_tls_config, 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,7 @@ 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); + let tls_config = create_tls_config(&certs, ALPN_H1, ExtraCerts::new()); tls_config .build() .connect(domain, stream) From 0ce2aa917a4fa11971d91315182e350577572478 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 29 May 2020 11:56:17 -0400 Subject: [PATCH 3/7] 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, u32)>>>, +} + +impl ConnectionCerts { + pub(crate) fn new() -> Self { + Self { + certs: Arc::new(Mutex::new(HashMap::new())), + } + } + + fn store(&self, host: String, cert_bytes: Vec) { + 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> { + 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; pub type TlsConfig = SslConnectorBuilder; #[derive(Clone)] -pub(crate) struct ExtraCerts(pub Arc>>>); +pub struct ExtraCerts(pub Arc>>>); impl ExtraCerts { pub(crate) fn new() -> Self { @@ -73,11 +106,21 @@ impl ExtraCerts { } } +struct Host(String); + lazy_static! { - static ref INDEX: Index = SslContext::new_ex_index().unwrap(); + static ref EXTRA_INDEX: Index = SslContext::new_ex_index().unwrap(); + static ref CONNECTION_INDEX: Index = + SslContext::new_ex_index().unwrap(); + static ref HOST_INDEX: Index = 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(tls_config: TlsConfig, executor: E) -> Client + 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, pub history_states: RwLock>>, pub client: Client, + 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, -) -> (Arc, Arc, ExtraCerts) { +) -> (Arc, Arc) { 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, memory_reporter: IpcReceiver, ) { - 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), } impl NetworkError { - pub fn from_hyper_error(error: &HyperError) -> Self { + pub fn from_hyper_error(error: &HyperError, cert_bytes: Option>) -> 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 = "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 @@ Certificate error -

${reason}

+

${reason}

+
${bytes}
From 433c154595f62d80210992cf889fdb8fd65848bc Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 29 May 2020 13:34:55 -0400 Subject: [PATCH 4/7] net: Allow SSL websockets to use dynamic list of certs as well. --- components/net/connector.rs | 9 +++++---- components/net/resource_thread.rs | 2 ++ components/net/websocket_loader.rs | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/components/net/connector.rs b/components/net/connector.rs index 20d58fee65e..41ab42e2184 100644 --- a/components/net/connector.rs +++ b/components/net/connector.rs @@ -184,12 +184,13 @@ pub(crate) fn create_tls_config( 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()); + // Ensure there's an entry stored in the set of known connection certs for this connection. + if let Some(host) = ssl.ex_data(*HOST_INDEX) { + 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(); diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs index eaea61da7c5..94184c3a046 100644 --- a/components/net/resource_thread.rs +++ b/components/net/resource_thread.rs @@ -727,6 +727,8 @@ impl CoreResourceManager { action_receiver, http_state.clone(), self.certificate_path.clone(), + http_state.extra_certs.clone(), + http_state.connection_certs.clone(), ); } } diff --git a/components/net/websocket_loader.rs b/components/net/websocket_loader.rs index 17e06705709..bece51173bb 100644 --- a/components/net/websocket_loader.rs +++ b/components/net/websocket_loader.rs @@ -38,6 +38,8 @@ struct Client<'a> { event_sender: &'a IpcSender, protocol_in_use: Option, certificate_path: Option, + extra_certs: ExtraCerts, + connection_certs: ConnectionCerts, } impl<'a> Factory for Client<'a> { @@ -167,8 +169,12 @@ 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(), ConnectionCerts::new()); + let tls_config = create_tls_config( + &certs, + ALPN_H1, + self.extra_certs.clone(), + self.connection_certs.clone(), + ); tls_config .build() .connect(domain, stream) @@ -182,6 +188,8 @@ pub fn init( dom_action_receiver: IpcReceiver, http_state: Arc, certificate_path: Option, + extra_certs: ExtraCerts, + connection_certs: ConnectionCerts, ) { thread::Builder::new() .name(format!("WebSocket connection to {}", req_builder.url)) @@ -230,6 +238,8 @@ pub fn init( event_sender: &resource_event_sender, protocol_in_use: None, certificate_path, + extra_certs, + connection_certs, }; let mut ws = WebSocket::new(client).unwrap(); From 6a6662195e85bbd990987ce6e541b048a6f4c194 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 29 May 2020 13:35:43 -0400 Subject: [PATCH 5/7] net: Add option to temporarily accept certs that failed the handshake. --- Cargo.lock | 1 + components/net/connector.rs | 6 +++- components/net/fetch/methods.rs | 46 ++++++++++++++++++------ components/net_traits/Cargo.toml | 1 + components/net_traits/lib.rs | 5 +++ components/script/dom/servoparser/mod.rs | 2 ++ resources/badcert.html | 21 ++++++++++- resources/servo.css | 4 +++ 8 files changed, 73 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 56608679feb..7b13cfce465 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3609,6 +3609,7 @@ dependencies = [ "pixels", "serde", "servo_arc", + "servo_rand", "servo_url", "std_test_override", "time", diff --git a/components/net/connector.rs b/components/net/connector.rs index 41ab42e2184..0e47666f864 100644 --- a/components/net/connector.rs +++ b/components/net/connector.rs @@ -98,12 +98,16 @@ pub type Connector = HttpsConnector; pub type TlsConfig = SslConnectorBuilder; #[derive(Clone)] -pub struct ExtraCerts(pub Arc>>>); +pub struct ExtraCerts(Arc>>>); impl ExtraCerts { pub(crate) fn new() -> Self { Self(Arc::new(Mutex::new(vec![]))) } + + pub(crate) fn add(&self, bytes: Vec) { + self.0.lock().unwrap().push(bytes); + } } struct Host(String); diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 457b462bc0b..a73255bf66b 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -25,7 +25,7 @@ use net_traits::request::{ use net_traits::request::{CredentialsMode, Destination, Referrer, Request, RequestMode}; use net_traits::response::{Response, ResponseBody, ResponseType}; use net_traits::{FetchTaskTarget, NetworkError, ReferrerPolicy, ResourceFetchTiming}; -use net_traits::{ResourceAttribute, ResourceTimeValue}; +use net_traits::{ResourceAttribute, ResourceTimeValue, ResourceTimingType}; use servo_arc::Arc as ServoArc; use servo_url::ServoUrl; use std::borrow::Cow; @@ -282,7 +282,10 @@ pub fn main_fetch( false }; - if (same_origin && !cors_flag) || current_url.scheme() == "data" { + if (same_origin && !cors_flag) || + current_url.scheme() == "data" || + current_url.scheme() == "chrome" + { // Substep 1. request.response_tainting = ResponseTainting::Basic; @@ -606,6 +609,17 @@ fn range_not_satisfiable_error(response: &mut Response) { response.raw_status = Some((StatusCode::RANGE_NOT_SATISFIABLE.as_u16(), reason.into())); } +fn create_blank_reply(url: ServoUrl, timing_type: ResourceTimingType) -> Response { + let mut response = Response::new(url, ResourceFetchTiming::new(timing_type)); + response + .headers + .typed_insert(ContentType::from(mime::TEXT_HTML_UTF_8)); + *response.body.lock().unwrap() = ResponseBody::Done(vec![]); + response.status = Some((StatusCode::OK, "OK".to_string())); + response.raw_status = Some((StatusCode::OK.as_u16(), b"OK".to_vec())); + response +} + /// [Scheme fetch](https://fetch.spec.whatwg.org#scheme-fetch) fn scheme_fetch( request: &mut Request, @@ -617,15 +631,25 @@ fn scheme_fetch( let url = request.current_url(); match url.scheme() { - "about" if url.path() == "blank" => { - let mut response = Response::new(url, ResourceFetchTiming::new(request.timing_type())); - response - .headers - .typed_insert(ContentType::from(mime::TEXT_HTML_UTF_8)); - *response.body.lock().unwrap() = ResponseBody::Done(vec![]); - response.status = Some((StatusCode::OK, "OK".to_string())); - response.raw_status = Some((StatusCode::OK.as_u16(), b"OK".to_vec())); - response + "about" if url.path() == "blank" => create_blank_reply(url, request.timing_type()), + + "chrome" if url.path() == "allowcert" => { + let mut secret = None; + let mut cert_bytes = None; + for (name, value) in url.as_url().query_pairs() { + match &*name { + "secret" => secret = Some(value), + "bytes" => cert_bytes = base64::decode(value.as_bytes()).ok(), + _ => (), + } + } + if let (Some(secret), Some(bytes)) = (secret, cert_bytes) { + if secret.parse() == Ok(*net_traits::PRIVILEGED_SECRET) { + context.state.extra_certs.add(bytes); + } + } + + create_blank_reply(url, request.timing_type()) }, "http" | "https" => http_fetch( diff --git a/components/net_traits/Cargo.toml b/components/net_traits/Cargo.toml index 543d2b538c9..0e663b91bfd 100644 --- a/components/net_traits/Cargo.toml +++ b/components/net_traits/Cargo.toml @@ -33,6 +33,7 @@ piston_image = { package = "image", version = "0.23" } pixels = { path = "../pixels" } serde = "1.0" servo_arc = { path = "../servo_arc" } +servo_rand = { path = "../rand" } servo_url = { path = "../url" } time = "0.1" url = "2.0" diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index cd0f78013ba..6dea965d7f8 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -30,6 +30,7 @@ use ipc_channel::router::ROUTER; use ipc_channel::Error as IpcError; use mime::Mime; use msg::constellation_msg::HistoryStateId; +use servo_rand::RngCore; use servo_url::{ImmutableOrigin, ServoUrl}; use time::precise_time_ns; use webrender_api::{ImageData, ImageDescriptor, ImageKey}; @@ -811,3 +812,7 @@ impl WebrenderIpcSender { } } } + +lazy_static! { + pub static ref PRIVILEGED_SECRET: u32 = servo_rand::ServoRng::new().next_u32(); +} diff --git a/components/script/dom/servoparser/mod.rs b/components/script/dom/servoparser/mod.rs index d2d1831c490..3949f70c53e 100644 --- a/components/script/dom/servoparser/mod.rs +++ b/components/script/dom/servoparser/mod.rs @@ -821,6 +821,8 @@ impl FetchResponseListener for ParserContext { let page = page.replace("${reason}", &reason); let page = page.replace("${bytes}", std::str::from_utf8(&bytes).unwrap_or_default()); + let page = + page.replace("${secret}", &net_traits::PRIVILEGED_SECRET.to_string()); parser.push_string_input_chunk(page); parser.parse_sync(); } diff --git a/resources/badcert.html b/resources/badcert.html index 15fc425a376..9f37bcaa2a1 100644 --- a/resources/badcert.html +++ b/resources/badcert.html @@ -4,6 +4,25 @@

${reason}

-
${bytes}
+
${bytes}
+ + + diff --git a/resources/servo.css b/resources/servo.css index bc8d4f1223a..ed6bc3e93dd 100644 --- a/resources/servo.css +++ b/resources/servo.css @@ -1,3 +1,7 @@ +button { + cursor: default; +} + button, input { background: white; From 2550600131e26c135d9a3e408572c24a85f72f75 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 9 Jun 2020 12:50:08 -0400 Subject: [PATCH 6/7] net: Use a POST request for allowing certs temporarily. --- components/net/fetch/methods.rs | 36 +++++++++++++++++++------------- components/net/http_loader.rs | 2 +- components/net_traits/request.rs | 6 +++--- resources/badcert.html | 4 ++-- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index a73255bf66b..c25ab10cbe6 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -15,14 +15,16 @@ use headers::{AccessControlExposeHeaders, ContentType, HeaderMapExt, Range}; use http::header::{self, HeaderMap, HeaderName}; use hyper::Method; use hyper::StatusCode; -use ipc_channel::ipc::IpcReceiver; +use ipc_channel::ipc::{self, IpcReceiver}; use mime::{self, Mime}; use net_traits::blob_url_store::{parse_blob_url, BlobURLStoreError}; use net_traits::filemanager_thread::{FileTokenCheck, RelativePos}; use net_traits::request::{ is_cors_safelisted_method, is_cors_safelisted_request_header, Origin, ResponseTainting, Window, }; -use net_traits::request::{CredentialsMode, Destination, Referrer, Request, RequestMode}; +use net_traits::request::{ + BodyChunkRequest, CredentialsMode, Destination, Referrer, Request, RequestMode, +}; use net_traits::response::{Response, ResponseBody, ResponseType}; use net_traits::{FetchTaskTarget, NetworkError, ReferrerPolicy, ResourceFetchTiming}; use net_traits::{ResourceAttribute, ResourceTimeValue, ResourceTimingType}; @@ -634,18 +636,24 @@ fn scheme_fetch( "about" if url.path() == "blank" => create_blank_reply(url, request.timing_type()), "chrome" if url.path() == "allowcert" => { - let mut secret = None; - let mut cert_bytes = None; - for (name, value) in url.as_url().query_pairs() { - match &*name { - "secret" => secret = Some(value), - "bytes" => cert_bytes = base64::decode(value.as_bytes()).ok(), - _ => (), - } - } - if let (Some(secret), Some(bytes)) = (secret, cert_bytes) { - if secret.parse() == Ok(*net_traits::PRIVILEGED_SECRET) { - context.state.extra_certs.add(bytes); + let data = request.body.as_mut().and_then(|body| { + let stream = body.take_stream(); + let (body_chan, body_port) = ipc::channel().unwrap(); + let _ = stream.send(BodyChunkRequest::Connect(body_chan)); + let _ = stream.send(BodyChunkRequest::Chunk); + body_port.recv().ok() + }); + let data = data.as_ref().and_then(|b| { + let idx = b.iter().position(|b| *b == b'&')?; + Some(b.split_at(idx)) + }); + + if let Some((secret, bytes)) = data { + let secret = str::from_utf8(secret).ok().and_then(|s| s.parse().ok()); + if secret == Some(*net_traits::PRIVILEGED_SECRET) { + if let Ok(bytes) = base64::decode(&bytes[1..]) { + context.state.extra_certs.add(bytes); + } } } diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 9f15dbcad72..d69f418006c 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -1571,7 +1571,7 @@ fn http_network_fetch( &url, &request.method, &request.headers, - request.body.as_mut().and_then(|body| body.take_stream()), + request.body.as_mut().map(|body| body.take_stream()), &request.pipeline_id, request_id.as_ref().map(Deref::deref), is_xhr, diff --git a/components/net_traits/request.rs b/components/net_traits/request.rs index 8906f4f038f..7af3855720d 100644 --- a/components/net_traits/request.rs +++ b/components/net_traits/request.rs @@ -164,7 +164,7 @@ impl RequestBody { } } - pub fn take_stream(&mut self) -> Option> { + pub fn take_stream(&mut self) -> IpcSender { if self.read_from { match self.source { BodySource::Null => panic!( @@ -174,12 +174,12 @@ impl RequestBody { let (chan, port) = ipc::channel().unwrap(); let _ = self.chan.send(BodyChunkRequest::Extract(port)); self.chan = chan.clone(); - return Some(chan); + return chan; }, } } self.read_from = true; - Some(self.chan.clone()) + self.chan.clone() } pub fn source_is_null(&self) -> bool { diff --git a/resources/badcert.html b/resources/badcert.html index 9f37bcaa2a1..392bddc0572 100644 --- a/resources/badcert.html +++ b/resources/badcert.html @@ -14,11 +14,11 @@ if (bytes.length) { button.onclick = function() { let xhr = new XMLHttpRequest(); - xhr.open('GET', 'chrome:allowcert?secret=${secret}&bytes=' + btoa(bytes)); + xhr.open('POST', 'chrome:allowcert'); xhr.onloadend = function() { location.reload(true); }; - xhr.send(); + xhr.send("${secret}&" + btoa(bytes)); }; } else { button.style.display = "none"; From c8692d83ab46899dc3ed3f58388164df50b485b9 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 9 Jun 2020 14:26:43 -0400 Subject: [PATCH 7/7] net: Add unit test for accepting a self-signed cert. --- components/net/connector.rs | 8 +-- components/net/tests/fetch.rs | 95 +++++++++++++++++++++++++++++++++-- components/net/tests/main.rs | 9 +++- 3 files changed, 103 insertions(+), 9 deletions(-) diff --git a/components/net/connector.rs b/components/net/connector.rs index 0e47666f864..dc44002a85a 100644 --- a/components/net/connector.rs +++ b/components/net/connector.rs @@ -41,7 +41,7 @@ pub struct ConnectionCerts { } impl ConnectionCerts { - pub(crate) fn new() -> Self { + pub fn new() -> Self { Self { certs: Arc::new(Mutex::new(HashMap::new())), } @@ -101,11 +101,11 @@ pub type TlsConfig = SslConnectorBuilder; pub struct ExtraCerts(Arc>>>); impl ExtraCerts { - pub(crate) fn new() -> Self { + pub fn new() -> Self { Self(Arc::new(Mutex::new(vec![]))) } - pub(crate) fn add(&self, bytes: Vec) { + pub fn add(&self, bytes: Vec) { self.0.lock().unwrap().push(bytes); } } @@ -119,7 +119,7 @@ lazy_static! { static ref HOST_INDEX: Index = Ssl::new_ex_index().unwrap(); } -pub(crate) fn create_tls_config( +pub fn create_tls_config( certs: &str, alpn: &[u8], extra_certs: ExtraCerts, diff --git a/components/net/tests/fetch.rs b/components/net/tests/fetch.rs index 38797cf027b..dc4c0f032c0 100644 --- a/components/net/tests/fetch.rs +++ b/components/net/tests/fetch.rs @@ -22,7 +22,7 @@ use hyper::body::Body; use hyper::{Request as HyperRequest, Response as HyperResponse}; use mime::{self, Mime}; use msg::constellation_msg::TEST_PIPELINE_ID; -use net::connector::{create_tls_config, ALPN_H2_H1}; +use net::connector::{create_tls_config, ConnectionCerts, ExtraCerts, ALPN_H2_H1}; use net::fetch::cors_cache::CorsCache; use net::fetch::methods::{self, CancellationListener, FetchContext}; use net::filemanager_thread::FileManager; @@ -682,7 +682,12 @@ fn test_fetch_with_hsts() { let (server, url) = make_ssl_server(handler, cert_path.clone(), key_path.clone()); let certs = fs::read_to_string(cert_path).expect("Couldn't find certificate file"); - let tls_config = create_tls_config(&certs, ALPN_H2_H1); + let tls_config = create_tls_config( + &certs, + ALPN_H2_H1, + ExtraCerts::new(), + ConnectionCerts::new(), + ); let mut context = FetchContext { state: Arc::new(HttpState::new(tls_config)), @@ -735,7 +740,12 @@ fn test_load_adds_host_to_hsts_list_when_url_is_https() { url.as_mut_url().set_scheme("https").unwrap(); let certs = fs::read_to_string(cert_path).expect("Couldn't find certificate file"); - let tls_config = create_tls_config(&certs, ALPN_H2_H1); + let tls_config = create_tls_config( + &certs, + ALPN_H2_H1, + ExtraCerts::new(), + ConnectionCerts::new(), + ); let mut context = FetchContext { state: Arc::new(HttpState::new(tls_config)), @@ -776,6 +786,85 @@ fn test_load_adds_host_to_hsts_list_when_url_is_https() { .is_host_secure(url.host_str().unwrap())); } +#[test] +fn test_fetch_self_signed() { + let handler = move |_: HyperRequest, response: &mut HyperResponse| { + *response.body_mut() = b"Yay!".to_vec().into(); + }; + let client_cert_path = Path::new("../../resources/certs").canonicalize().unwrap(); + let cert_path = Path::new("../../resources/self_signed_certificate_for_testing.crt") + .canonicalize() + .unwrap(); + let key_path = Path::new("../../resources/privatekey_for_testing.key") + .canonicalize() + .unwrap(); + let (_server, mut url) = make_ssl_server(handler, cert_path.clone(), key_path.clone()); + url.as_mut_url().set_scheme("https").unwrap(); + + let cert_data = fs::read_to_string(cert_path.clone()).expect("Couldn't find certificate file"); + let client_cert_data = + fs::read_to_string(client_cert_path.clone()).expect("Couldn't find certificate file"); + let extra_certs = ExtraCerts::new(); + let tls_config = create_tls_config( + &client_cert_data, + ALPN_H2_H1, + extra_certs.clone(), + ConnectionCerts::new(), + ); + + let mut context = FetchContext { + state: Arc::new(HttpState::new(tls_config)), + user_agent: DEFAULT_USER_AGENT.into(), + devtools_chan: None, + filemanager: FileManager::new(create_embedder_proxy(), Weak::new()), + file_token: FileTokenCheck::NotRequired, + cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))), + timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new( + ResourceTimingType::Navigation, + ))), + }; + + let mut request = RequestBuilder::new(url.clone()) + .method(Method::GET) + .body(None) + .destination(Destination::Document) + .origin(url.clone().origin()) + .pipeline_id(Some(TEST_PIPELINE_ID)) + .build(); + + let response = fetch_with_context(&mut request, &mut context); + + assert!(matches!( + response.get_network_error(), + Some(NetworkError::SslValidation(..)) + )); + + extra_certs.add(cert_data.as_bytes().into()); + + // FIXME: something weird happens inside the SSL server after the first + // connection encounters a verification error, and it no longer + // accepts new connections that should work fine. We are forced + // to start a new server and connect to that to verfiy that + // the self-signed cert is now accepted. + + let (server, mut url) = make_ssl_server(handler, cert_path.clone(), key_path.clone()); + url.as_mut_url().set_scheme("https").unwrap(); + + let mut request = RequestBuilder::new(url.clone()) + .method(Method::GET) + .body(None) + .destination(Destination::Document) + .origin(url.clone().origin()) + .pipeline_id(Some(TEST_PIPELINE_ID)) + .build(); + + let response = fetch_with_context(&mut request, &mut context); + + assert!(response.status.unwrap().0.is_success()); + + let _ = server.close(); +} + #[test] fn test_fetch_with_sri_network_error() { static MESSAGE: &'static [u8] = b"alert('Hello, Network Error');"; diff --git a/components/net/tests/main.rs b/components/net/tests/main.rs index c48ac873729..7583fdf0fc5 100644 --- a/components/net/tests/main.rs +++ b/components/net/tests/main.rs @@ -29,7 +29,7 @@ use hyper::server::conn::Http; use hyper::server::Server as HyperServer; use hyper::service::service_fn_ok; use hyper::{Body, Request as HyperRequest, Response as HyperResponse}; -use net::connector::{create_tls_config, ALPN_H2_H1}; +use net::connector::{create_tls_config, ConnectionCerts, ExtraCerts, ALPN_H2_H1}; use net::fetch::cors_cache::CorsCache; use net::fetch::methods::{self, CancellationListener, FetchContext}; use net::filemanager_thread::FileManager; @@ -91,7 +91,12 @@ fn new_fetch_context( pool_handle: Option>, ) -> FetchContext { let certs = resources::read_string(Resource::SSLCertificates); - let tls_config = create_tls_config(&certs, ALPN_H2_H1); + let tls_config = create_tls_config( + &certs, + ALPN_H2_H1, + ExtraCerts::new(), + ConnectionCerts::new(), + ); let sender = fc.unwrap_or_else(|| create_embedder_proxy()); FetchContext {