Merge #182

182: Initial changes for async support r=jethrogb a=AdrianCX

Splitting #163 into separate parts.

Also rebased since `@raoulstrackx` already helped with some pieces of the PR above.

Code does the following:
- Minor update to comments on Pk (revisited the latest code to check if any issues, none found)
- verify with sig len 0 makes mbedtls assume default siglen and potentially overflow instead of stopping.
- EntropyHolder can have different sources, either shared or not (request from another older PR)
- Ported ALPN changes from `@mzohreva` branch - NullTerminatedList
- Split off HandshakeContext and made it a sort of base class to Context so we can use it safely. (lots of ideas from `@jethrogb` here)
- Made Context < T >  as opposed to using Any, makes life easier for async and a lot of other use cases.

Co-authored-by: Adrian Cruceru <adrian.cruceru@fortanix.com>

Author: bors[bot]

mbedtls/src/pk/mod.rs

@@ -832,6 +832,11 @@ impl Pk {
         sig: &mut [u8],
         rng: &mut F,
     ) -> Result<usize> {
+        // If hash or sig are allowed with size 0 (&[]) then mbedtls will attempt to auto-detect size and cause an invalid write.
+        if hash.len() == 0 || sig.len() == 0 {
+            return Err(Error::PkBadInputData)
+        }
+
         match self.pk_type() {
             Type::Rsa | Type::RsaAlt | Type::RsassaPss => {
                 if sig.len() < (self.len() / 8) {
@@ -868,6 +873,11 @@ impl Pk {
         sig: &mut [u8],
         rng: &mut F,
     ) -> Result<usize> {
+        // If hash or sig are allowed with size 0 (&[]) then mbedtls will attempt to auto-detect size and cause an invalid write.
+        if hash.len() == 0 || sig.len() == 0 {
+            return Err(Error::PkBadInputData)
+        }
+
         use crate::rng::RngCallbackMut;
 
         if self.pk_type() == Type::Ecdsa || self.pk_type() == Type::Eckey {
@@ -913,6 +923,11 @@ impl Pk {
     }
 
     pub fn verify(&mut self, md: MdType, hash: &[u8], sig: &[u8]) -> Result<()> {
+        // If hash or sig are allowed with size 0 (&[]) then mbedtls will attempt to auto-detect size and cause an invalid write.
+        if hash.len() == 0 || sig.len() == 0 {
+            return Err(Error::PkBadInputData)
+        }
+        
         unsafe {
             pk_verify(
                 &mut self.inner,
@@ -1274,6 +1289,18 @@ iy6KC991zzvaWY/Ys+q/84Afqa+0qJKQnPuy/7F5GkVdQA/lfbhi
                 )
                 .unwrap();
             pk.verify(digest, data, &signature[0..len]).unwrap();
+
+            assert_eq!(pk.verify(digest, data, &[]).unwrap_err(), Error::PkBadInputData);
+            assert_eq!(pk.verify(digest, &[], &signature[0..len]).unwrap_err(), Error::PkBadInputData);
+
+
+            let mut dummy_sig = [];
+            assert_eq!(pk.sign(digest, data, &mut dummy_sig, &mut crate::test_support::rand::test_rng()).unwrap_err(), Error::PkBadInputData);
+            assert_eq!(pk.sign(digest, &[], &mut signature, &mut crate::test_support::rand::test_rng()).unwrap_err(), Error::PkBadInputData);
+            
+            assert_eq!(pk.sign_deterministic(digest, data, &mut dummy_sig, &mut crate::test_support::rand::test_rng()).unwrap_err(), Error::PkBadInputData);
+            assert_eq!(pk.sign_deterministic(digest, &[], &mut signature, &mut crate::test_support::rand::test_rng()).unwrap_err(), Error::PkBadInputData);
+
         }
     }
 

mbedtls/src/rng/ctr_drbg.rs

@@ -17,7 +17,12 @@ use mbedtls_sys::types::size_t;
 #[cfg(not(feature = "std"))]
 use crate::alloc_prelude::*;
 use crate::error::{IntoResult, Result};
-use crate::rng::{EntropyCallback, RngCallback, RngCallbackMut};
+use crate::rng::{EntropyCallback, EntropyCallbackMut, RngCallback, RngCallbackMut};
+
+enum EntropyHolder {
+    Shared(Arc<dyn EntropyCallback + 'static>),
+    Unique(Box<dyn EntropyCallbackMut + 'static>),
+}
 
 define!(
     // `ctr_drbg_context` inlines an `aes_context`, which is immovable. See
@@ -30,7 +35,7 @@ define!(
     #[c_box_ty(ctr_drbg_context)]
     #[repr(C)]
     struct CtrDrbg {
-        entropy: Arc<dyn EntropyCallback + 'static>,
+        entropy: EntropyHolder,
     };
     const drop: fn(&mut Self) = ctr_drbg_free;
     impl<'a> Into<ptr> {}
@@ -63,8 +68,28 @@ impl CtrDrbg {
             ).into_result()?;
         }
 
-        Ok(CtrDrbg { inner, entropy })
+        Ok(CtrDrbg { inner, entropy: EntropyHolder::Shared(entropy) })
+    }
+
+    pub fn with_mut_entropy<T: EntropyCallbackMut + 'static>(entropy: T, additional_entropy: Option<&[u8]>) -> Result<Self> {
+        let mut inner = Box::new(ctr_drbg_context::default());
+
+        // We take sole ownership of entropy, all access is guarded via mutexes.
+        let mut entropy = Box::new(entropy);
+        unsafe {
+            ctr_drbg_init(&mut *inner);
+            ctr_drbg_seed(
+                &mut *inner,
+                Some(T::call_mut),
+                entropy.data_ptr_mut(),
+                additional_entropy.map(<[_]>::as_ptr).unwrap_or(::core::ptr::null()),
+                additional_entropy.map(<[_]>::len).unwrap_or(0)
+            ).into_result()?;
+        }
+
+        Ok(CtrDrbg { inner, entropy: EntropyHolder::Unique(entropy) })
     }
+
 
     pub fn prediction_resistance(&self) -> bool {
         if self.inner.prediction_resistance == CTR_DRBG_PR_OFF {

mbedtls/src/ssl/config.rs

@@ -100,6 +100,46 @@ callback!(DbgCallback: Fn(i32, Cow<'_, str>, i32, Cow<'_, str>) -> ());
 callback!(SniCallback: Fn(&mut HandshakeContext, &[u8]) -> Result<()>);
 callback!(CaCallback: Fn(&MbedtlsList<Certificate>) -> Result<MbedtlsList<Certificate>>);
 
+
+#[repr(transparent)]
+pub struct NullTerminatedStrList {
+    c: Vec<*mut c_char>,
+}
+
+unsafe impl Send for NullTerminatedStrList {}
+unsafe impl Sync for NullTerminatedStrList {}
+
+impl NullTerminatedStrList {
+    #[cfg(feature = "std")]
+    pub fn new(list: &[&str]) -> Result<Self> {
+        let mut ret = NullTerminatedStrList { c: Vec::with_capacity(list.len() + 1) };
+
+        for item in list {
+            ret.c.push(::std::ffi::CString::new(*item).map_err(|_| Error::SslBadInputData)?.into_raw());
+        }
+        
+        ret.c.push(core::ptr::null_mut()); 
+        Ok(ret)
+    }
+
+    pub fn as_ptr(&self) -> *const *const c_char {
+        self.c.as_ptr() as *const _
+    }
+}
+
+#[cfg(feature = "std")]
+impl Drop for NullTerminatedStrList {
+    fn drop(&mut self) {
+        for i in self.c.iter() {
+            unsafe {
+                if !(*i).is_null() {
+                    let _ = ::std::ffi::CString::from_raw(*i);
+                }
+            }
+        }
+    }
+}
+
 define!(
     #[c_ty(ssl_config)]
     #[repr(C)]
@@ -116,9 +156,7 @@ define!(
 
         ciphersuites: Vec<Arc<Vec<c_int>>>,
         curves: Option<Arc<Vec<ecp_group_id>>>,
-        
-        #[allow(dead_code)]
-        dhm: Option<Arc<Dhm>>,
+        protocols: Option<Arc<NullTerminatedStrList>>,
 
         verify_callback: Option<Arc<dyn VerifyCallback + 'static>>,
         #[cfg(feature = "std")]
@@ -154,7 +192,7 @@ impl Config {
             rng: None,
             ciphersuites: vec![],
             curves: None,
-            dhm: None,
+            protocols: None,
             verify_callback: None,
             #[cfg(feature = "std")]
             dbg_callback: None,
@@ -184,6 +222,18 @@ impl Config {
         self.ciphersuites.push(list);
     }
 
+    /// Set the supported Application Layer Protocols.
+    pub fn set_alpn_protocols(&mut self, protocols: Arc<NullTerminatedStrList>) -> Result<()> {
+        unsafe {
+            ssl_conf_alpn_protocols(&mut self.inner, protocols.as_ptr() as *mut _)
+                .into_result()
+                .map(|_| ())?;
+        }
+
+        self.protocols = Some(protocols);
+        Ok(())
+    }
+    
     pub fn set_ciphersuites_for_version(&mut self, list: Arc<Vec<c_int>>, major: c_int, minor: c_int) {
         Self::check_c_list(&list);
         unsafe { ssl_conf_ciphersuites_for_version(self.into(), list.as_ptr(), major, minor) }
@@ -232,13 +282,13 @@ impl Config {
     /// Takes both DER and PEM forms of FFDH parameters in `DHParams` format.
     ///
     /// When calling on PEM-encoded data, `params` must be NULL-terminated
-    pub fn set_dh_params(&mut self, dhm: Arc<Dhm>) -> Result<()> {
+    pub fn set_dh_params(&mut self, dhm: &Dhm) -> Result<()> {
         unsafe {
+            // This copies the dhm parameters and does not store any pointer to it
             ssl_conf_dh_param_ctx(self.into(), dhm.inner_ffi_mut())
                 .into_result()
                 .map(|_| ())?;
         }
-        self.dhm = Some(dhm);
         Ok(())
     }
 
@@ -316,12 +366,10 @@ impl Config {
             // - We can pointer cast to it to allow storing additional objects.
             //
             let cb = &mut *(closure as *mut F);
-            let context = UnsafeFrom::from(ctx).unwrap();
-            
-            let mut ctx = HandshakeContext::init(context);
+            let ctx = UnsafeFrom::from(ctx).unwrap();
 
             let name = from_raw_parts(name, name_len);
-            match cb(&mut ctx, name) {
+            match cb(ctx, name) {
                 Ok(()) => 0,
                 Err(_) => -1,
             }