Merge #308 #312

308: fix: return error when verify empty cert chain r=Taowyoo a=Taowyoo For #307 on master. Several back-port PRs needed for older versions. Only return X509BadInputData error when candidate certificate chain is empty because: - underlying `mbedtls` does not have null pointer check on it. - underlying `mbedtls` has null pointer check on `trust_ca` chain during the process of finding parent certificate in the chain. 312: Update CI r=Taowyoo a=Taowyoo Refactor is prime test Fix bors status problem Co-authored-by: Yuxiang Cao <[email protected]>
fortanix · Aug 30, 2023 · 1cc1f47 · 1cc1f47
3 parents 57c7efc + 2958dce + e0b9114
commit 1cc1f47
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 85 deletions.
diff --git a/.config/nextest.toml b/.config/nextest.toml
@@ -113,8 +113,8 @@ store-failure-output = true
 [[profile.default.overrides]]
 # `hyper` test targeting external Google's service, it would sometomes failed because of rate limit
 # `is_probably_prime` test heavily use rdrand, sometimes the VM where test is running has shortage on it
-filter = 'binary(=hyper) or test(=is_probably_prime)'
-retries = { backoff = "exponential", count = 4, delay = "1s", jitter = true, max-delay = "10s" }
+filter = 'binary(=hyper) or test(is_prime_tests::)'
+retries = { backoff = "exponential", count = 4, delay = "1s", jitter = true }
 test-group = 'serial-integration'
 
 [[profile.default.overrides]]

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -1,4 +1,4 @@
-name: Test
+name: CI
 
 on:
   push:
@@ -19,8 +19,8 @@ env:
   CARGO_NET_RETRY: 10
 
 jobs:
-  build:
-    name: test
+  test:
+    name: Test
     strategy:
       matrix:
         include:
@@ -74,3 +74,13 @@ jobs:
         TARGET: ${{ matrix.target }}
         ZLIB_INSTALLED: ${{ matrix.target == 'x86_64-unknown-linux-gnu' && 'true' || '' }}
         AES_NI_SUPPORT: ${{ matrix.target == 'x86_64-unknown-linux-gnu' && 'true' || '' }}
+  ci-success:
+    name: ci
+    if: always()
+    needs:
+      - test
+    runs-on: ubuntu-20.04
+    steps:
+      - run: jq --exit-status 'all(.result == "success")' <<< '${{ toJson(needs) }}'
+      - name: Done
+        run: exit 0
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bors.toml b/bors.toml
@@ -1,4 +1,4 @@
 status = [
-  "test",
+  "ci",
 ]
 timeout_sec = 36000 # ten hours
diff --git a/mbedtls/Cargo.toml b/mbedtls/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "mbedtls"
-version = "0.11.0"
+version = "0.11.1"
 authors = ["Jethro Beekman <[email protected]>"]
 build = "build.rs"
 edition = "2018"

diff --git a/mbedtls/src/ssl/config.rs b/mbedtls/src/ssl/config.rs
@@ -338,6 +338,9 @@ impl Config {
     }
 
     pub fn push_cert(&mut self, own_cert: Arc<MbedtlsList<Certificate>>, own_pk: Arc<Pk>) -> Result<()> {
+        if own_cert.is_empty() {
+            return Err(crate::error::codes::SslBadInputData.into());
+        }
         // Need to ensure own_cert/pk_key outlive the config.
         self.own_cert.push(own_cert.clone());
         self.own_pk.push(own_pk.clone());

diff --git a/mbedtls/src/ssl/context.rs b/mbedtls/src/ssl/context.rs
@@ -573,7 +573,7 @@ impl HandshakeContext {
         key: Arc<Pk>,
     ) -> Result<()> {
         // mbedtls_ssl_set_hs_own_cert does not check for NULL handshake.
-        if self.inner.private_handshake as *const _ == ::core::ptr::null() {
+        if self.inner.private_handshake as *const _ == ::core::ptr::null() || chain.is_empty() {
             return Err(codes::SslBadInputData.into());
         }
 

diff --git a/mbedtls/src/x509/certificate.rs b/mbedtls/src/x509/certificate.rs
@@ -95,15 +95,15 @@ impl Certificate {
     pub fn from_pem(pem: &[u8]) -> Result<MbedtlsBox<Certificate>> {
         let mut cert = MbedtlsBox::<Certificate>::init()?;
         unsafe { x509_crt_parse((&mut (*cert)).into(), pem.as_ptr(), pem.len()) }.into_result()?;
-        
+
         if !(*cert).inner.next.is_null() {
             // Use from_pem_multiple for parsing multiple certificates in a pem.
             return Err(codes::X509BadInputData.into());
         }
 
         Ok(cert)
     }
-    
+
     /// Input must be NULL-terminated
     pub fn from_pem_multiple(pem: &[u8]) -> Result<MbedtlsList<Certificate>> {
         let mut cert = MbedtlsBox::<Certificate>::init()?;
@@ -113,7 +113,7 @@ impl Certificate {
         list.push(cert);
         Ok(list)
     }
-    
+
     pub fn check_key_usage(&self, usage: super::KeyUsage) -> bool {
         unsafe { x509_crt_check_key_usage(&self.inner, usage.bits()) }
             .into_result()
@@ -233,6 +233,9 @@ impl Certificate {
     where
         F: VerifyCallback + 'static,
     {
+        if chain.is_empty() {
+            return Err(codes::X509BadInputData.into());
+        }
         let (f_vrfy, p_vrfy): (Option<unsafe extern "C" fn(_, _, _, _) -> _>, _) = if let Some(cb) = cb.as_ref() {
             (Some(x509::verify_callback::<F>),
             cb as *const _ as *mut c_void)
@@ -509,11 +512,11 @@ impl MbedtlsBox<Certificate> {
 
             let inner = NonNull::new(inner).ok_or(codes::X509AllocFailed)?;
             x509_crt_init(inner.as_ptr());
-            
+
             Ok(MbedtlsBox { inner: inner.cast() })
         }
     }
-    
+
     fn list_next(&self) -> Option<&MbedtlsBox<Certificate>> {
         unsafe {
             <&Option<MbedtlsBox<_>> as UnsafeFrom<_>>::from(&(**self).inner.next).unwrap().as_ref()
@@ -566,11 +569,11 @@ impl MbedtlsList<Certificate> {
         // This leaks a *mut Certificate that we can cast to x509_crt as it's transparent and has no extra fields.
         self.inner.take().map(|x| x.into_raw()).unwrap_or(core::ptr::null_mut()) as *mut x509_crt
     }
-    
+
     pub fn is_empty(&self) -> bool {
         self.inner.is_none()
     }
-    
+
     pub fn push(&mut self, certificate: MbedtlsBox<Certificate>) -> () {
         self.append(MbedtlsList::<Certificate> { inner: Some(certificate) });
     }
@@ -585,12 +588,12 @@ impl MbedtlsList<Certificate> {
             }
             prev = cur;
         }
-        
+
         // no iterations in for loop: head equals tail
         self.inner.take()
     }
 
-    
+
     pub fn pop_front(&mut self) -> Option<MbedtlsBox<Certificate>> {
         let mut ret = self.inner.take()?;
         self.inner = ret.list_next_mut().take();
@@ -605,7 +608,7 @@ impl MbedtlsList<Certificate> {
         };
         *tail = list.inner;
     }
-    
+
     pub fn iter(&self) -> Iter<'_> {
         Iter { next: self.inner.as_ref() }
     }
@@ -874,7 +877,7 @@ JS7pkcufTIoN0Yj0SxAWLW711FgB
         assert_eq!(output, TEST_PEM);
     }
 
-    
+
     const TEST_CERT_PEM: &'static str = "-----BEGIN CERTIFICATE-----
 MIIDLDCCAhSgAwIBAgIRALY0SS5pY9Yb/aIHvSAvmOswDQYJKoZIhvcNAQELBQAw
 HzEQMA4GA1UEAxMHVGVzdCBDQTELMAkGA1UEBhMCVVMwHhcNMTkwMTA4MDAxODM1
@@ -898,7 +901,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
     #[test]
     fn cert_field_access() {
         let cert = Certificate::from_pem(TEST_CERT_PEM.as_bytes()).unwrap();
-        
+
         assert_eq!(cert.version().unwrap(), CertificateVersion::V3);
         assert_eq!(cert.issuer().unwrap(), "CN=Test CA, C=US");
         assert_eq!(cert.subject().unwrap(), "CN=Test Cert, O=Test");
@@ -975,7 +978,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
 
         let mut iter = list.iter();
         let cert = iter.next().unwrap();
-        
+
         let pk = cert.public_key();
 
         assert_eq!(pk.pk_type(), crate::pk::Type::Rsa);
@@ -1011,7 +1014,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         let c_int1 = Certificate::from_pem(C_INT1.as_bytes()).unwrap();
         let c_int2 = Certificate::from_pem(C_INT2.as_bytes()).unwrap();
         let mut c_root = Certificate::from_pem_multiple(C_ROOT.as_bytes()).unwrap();
-        
+
         // Certificate C_INT2 is missing at the beginning so the verification should fail at first
         let mut chain = MbedtlsList::<Certificate>::new();
         chain.push(c_leaf.clone());
@@ -1041,7 +1044,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             Err(e) => panic!("Failed to verify, error: {}, err_str: {}", e, err_str),
         };
     }
-    
+
     #[test]
     fn clone_test() {
         let cert_chain = Certificate::from_pem(TEST_CERT_PEM.as_bytes()).unwrap();
@@ -1050,7 +1053,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         chain.push(cert_chain.clone());
         chain.push(cert_chain.clone());
         chain.push(cert_chain);
-        
+
         let clone = chain.clone();
         let mut it = clone.iter();
 
@@ -1085,7 +1088,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         let c1_info = format!("{:?}", c1.iter().next().unwrap());
         let c2_info = format!("{:?}", c2.iter().next().unwrap());
         let c3_info = format!("{:?}", c3.iter().next().unwrap());
-        
+
         chain.append(c1.clone());
         chain.append(c2.clone());
         chain.append(c3.clone());
@@ -1105,7 +1108,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             assert_eq!(c2_info, format!("{:?}", it.next().unwrap()));
             assert!(it.next().is_none());
         }
-        
+
         chain.append(c3.clone());
         chain.append(c1.clone());
         {
@@ -1133,7 +1136,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             let mut it = chain.iter();
             assert!(it.next().is_none());
         }
-        
+
         assert!(chain.pop_back().is_none());
         {
             let mut it = chain.iter();
@@ -1167,7 +1170,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         let c1_info = format!("{:?}", c1);
         let c2_info = format!("{:?}", c2);
         let c3_info = format!("{:?}", c3);
-        
+
         chain.push(c1.clone());
         chain.push(c2.clone());
         chain.push(c3.clone());
@@ -1187,7 +1190,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             assert_eq!(c3_info, format!("{:?}", it.next().unwrap()));
             assert!(it.next().is_none());
         }
-        
+
         chain.push(c3.clone());
         chain.push(c1.clone());
         {
@@ -1215,7 +1218,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             let mut it = chain.iter();
             assert!(it.next().is_none());
         }
-        
+
         assert!(chain.pop_front().is_none());
         {
             let mut it = chain.iter();
@@ -1302,7 +1305,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         }
 
         let clone = chain.clone();
-        
+
         {
             let mut it = chain.into_iter();
             assert_eq!(c1_info, format!("{:?}", it.next().unwrap()));
@@ -1331,7 +1334,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
             <&mut Option<MbedtlsBox<_>> as UnsafeFrom<_>>::from(ptr_test).unwrap()
         };
         assert!(option.is_none());
-        
+
         let cert_list : Option<&mut MbedtlsList<Certificate>> = unsafe { UnsafeFrom::from(ptr_test) };
         assert!(cert_list.is_none());
 
@@ -1341,7 +1344,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         assert!(cert_list.is_none());
 
 
-        
+
     }
 
     #[test]
@@ -1358,7 +1361,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
 
         // Using debug strings so failing unit tests can identify any issue with contents.
         let c1_info = format!("{:?}", c1);
-        
+
         chain.push(c1.clone());
         chain.push(c2.clone());
         chain.push(c3.clone());
@@ -1375,7 +1378,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         assert_eq!((*cert).inner.next, ::core::ptr::null_mut());
         assert_eq!(c1_info, format!("{:?}", cert));
 
-        
+
         let cert = chain.clone().into_iter().next().unwrap();
 
         // Using into_iter and extracting a certificate must have its next set to null
@@ -1406,7 +1409,7 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
 
         let cert : &Certificate = unsafe { UnsafeFrom::from(ptr).unwrap() };
         assert_eq!(c1_info, format!("{:?}", cert));
-        
+
         let ptr : *mut x509_crt = (&mut chain).into();
         assert_ne!(ptr, ::core::ptr::null_mut());
 
@@ -1424,6 +1427,34 @@ cYp0bH/RcPTC0Z+ZaqSWMtfxRrk63MJQF9EXpDCdvQRcTMD9D85DJrMKn8aumq0M
         assert!(crate::tests::TestTrait::<dyn Sync, MbedtlsList<Certificate>>::new().impls_trait(), "MbedtlsList<Certificate> should be Sync");
     }
 
+    #[test]
+    fn empty_cert_chain_test() {
+        const C_CERT: &'static str = concat!(include_str!("../../tests/data/certificate.crt"), "\0");
+        const C_ROOT: &'static str = concat!(include_str!("../../tests/data/root.crt"), "\0");
+
+        let mut certs = MbedtlsList::new();
+        certs.push(Certificate::from_pem(&C_CERT.as_bytes()).unwrap());
+        let mut roots = MbedtlsList::new();
+        roots.push(Certificate::from_pem(&C_ROOT.as_bytes()).unwrap());
+
+        assert!(Certificate::verify(&certs, &roots, None, None).is_ok());
+
+        let empty_certs = MbedtlsList::new();
+
+        assert_eq!(
+            Certificate::verify(&certs, &empty_certs, None, None).unwrap_err(),
+            codes::X509CertVerifyFailed.into()
+        );
+        assert_eq!(
+            Certificate::verify(&empty_certs, &roots, None, None).unwrap_err(),
+            codes::X509BadInputData.into()
+        );
+        assert_eq!(
+            Certificate::verify(&empty_certs, &empty_certs, None, None).unwrap_err(),
+            codes::X509BadInputData.into()
+        );
+    }
+
     #[test]
     fn empty_crl_test() {
         const C_CERT: &'static str = concat!(include_str!("../../tests/data/certificate.crt"), "\0");