diff --git a/Cargo.toml b/Cargo.toml index 4fda2bb1..66fb38c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,7 @@ untrusted = "0.5.1" base64 = "0.6" log = { version = "0.3.6", optional = true } ring = { version = "0.12", features = ["rsa_signing"] } -webpki = "0.17" +webpki = { git = "https://github.com/briansmith/webpki" } sct = "0.2" [features] @@ -29,7 +29,7 @@ mio = "0.6" docopt = "0.8" serde = "1.0" serde_derive = "1.0" -webpki-roots = "0.13" +webpki-roots = { git = "https://github.com/briansmith/webpki-roots", branch = "webpki-github" } ct-logs = "0.2" regex = "0.2" diff --git a/src/error.rs b/src/error.rs index 1c03bcc8..e15c6125 100644 --- a/src/error.rs +++ b/src/error.rs @@ -63,6 +63,9 @@ pub enum TLSError { /// We failed to figure out what time it currently is. FailedToGetCurrentTime, + + /// A syntactically-invalid DNS hostname was given. + InvalidDNSName(String), } fn join(items: &[T]) -> String { @@ -122,6 +125,7 @@ impl Error for TLSError { TLSError::InvalidSCT(_) => "invalid certificate timestamp", TLSError::General(_) => "unexpected error", // (please file a bug), TLSError::FailedToGetCurrentTime => "failed to get current time", + TLSError::InvalidDNSName(_) => "Invalid DNS name", } } } diff --git a/src/msgs/handshake.rs b/src/msgs/handshake.rs index a98c4d72..e8ac95e7 100644 --- a/src/msgs/handshake.rs +++ b/src/msgs/handshake.rs @@ -371,21 +371,6 @@ impl ConvertServerNameList for ServerNameRequest { } } -pub fn same_hostname_or_both_none(a: Option<&ServerName>, - b: Option<&ServerName>) -> bool { - match (a, b) { - (Some(a), Some(b)) => { - match (&a.payload, &b.payload) { - (&ServerNamePayload::HostName(ref a_str), - &ServerNamePayload::HostName(ref b_str)) => a_str == b_str, - (_, _) => false, - } - }, - (None, None) => true, - _ => false, - } -} - pub type ProtocolNameList = VecU16OfPayloadU8; pub trait ConvertProtocolNameList { diff --git a/src/msgs/persist.rs b/src/msgs/persist.rs index fdadb2bf..b59fcee1 100644 --- a/src/msgs/persist.rs +++ b/src/msgs/persist.rs @@ -1,10 +1,13 @@ use msgs::handshake::SessionID; use msgs::enums::{CipherSuite, ProtocolVersion}; use msgs::codec::{Reader, Codec}; -use msgs::handshake::{CertificatePayload, ServerName}; +use msgs::handshake::CertificatePayload; use msgs::base::{PayloadU8, PayloadU16}; use msgs::codec; +use webpki; +use untrusted; + use std::mem; use std::cmp; @@ -153,7 +156,7 @@ pub type ServerSessionKey = SessionID; #[derive(Debug)] pub struct ServerSessionValue { - pub sni: Option, + pub sni: Option, pub version: ProtocolVersion, pub cipher_suite: CipherSuite, pub master_secret: PayloadU8, @@ -165,7 +168,8 @@ impl Codec for ServerSessionValue { fn encode(&self, bytes: &mut Vec) { if let &Some(ref sni) = &self.sni { codec::encode_u8(1, bytes); - sni.encode(bytes); + let sni_bytes: &str = sni.as_ref().into(); + PayloadU8::new(Vec::from(sni_bytes)).encode(bytes); } else { codec::encode_u8(0, bytes); } @@ -181,7 +185,10 @@ impl Codec for ServerSessionValue { fn read(r: &mut Reader) -> Option { let has_sni = try_ret!(codec::read_u8(r)); let sni = if has_sni == 1 { - Some(try_ret!(ServerName::read(r))) + let dns_name = try_ret!(PayloadU8::read(r)); + let dns_name = try_ret!(webpki::DNSNameRef::try_from_ascii( + untrusted::Input::from(&dns_name.0)).ok()); + Some(dns_name.into()) } else { None }; @@ -207,7 +214,7 @@ impl Codec for ServerSessionValue { } impl ServerSessionValue { - pub fn new(sni: Option<&ServerName>, + pub fn new(sni: Option<&webpki::DNSName>, v: ProtocolVersion, cs: CipherSuite, ms: Vec, diff --git a/src/server/hs.rs b/src/server/hs.rs index dc25a92d..c9b43a37 100644 --- a/src/server/hs.rs +++ b/src/server/hs.rs @@ -7,8 +7,7 @@ use msgs::base::{Payload, PayloadU8}; use msgs::handshake::{HandshakePayload, SupportedSignatureSchemes}; use msgs::handshake::{HandshakeMessagePayload, ServerHelloPayload, Random}; use msgs::handshake::{ClientHelloPayload, ServerExtension, SessionID}; -use msgs::handshake::{ConvertProtocolNameList, ConvertServerNameList, - same_hostname_or_both_none, ServerName}; +use msgs::handshake::{ConvertProtocolNameList, ConvertServerNameList}; use msgs::handshake::{NamedGroups, SupportedGroups, ClientExtension}; use msgs::handshake::{ECPointFormatList, SupportedPointFormats}; use msgs::handshake::{ServerECDHParams, DigitallySignedStruct}; @@ -91,12 +90,26 @@ fn can_resume(sess: &ServerSessionImpl, resume.cipher_suite == sess.common.get_suite().suite && (resume.extended_ms == handshake.using_ems || (resume.extended_ms && !handshake.using_ems)) && - same_hostname_or_both_none(resume.sni.as_ref(), sess.sni.as_ref()) + same_dns_name_or_both_none(resume.sni.as_ref(), sess.sni.as_ref()) } else { false } } +// Require an exact match for the purpose of comparing SNI DNS Names from two +// client hellos, even though a case-insensitive comparison might also be OK. +fn same_dns_name_or_both_none(a: Option<&webpki::DNSName>, + b: Option<&webpki::DNSName>) -> bool { + match (a, b) { + (Some(a), Some(b)) => { + let a: &str = a.as_ref().into(); + let b: &str = b.as_ref().into(); + a == b + }, + (None, None) => true, + _ => false, + } +} // Changing the keys must not span any fragmented handshake // messages. Otherwise the defragmented messages will have @@ -724,7 +737,7 @@ impl ExpectClientHello { fn start_resumption(mut self, sess: &mut ServerSessionImpl, client_hello: &ClientHelloPayload, - sni: Option<&ServerName>, + sni: Option<&webpki::DNSName>, id: &SessionID, resumedata: persist::ServerSessionValue) -> StateResult { @@ -750,7 +763,7 @@ impl ExpectClientHello { emit_ccs(sess); emit_finished(&mut self.handshake, sess); - assert!(same_hostname_or_both_none(sni, sess.get_sni())); + assert!(same_dns_name_or_both_none(sni, sess.get_sni())); Ok(self.into_expect_tls12_ccs()) } @@ -806,10 +819,8 @@ impl ExpectClientHello { } } - self.cross_check_certificate_and_save_sni(sess, - client_hello.get_sni_extension() - .and_then(|sni| sni.get_hostname()), - &server_key)?; + let sni = self.get_sni_dns_name(sess, client_hello)?; + self.cross_check_certificate_and_save_sni(sess, sni, &server_key)?; let chosen_group = chosen_group.unwrap(); let chosen_share = shares_ext.iter() @@ -887,9 +898,23 @@ impl ExpectClientHello { } } + fn get_sni_dns_name(&self, sess: &mut ServerSessionImpl, + client_hello: &ClientHelloPayload) + -> Result, TLSError> { + match client_hello.get_sni_extension() + .and_then(|sni| sni.get_hostname()) + .and_then(|sni| sni.get_hostname_str()) { + Some(sni) => + webpki::DNSNameRef::try_from_ascii_str(sni) + .map(|dns_name_ref| Some(webpki::DNSName::from(dns_name_ref))) + .map_err(|()| illegal_param(sess, "ClientHello SNI DNS name is invalid.")), + None => Ok(None), + } + } + fn cross_check_certificate_and_save_sni(&self, sess: &mut ServerSessionImpl, - sni: Option<&ServerName>, + sni: Option, certkey: &sign::CertifiedKey) -> Result<(), TLSError> { // Always reject an empty certificate chain. let end_entity_cert = certkey.end_entity_cert().map_err(|()| { @@ -905,10 +930,7 @@ impl ExpectClientHello { "end-entity certificate in certificate chain is syntactically invalid".to_string()) })?; - if let Some(sni) = sni { - let sni_str = sni.get_hostname_str() - .unwrap(); - + if let Some(ref sni) = sni { // If SNI was offered then the certificate must be valid for // that hostname. Note that this doesn't fully validate that the // certificate is valid; it only validates that the name is one @@ -916,18 +938,17 @@ impl ExpectClientHello { // valid. Indirectly, this also verifies that the SNI is a // syntactically-valid hostname, according to Web PKI rules, // which may differ from DNS and/or URL rules. - if !end_entity_cert.verify_is_valid_for_dns_name( - untrusted::Input::from(sni_str.as_bytes())).is_ok() { + if !end_entity_cert.verify_is_valid_for_dns_name(sni.as_ref()).is_ok() { sess.common.send_fatal_alert(AlertDescription::InternalError); return Err(TLSError::General( "The server certificate is not valid for the given SNI name".to_string())); } // Save the SNI into the session after it's been validated. - sess.set_sni(sni); + sess.set_sni(sni.clone()); } - assert!(same_hostname_or_both_none(sni, sess.get_sni())); + assert!(same_dns_name_or_both_none(sni.as_ref(), sess.get_sni())); Ok(()) } } @@ -979,22 +1000,28 @@ impl State for ExpectClientHello { // Common to TLS1.2 and TLS1.3: ciphersuite and certificate selection. let default_sigschemes_ext = SupportedSignatureSchemes::default(); - let sni = client_hello.get_sni_extension() - .and_then(|sni| sni.get_hostname()); + // Extract and validate the SNI DNS name, if any, before giving it to + // the cert resolver. In particular, if it is invalid then we should + // send an Illegal Parameter alert instead of the Internal Error alert + // (or whatever) that we'd send if this were checked later or in a + // different way. + let sni = self.get_sni_dns_name(sess, client_hello)?; + + let sigschemes_ext = client_hello.get_sigalgs_extension() + .unwrap_or(&default_sigschemes_ext); // Choose a certificate. - let sni_str = sni.and_then(|sni| sni.get_hostname_str()); - // XXX: Ideally we'd verify that the SNI hostname is syntactically valid - // before logging it and before giving it to the cert_resolver. - debug!("sni {:?}", sni_str); - let sigschemes_ext = client_hello.get_sigalgs_extension() - .unwrap_or(&default_sigschemes_ext); - debug!("sig schemes {:?}", sigschemes_ext); - let certkey = sess.config.cert_resolver.resolve(sni_str, sigschemes_ext); - let mut certkey = certkey.ok_or_else(|| { - sess.common.send_fatal_alert(AlertDescription::AccessDenied); - TLSError::General("no server certificate chain resolved".to_string()) - })?; + let mut certkey = { + let sni_str: Option<&str> = + sni.as_ref().map(|dns_name| dns_name.as_ref().into()); + debug!("sni {:?}", sni_str); + debug!("sig schemes {:?}", sigschemes_ext); + let certkey = sess.config.cert_resolver.resolve(sni_str, sigschemes_ext); + certkey.ok_or_else(|| { + sess.common.send_fatal_alert(AlertDescription::AccessDenied); + TLSError::General("no server certificate chain resolved".to_string()) + })? + }; // Reduce our supported ciphersuites by the certificate. // (no-op for TLS1.3) @@ -1030,7 +1057,7 @@ impl State for ExpectClientHello { } // -- TLS1.2 only from hereon in -- - self.cross_check_certificate_and_save_sni(sess, sni, &certkey)?; + self.cross_check_certificate_and_save_sni(sess, sni.clone(), &certkey)?; self.handshake.transcript.add_message(&m); // Save their Random. @@ -1080,7 +1107,7 @@ impl State for ExpectClientHello { if can_resume(sess, &self.handshake, &maybe_resume) { return self.start_resumption(sess, - client_hello, sni, + client_hello, sni.as_ref(), &client_hello.session_id, maybe_resume.unwrap()); } else { @@ -1098,7 +1125,7 @@ impl State for ExpectClientHello { if can_resume(sess, &self.handshake, &maybe_resume) { return self.start_resumption(sess, - client_hello, sni, + client_hello, sni.as_ref(), &client_hello.session_id, maybe_resume.unwrap()); } diff --git a/src/server/mod.rs b/src/server/mod.rs index e9a46e9f..924724b4 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -2,7 +2,7 @@ use session::{Session, SessionSecrets, SessionCommon}; use suites::{SupportedCipherSuite, ALL_CIPHERSUITES}; use msgs::enums::{ContentType, SignatureScheme}; use msgs::enums::{AlertDescription, HandshakeType, ProtocolVersion}; -use msgs::handshake::{SessionID, ServerName}; +use msgs::handshake::SessionID; use msgs::message::Message; use msgs::codec::Codec; use error::TLSError; @@ -11,6 +11,7 @@ use sign; use verify; use anchors; use key; +use webpki; use std::collections; use std::sync::{Arc, Mutex}; @@ -400,7 +401,7 @@ pub struct ServerSessionImpl { pub config: Arc, pub secrets: Option, pub common: SessionCommon, - sni: Option, + sni: Option, pub alpn_protocol: Option, pub error: Option, pub state: Option>, @@ -556,14 +557,14 @@ impl ServerSessionImpl { self.common.negotiated_version } - pub fn get_sni(&self)-> Option<&ServerName> { + pub fn get_sni(&self)-> Option<&webpki::DNSName> { self.sni.as_ref() } - pub fn set_sni(&mut self, value: &ServerName) { + pub fn set_sni(&mut self, value: webpki::DNSName) { // The SNI hostname is immutable once set. assert!(self.sni.is_none()); - self.sni = Some(value.clone()) + self.sni = Some(value) } } @@ -601,7 +602,7 @@ impl ServerSession { /// The SNI hostname is also used to match sessions during session /// resumption. pub fn get_sni_hostname(&self)-> Option<&str> { - self.imp.get_sni().and_then(|s| s.get_hostname_str()) + self.imp.get_sni().map(|s| s.as_ref().into()) } } diff --git a/src/verify.rs b/src/verify.rs index ee280631..b24597e8 100644 --- a/src/verify.rs +++ b/src/verify.rs @@ -93,7 +93,11 @@ impl ServerCertVerifier for WebPKIVerifier { info!("Unvalidated OCSP response: {:?}", ocsp_response.to_vec()); } - cert.verify_is_valid_for_dns_name(untrusted::Input::from(dns_name.as_bytes())) + let dns_name = webpki::DNSNameRef::try_from_ascii( + untrusted::Input::from(dns_name.as_bytes())) + .map_err(|()| TLSError::InvalidDNSName(dns_name.into()))?; + + cert.verify_is_valid_for_dns_name(dns_name) .map_err(TLSError::WebPKIError) .map(|_| ServerCertVerified::assertion()) }