From f7e6d7fce60baa8cb4f1b3351f0b3fee40c42eab Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 18 Oct 2016 21:05:37 -0700 Subject: [PATCH 1/2] 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(); From c4459c37d98b3a56723e6698852698fb2f354d47 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 18 Oct 2016 21:11:23 -0700 Subject: [PATCH 2/2] Callback cleanup --- openssl-sys/src/lib.rs | 24 ++++++++++----------- openssl/src/crypto/util.rs | 16 +++++++------- openssl/src/ssl/mod.rs | 43 ++++++++++++++------------------------ 3 files changed, 36 insertions(+), 47 deletions(-) diff --git a/openssl-sys/src/lib.rs b/openssl-sys/src/lib.rs index ae38d1b3..aae2540c 100644 --- a/openssl-sys/src/lib.rs +++ b/openssl-sys/src/lib.rs @@ -78,19 +78,19 @@ pub type BN_ULONG = libc::c_ulonglong; #[cfg(target_pointer_width = "32")] pub type BN_ULONG = c_uint; -pub type CRYPTO_EX_new = extern "C" fn(parent: *mut c_void, ptr: *mut c_void, - ad: *const CRYPTO_EX_DATA, idx: c_int, - argl: c_long, argp: *const c_void) -> c_int; -pub type CRYPTO_EX_dup = extern "C" fn(to: *mut CRYPTO_EX_DATA, - from: *mut CRYPTO_EX_DATA, from_d: *mut c_void, - idx: c_int, argl: c_long, argp: *mut c_void) - -> c_int; -pub type CRYPTO_EX_free = extern "C" fn(parent: *mut c_void, ptr: *mut c_void, - ad: *mut CRYPTO_EX_DATA, idx: c_int, - argl: c_long, argp: *mut c_void); -pub type PasswordCallback = extern "C" fn(buf: *mut c_char, size: c_int, - rwflag: c_int, user_data: *mut c_void) +pub type CRYPTO_EX_new = unsafe extern fn(parent: *mut c_void, ptr: *mut c_void, + ad: *const CRYPTO_EX_DATA, idx: c_int, + argl: c_long, argp: *const c_void) -> c_int; +pub type CRYPTO_EX_dup = unsafe extern fn(to: *mut CRYPTO_EX_DATA, + from: *mut CRYPTO_EX_DATA, from_d: *mut c_void, + idx: c_int, argl: c_long, argp: *mut c_void) -> c_int; +pub type CRYPTO_EX_free = unsafe extern fn(parent: *mut c_void, ptr: *mut c_void, + ad: *mut CRYPTO_EX_DATA, idx: c_int, + argl: c_long, argp: *mut c_void); +pub type PasswordCallback = unsafe extern fn(buf: *mut c_char, size: c_int, + rwflag: c_int, user_data: *mut c_void) + -> c_int; pub const BIO_TYPE_NONE: c_int = 0; diff --git a/openssl/src/crypto/util.rs b/openssl/src/crypto/util.rs index c11285f8..07099b7c 100644 --- a/openssl/src/crypto/util.rs +++ b/openssl/src/crypto/util.rs @@ -36,16 +36,16 @@ impl Drop for CallbackState { /// Password callback function, passed to private key loading functions. /// /// `cb_state` is expected to be a pointer to a `CallbackState`. -pub extern "C" fn invoke_passwd_cb(buf: *mut c_char, - size: c_int, - _rwflag: c_int, - cb_state: *mut c_void) - -> c_int - where F: FnOnce(&mut [c_char]) -> usize { +pub unsafe extern fn invoke_passwd_cb(buf: *mut c_char, + size: c_int, + _rwflag: c_int, + cb_state: *mut c_void) + -> c_int + where F: FnOnce(&mut [c_char]) -> usize { let result = panic::catch_unwind(|| { // build a `i8` slice to pass to the user callback - let pass_slice = unsafe { slice::from_raw_parts_mut(buf, size as usize) }; - let callback = unsafe { &mut *(cb_state as *mut CallbackState) }; + let pass_slice = slice::from_raw_parts_mut(buf, size as usize); + let callback = &mut *(cb_state as *mut CallbackState); callback.cb.take().unwrap()(pass_slice) }); diff --git a/openssl/src/ssl/mod.rs b/openssl/src/ssl/mod.rs index cd7fe7b9..fafac45c 100644 --- a/openssl/src/ssl/mod.rs +++ b/openssl/src/ssl/mod.rs @@ -142,20 +142,20 @@ lazy_static! { static ref ALPN_PROTOS_IDX: c_int = get_new_idx::>(); } +unsafe extern fn free_data_box(_parent: *mut c_void, + ptr: *mut c_void, + _ad: *mut ffi::CRYPTO_EX_DATA, + _idx: c_int, + _argl: c_long, + _argp: *mut c_void) { + if !ptr.is_null() { + Box::::from_raw(ptr as *mut T); + } +} + /// Determine a new index to use for SSL CTX ex data. /// Registers a destruct for the data which will be called by openssl when the context is freed. fn get_new_idx() -> c_int { - extern fn free_data_box(_parent: *mut c_void, - ptr: *mut c_void, - _ad: *mut ffi::CRYPTO_EX_DATA, - _idx: c_int, - _argl: c_long, - _argp: *mut c_void) { - if !ptr.is_null() { - let _: Box = unsafe { mem::transmute(ptr) }; - } - } - unsafe { let idx = compat::get_new_idx(free_data_box::); assert!(idx >= 0); @@ -164,17 +164,6 @@ fn get_new_idx() -> c_int { } fn get_new_ssl_idx() -> c_int { - extern fn free_data_box(_parent: *mut c_void, - ptr: *mut c_void, - _ad: *mut ffi::CRYPTO_EX_DATA, - _idx: c_int, - _argl: c_long, - _argp: *mut c_void) { - if !ptr.is_null() { - let _: Box = unsafe { mem::transmute(ptr) }; - } - } - unsafe { let idx = compat::get_new_ssl_idx(free_data_box::); assert!(idx >= 0); @@ -190,7 +179,7 @@ extern fn raw_verify(preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_CTX) let ssl = ffi::X509_STORE_CTX_get_ex_data(x509_ctx, idx); let ssl_ctx = ffi::SSL_get_SSL_CTX(ssl as *const _); let verify = ffi::SSL_CTX_get_ex_data(ssl_ctx, get_verify_data_idx::()); - let verify: &F = mem::transmute(verify); + let verify: &F = &*(verify as *mut F); let ctx = X509StoreContext::new(x509_ctx); @@ -206,7 +195,7 @@ extern fn ssl_raw_verify(preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_ let ssl = ffi::X509_STORE_CTX_get_ex_data(x509_ctx, idx); let verify = ffi::SSL_get_ex_data(ssl as *const _, get_ssl_verify_data_idx::()); - let verify: &F = mem::transmute(verify); + let verify: &F = &*(verify as *mut F); let ctx = X509StoreContext::new(x509_ctx); @@ -220,7 +209,7 @@ extern fn raw_sni(ssl: *mut ffi::SSL, al: *mut c_int, _arg: *mut c_void) -> c unsafe { let ssl_ctx = ffi::SSL_get_SSL_CTX(ssl); let callback = ffi::SSL_CTX_get_ex_data(ssl_ctx, get_verify_data_idx::()); - let callback: &F = mem::transmute(callback); + let callback: &F = &*(callback as *mut F); let mut ssl = SslRef::from_ptr(ssl); match callback(&mut ssl) { @@ -250,7 +239,7 @@ unsafe fn select_proto_using(ssl: *mut ffi::SSL, // extra data. let ssl_ctx = ffi::SSL_get_SSL_CTX(ssl); let protocols = ffi::SSL_CTX_get_ex_data(ssl_ctx, ex_data); - let protocols: &Vec = mem::transmute(protocols); + let protocols: &Vec = &*(protocols as *mut Vec); // Prepare the client list parameters to be passed to the OpenSSL function... let client = protocols.as_ptr(); let client_len = protocols.len() as c_uint; @@ -313,7 +302,7 @@ extern fn raw_next_protos_advertise_cb(ssl: *mut ffi::SSL, } else { // If the pointer is valid, put the pointer to the actual byte array into the // output parameter `out`, as well as its length into `outlen`. - let protocols: &Vec = mem::transmute(protocols); + let protocols: &Vec = &*(protocols as *mut Vec); *out = protocols.as_ptr(); *outlen = protocols.len() as c_uint; }