From 46d5fc03a569743778956cf6901dfc05c8862e89 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Sat, 13 May 2017 13:52:45 +0100 Subject: [PATCH] Add tests for dangerous_configuration Also: - use it for bogo_shim, which previously used DANGEROUS_DISABLE_VERIFY. bogo_shim now only built with dangerous_configuration. - require a non-empty certificate list outside the external verifier; this is a internal invariant. - Abolish ASN1Cert in preference to key::Certificate --- .travis.yml | 1 + Cargo.toml | 1 + bogo/runme | 5 +---- examples/internal/bogo_shim.rs | 33 ++++++++++++++++++++++++++++++--- src/client_hs.rs | 10 +++++++++- src/msgs/handshake.rs | 7 +++---- src/verify.rs | 16 ++++++++-------- tests/badssl.rs | 12 ++++++++++++ tests/common/mod.rs | 11 +++++++++++ 9 files changed, 76 insertions(+), 20 deletions(-) diff --git a/.travis.yml b/.travis.yml index 52128e6a..1a2114f5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,6 +13,7 @@ before_install: script: - cargo build - RUST_BACKTRACE=1 cargo test + - RUST_BACKTRACE=1 cargo test --features dangerous_configuration danger - cargo test --release --no-run - ./target/release/examples/bench - ( cd trytls && ./runme ) diff --git a/Cargo.toml b/Cargo.toml index 6b44428c..0078aec2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ regex = "0.2" [[example]] name = "bogo_shim" path = "examples/internal/bogo_shim.rs" +required-features = ["dangerous_configuration"] [[example]] name = "trytls_shim" diff --git a/bogo/runme b/bogo/runme index a27bcbc6..5104f46d 100755 --- a/bogo/runme +++ b/bogo/runme @@ -5,9 +5,7 @@ set -xe -cp ../src/verify.rs ../src/verify-secure.rs -sed -i -e 's/\(DANGEROUS_DISABLE_VERIFY: bool = \)false/\1true/g' ../src/verify.rs -cargo test --no-run +cargo test --no-run --features dangerous_configuration if [ ! -e bogo/ ] ; then ./fetch-and-build @@ -17,4 +15,3 @@ fi -shim-config ../config.json \ -test.parallel 1 -num-workers 1 \ -pipe -allow-unimplemented ) -mv ../src/verify-secure.rs ../src/verify.rs diff --git a/examples/internal/bogo_shim.rs b/examples/internal/bogo_shim.rs index 657be437..6f77ef48 100644 --- a/examples/internal/bogo_shim.rs +++ b/examples/internal/bogo_shim.rs @@ -116,6 +116,27 @@ fn split_protocols(protos: &str) -> Vec { ret } +struct NoVerification {} + +impl rustls::ClientCertVerifier for NoVerification { + fn verify_client_cert(&self, + _roots: &rustls::RootCertStore, + _certs: &[rustls::Certificate]) -> Result<(), rustls::TLSError> { + Ok(()) + } +} + +impl rustls::ServerCertVerifier for NoVerification { + fn verify_server_cert(&self, + _roots: &rustls::RootCertStore, + _certs: &[rustls::Certificate], + _hostname: &str) -> Result<(), rustls::TLSError> { + Ok(()) + } +} + +static NO_VERIFICATION: NoVerification = NoVerification {}; + fn make_server_cfg(opts: &Options) -> Arc { let mut cfg = rustls::ServerConfig::new(); let persist = rustls::ServerSessionMemoryCache::new(32); @@ -125,10 +146,13 @@ fn make_server_cfg(opts: &Options) -> Arc { let key = load_key(&opts.key_file.replace(".pem", ".rsa")); cfg.set_single_cert(cert.clone(), key); - if opts.offer_no_client_cas { - cfg.client_auth_offer = true; - } else if opts.require_any_client_cert { + if opts.offer_no_client_cas || opts.require_any_client_cert { cfg.client_auth_offer = true; + cfg.dangerous() + .set_certificate_verifier(&NO_VERIFICATION); + } + + if opts.require_any_client_cert { cfg.client_auth_mandatory = true; } @@ -165,6 +189,9 @@ fn make_client_cfg(opts: &Options) -> Arc { cfg.set_single_client_cert(cert, key); } + cfg.dangerous() + .set_certificate_verifier(&NO_VERIFICATION); + if !opts.protocols.is_empty() { cfg.set_protocols(&opts.protocols); } diff --git a/src/client_hs.rs b/src/client_hs.rs index 208f343e..c300aa4c 100644 --- a/src/client_hs.rs +++ b/src/client_hs.rs @@ -840,13 +840,17 @@ fn handle_certificate_verify(sess: &mut ClientSessionImpl, info!("Server cert is {:?}", sess.handshake_data.server_cert_chain); // 1. Verify the certificate chain. - // 2. Verify their signature on the handshake. + if sess.handshake_data.server_cert_chain.is_empty() { + return Err(TLSError::NoCertificatesPresented); + } + try! { sess.config.get_verifier().verify_server_cert(&sess.config.root_store, &sess.handshake_data.server_cert_chain, &sess.handshake_data.dns_name) }; + // 2. Verify their signature on the handshake. let handshake_hash = sess.handshake_data.transcript.get_current_hash(); try! { verify::verify_tls13(&sess.handshake_data.server_cert_chain[0], @@ -1092,6 +1096,10 @@ fn handle_server_hello_done(sess: &mut ClientSessionImpl, // 5. emit a Finished, our first encrypted message under the new keys. // 1. + if sess.handshake_data.server_cert_chain.is_empty() { + return Err(TLSError::NoCertificatesPresented); + } + try! { sess.config.get_verifier().verify_server_cert(&sess.config.root_store, &sess.handshake_data.server_cert_chain, diff --git a/src/msgs/handshake.rs b/src/msgs/handshake.rs index bc0c0b8f..b4cbb843 100644 --- a/src/msgs/handshake.rs +++ b/src/msgs/handshake.rs @@ -1159,7 +1159,6 @@ impl ServerHelloPayload { } } -pub type ASN1Cert = key::Certificate; pub type CertificatePayload = Vec; impl Codec for CertificatePayload { @@ -1218,7 +1217,7 @@ declare_u16_vec!(CertificateExtensions, CertificateExtension); #[derive(Debug)] pub struct CertificateEntry { - pub cert: ASN1Cert, + pub cert: key::Certificate, pub exts: CertificateExtensions, } @@ -1230,14 +1229,14 @@ impl Codec for CertificateEntry { fn read(r: &mut Reader) -> Option { Some(CertificateEntry { - cert: try_ret!(ASN1Cert::read(r)), + cert: try_ret!(key::Certificate::read(r)), exts: try_ret!(CertificateExtensions::read(r)), }) } } impl CertificateEntry { - pub fn new(cert: ASN1Cert) -> CertificateEntry { + pub fn new(cert: key::Certificate) -> CertificateEntry { CertificateEntry { cert: cert, exts: Vec::new(), diff --git a/src/verify.rs b/src/verify.rs index add318d7..e284a7f8 100644 --- a/src/verify.rs +++ b/src/verify.rs @@ -2,7 +2,7 @@ use webpki; use time; use untrusted; -use msgs::handshake::ASN1Cert; +use key::Certificate; use msgs::handshake::DigitallySignedStruct; use msgs::enums::SignatureScheme; use error::TLSError; @@ -32,7 +32,7 @@ pub trait ServerCertVerifier : Send + Sync { /// the top certificate in the chain. fn verify_server_cert(&self, roots: &RootCertStore, - presented_certs: &[ASN1Cert], + presented_certs: &[Certificate], dns_name: &str) -> Result<(), TLSError>; } @@ -42,7 +42,7 @@ pub trait ClientCertVerifier : Send + Sync { /// Does no further checking of the certificate. fn verify_client_cert(&self, roots: &RootCertStore, - presented_certs: &[ASN1Cert]) -> Result<(), TLSError>; + presented_certs: &[Certificate]) -> Result<(), TLSError>; } pub struct WebPKIVerifier {} @@ -51,7 +51,7 @@ pub static WEB_PKI: WebPKIVerifier = WebPKIVerifier {}; impl ServerCertVerifier for WebPKIVerifier { fn verify_server_cert(&self, roots: &RootCertStore, - presented_certs: &[ASN1Cert], + presented_certs: &[Certificate], dns_name: &str) -> Result<(), TLSError> { let cert = try!(self.verify_common_cert(roots, presented_certs)); @@ -63,7 +63,7 @@ impl ServerCertVerifier for WebPKIVerifier { impl ClientCertVerifier for WebPKIVerifier { fn verify_client_cert(&self, roots: &RootCertStore, - presented_certs: &[ASN1Cert]) -> Result<(), TLSError> { + presented_certs: &[Certificate]) -> Result<(), TLSError> { self.verify_common_cert(roots, presented_certs).map(|_| ()) } } @@ -74,7 +74,7 @@ impl WebPKIVerifier { /// in `presented_certs`. fn verify_common_cert<'a>(&self, roots: &RootCertStore, - presented_certs: &'a [ASN1Cert]) + presented_certs: &'a [Certificate]) -> Result, TLSError> { if presented_certs.is_empty() { return Err(TLSError::NoCertificatesPresented); @@ -163,7 +163,7 @@ fn verify_sig_using_any_alg(cert: &webpki::EndEntityCert, /// `cert` MUST have been authenticated before using this function, /// typically using `verify_cert`. pub fn verify_signed_struct(message: &[u8], - cert: &ASN1Cert, + cert: &Certificate, dss: &DigitallySignedStruct) -> Result<(), TLSError> { @@ -195,7 +195,7 @@ fn convert_alg_tls13(scheme: SignatureScheme) } } -pub fn verify_tls13(cert: &ASN1Cert, +pub fn verify_tls13(cert: &Certificate, dss: &DigitallySignedStruct, handshake_hash: &[u8], context_string_with_0: &[u8]) diff --git a/tests/badssl.rs b/tests/badssl.rs index 2e44c80f..8249ba03 100644 --- a/tests/badssl.rs +++ b/tests/badssl.rs @@ -148,4 +148,16 @@ mod online { .unwrap(); } + #[cfg(feature = "dangerous_configuration")] + mod danger { + #[test] + fn self_signed() { + super::polite(); + super::connect("self-signed.badssl.com") + .insecure() + .expect("self-signed.badssl.com") + .go() + .unwrap(); + } + } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index d025cfff..ee822121 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -113,6 +113,7 @@ pub struct TlsClient { pub suites: Vec, pub protos: Vec, pub no_tickets: bool, + pub insecure: bool, pub verbose: bool, pub mtu: Option, pub expect_fails: bool, @@ -131,6 +132,7 @@ impl TlsClient { client_auth_certs: None, cache: None, no_tickets: false, + insecure: false, verbose: false, mtu: None, suites: Vec::new(), @@ -162,6 +164,11 @@ impl TlsClient { self } + pub fn insecure(&mut self) -> &mut TlsClient { + self.insecure = true; + self + } + pub fn verbose(&mut self) -> &mut TlsClient { self.verbose = true; self @@ -225,6 +232,10 @@ impl TlsClient { args.push("--no-tickets"); } + if self.insecure { + args.push("--insecure"); + } + if self.cafile.is_some() { args.push("--cafile"); args.push(self.cafile.as_ref().unwrap());