From f7e6d7fce60baa8cb4f1b3351f0b3fee40c42eab Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 18 Oct 2016 21:05:37 -0700 Subject: [PATCH] Don't ignore errors in NPN/ALPN logic Closes #479 --- openssl/src/ssl/mod.rs | 23 ++++++++++++++++++----- openssl/src/ssl/tests/mod.rs | 28 ++++++++++++++-------------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/openssl/src/ssl/mod.rs b/openssl/src/ssl/mod.rs index 11ecd32d..cd7fe7b9 100644 --- a/openssl/src/ssl/mod.rs +++ b/openssl/src/ssl/mod.rs @@ -571,7 +571,7 @@ impl<'a> SslContextRef<'a> { /// Set the protocols to be used during Next Protocol Negotiation (the protocols /// supported by the application). - pub fn set_npn_protocols(&mut self, protocols: &[&[u8]]) { + pub fn set_npn_protocols(&mut self, protocols: &[&[u8]]) -> Result<(), ErrorStack> { // Firstly, convert the list of protocols to a byte-array that can be passed to OpenSSL // APIs -- a list of length-prefixed strings. let protocols: Box> = Box::new(ssl_encode_byte_strings(protocols)); @@ -579,7 +579,9 @@ impl<'a> SslContextRef<'a> { unsafe { // Attach the protocol list to the OpenSSL context structure, // so that we can refer to it within the callback. - ffi::SSL_CTX_set_ex_data(self.as_ptr(), *NPN_PROTOS_IDX, mem::transmute(protocols)); + try!(cvt(ffi::SSL_CTX_set_ex_data(self.as_ptr(), + *NPN_PROTOS_IDX, + Box::into_raw(protocols) as *mut c_void))); // Now register the callback that performs the default protocol // matching based on the client-supported list of protocols that // has been saved. @@ -591,6 +593,7 @@ impl<'a> SslContextRef<'a> { ffi::SSL_CTX_set_next_protos_advertised_cb(self.as_ptr(), raw_next_protos_advertise_cb, ptr::null_mut()); + Ok(()) } } @@ -603,22 +606,32 @@ impl<'a> SslContextRef<'a> { /// /// Requires the `v102` or `v110` features and OpenSSL 1.0.2 or OpenSSL 1.1.0. #[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))] - pub fn set_alpn_protocols(&mut self, protocols: &[&[u8]]) { + pub fn set_alpn_protocols(&mut self, protocols: &[&[u8]]) -> Result<(), ErrorStack> { let protocols: Box> = Box::new(ssl_encode_byte_strings(protocols)); unsafe { // Set the context's internal protocol list for use if we are a server - ffi::SSL_CTX_set_alpn_protos(self.as_ptr(), protocols.as_ptr(), protocols.len() as c_uint); + let r = ffi::SSL_CTX_set_alpn_protos(self.as_ptr(), + protocols.as_ptr(), + protocols.len() as c_uint); + // fun fact, SSL_CTX_set_alpn_protos has a reversed return code D: + if r != 0 { + return Err(ErrorStack::get()); + } // Rather than use the argument to the callback to contain our data, store it in the // ssl ctx's ex_data so that we can configure a function to free it later. In the // future, it might make sense to pull this into our internal struct Ssl instead of // leaning on openssl and using function pointers. - ffi::SSL_CTX_set_ex_data(self.as_ptr(), *ALPN_PROTOS_IDX, mem::transmute(protocols)); + try!(cvt(ffi::SSL_CTX_set_ex_data(self.as_ptr(), + *ALPN_PROTOS_IDX, + Box::into_raw(protocols) as *mut c_void))); // Now register the callback that performs the default protocol // matching based on the client-supported list of protocols that // has been saved. ffi::SSL_CTX_set_alpn_select_cb(self.as_ptr(), raw_alpn_select_cb, ptr::null_mut()); + + Ok(()) } } } diff --git a/openssl/src/ssl/tests/mod.rs b/openssl/src/ssl/tests/mod.rs index 051a12f5..966df3c5 100644 --- a/openssl/src/ssl/tests/mod.rs +++ b/openssl/src/ssl/tests/mod.rs @@ -514,7 +514,7 @@ fn test_connect_with_unilateral_alpn() { let (_s, stream) = Server::new(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -535,7 +535,7 @@ fn test_connect_with_unilateral_npn() { let (_s, stream) = Server::new(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_npn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_npn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -557,7 +557,7 @@ fn test_connect_with_alpn_successful_multiple_matching() { let (_s, stream) = Server::new_alpn(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"spdy/3.1", b"http/1.1"]); + ctx.set_alpn_protocols(&[b"spdy/3.1", b"http/1.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -579,7 +579,7 @@ fn test_connect_with_npn_successful_multiple_matching() { let (_s, stream) = Server::new_alpn(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_npn_protocols(&[b"spdy/3.1", b"http/1.1"]); + ctx.set_npn_protocols(&[b"spdy/3.1", b"http/1.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -602,7 +602,7 @@ fn test_connect_with_alpn_successful_single_match() { let (_s, stream) = Server::new_alpn(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -626,7 +626,7 @@ fn test_connect_with_npn_successful_single_match() { let (_s, stream) = Server::new_alpn(); let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_npn_protocols(&[b"spdy/3.1"]); + ctx.set_npn_protocols(&[b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -650,7 +650,7 @@ fn test_npn_server_advertise_multiple() { let listener_ctx = { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_npn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_npn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); assert!(ctx.set_certificate_file(&Path::new("test/cert.pem"), X509FileType::PEM) .is_ok()); ctx.set_private_key_file(&Path::new("test/key.pem"), X509FileType::PEM) @@ -665,7 +665,7 @@ fn test_npn_server_advertise_multiple() { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_npn_protocols(&[b"spdy/3.1"]); + ctx.set_npn_protocols(&[b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -691,7 +691,7 @@ fn test_alpn_server_advertise_multiple() { let listener_ctx = { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); assert!(ctx.set_certificate_file(&Path::new("test/cert.pem"), X509FileType::PEM) .is_ok()); ctx.set_private_key_file(&Path::new("test/key.pem"), X509FileType::PEM) @@ -706,7 +706,7 @@ fn test_alpn_server_advertise_multiple() { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"spdy/3.1"]).unwrap(); match ctx.set_CA_file(&Path::new("test/root-ca.pem")) { Ok(_) => {} Err(err) => panic!("Unexpected error {:?}", err), @@ -732,7 +732,7 @@ fn test_alpn_server_select_none() { let listener_ctx = { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); assert!(ctx.set_certificate_file(&Path::new("test/cert.pem"), X509FileType::PEM) .is_ok()); ctx.set_private_key_file(&Path::new("test/key.pem"), X509FileType::PEM) @@ -747,7 +747,7 @@ fn test_alpn_server_select_none() { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/2"]); + ctx.set_alpn_protocols(&[b"http/2"]).unwrap(); ctx.set_CA_file(&Path::new("test/root-ca.pem")).unwrap(); // Now connect to the socket and make sure the protocol negotiation works... let stream = TcpStream::connect(localhost).unwrap(); @@ -767,7 +767,7 @@ fn test_alpn_server_select_none() { let listener_ctx = { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]); + ctx.set_alpn_protocols(&[b"http/1.1", b"spdy/3.1"]).unwrap(); assert!(ctx.set_certificate_file(&Path::new("test/cert.pem"), X509FileType::PEM) .is_ok()); ctx.set_private_key_file(&Path::new("test/key.pem"), X509FileType::PEM) @@ -782,7 +782,7 @@ fn test_alpn_server_select_none() { let mut ctx = SslContext::new(SslMethod::tls()).unwrap(); ctx.set_verify(SSL_VERIFY_PEER); - ctx.set_alpn_protocols(&[b"http/2"]); + ctx.set_alpn_protocols(&[b"http/2"]).unwrap(); ctx.set_CA_file(&Path::new("test/root-ca.pem")).unwrap(); // Now connect to the socket and make sure the protocol negotiation works... let stream = TcpStream::connect(localhost).unwrap();