Introduce a mutex around the FFI calls to OpenSSL (#516)

This mutex is currently behind a new feature flag `openssl_ffi_mutex`, which is not enabled by default.

OpenSSL code is not re-entrant; if tests run fast enough, that will cause unexpected behavior.

Co-authored-by: Gavin  Peacock <[email protected]>

Author: scouten-adobe

README.md

@@ -92,7 +92,7 @@ The Rust library crate provides:
 * `fetch_remote_manifests` enables the verification step to retrieve externally referenced manifest stores.  External manifests are only fetched if there is no embedded manifest store and no locally adjacent .c2pa manifest store file of the same name.
 * `json_schema` is used by `make schema` to produce a JSON schema document that represents the `ManifestStore` data structures.
 * `psxxx_ocsp_stapling_experimental` this is an demonstration feature that will attempt to fetch the OCSP data from the OCSP responders listed in the manifest signing certificate.  The response becomes part of the manifest and is used to prove the certificate was not revoked at the time of signing.  This is only implemented for PS256, PS384 and PS512 signatures and is intended as a demonstration.
-
+* `openssl_ffi_mutex` prevents multiple threads from accessing the C OpenSSL library simultaneously. (This library is not re-entrant.) In a multi-threaded process (such as Cargo's test runner), this can lead to unpredictable behavior.
 
 ## Example code
 

sdk/Cargo.toml

@@ -37,6 +37,7 @@ json_schema = ["dep:schemars"]
 pdf = ["dep:lopdf"]
 v1_api = []
 unstable_api = []
+openssl_ffi_mutex = []
 
 # The diagnostics feature is unsupported and might be removed.
 # It enables some low-overhead timing features used in our development cycle.

sdk/src/error.rs

@@ -281,6 +281,9 @@ pub enum Error {
     #[error(transparent)]
     CborError(#[from] serde_cbor::Error),
 
+    #[error("could not acquire OpenSSL FFI mutex")]
+    OpenSslMutexError,
+
     #[error(transparent)]
     #[cfg(feature = "openssl")]
     OpenSslError(#[from] openssl::error::ErrorStack),

sdk/src/openssl/ec_signer.rs

@@ -49,6 +49,8 @@ impl ConfigurableSigner for EcSigner {
         alg: SigningAlg,
         tsa_url: Option<String>,
     ) -> Result<Self> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let certs_size = signcert.len();
         let pkey = EcKey::private_key_from_pem(pkey).map_err(Error::OpenSslError)?;
         let signcerts = X509::stack_from_pem(signcert).map_err(Error::OpenSslError)?;
@@ -73,6 +75,8 @@ impl ConfigurableSigner for EcSigner {
 
 impl Signer for EcSigner {
     fn sign(&self, data: &[u8]) -> Result<Vec<u8>> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let key = PKey::from_ec_key(self.pkey.clone()).map_err(Error::OpenSslError)?;
 
         let mut signer = match self.alg {
@@ -93,6 +97,8 @@ impl Signer for EcSigner {
     }
 
     fn certs(&self) -> Result<Vec<Vec<u8>>> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let mut certs: Vec<Vec<u8>> = Vec::new();
 
         for c in &self.signcerts {
@@ -120,6 +126,10 @@ struct ECSigComps<'a> {
 }
 
 fn parse_ec_sig(data: &[u8]) -> der_parser::error::BerResult<ECSigComps> {
+    // IMPORTANT: OpenSslMutex::acquire() should have been called by calling fn.
+    // Please don't make this pub or pub(crate) without finding a way to ensure
+    // that precondition.
+
     parse_der_sequence_defined_g(|content: &[u8], _| {
         let (rem1, r) = parse_der_integer(content)?;
         let (_rem2, s) = parse_der_integer(rem1)?;
@@ -135,6 +145,10 @@ fn parse_ec_sig(data: &[u8]) -> der_parser::error::BerResult<ECSigComps> {
 }
 
 fn der_to_p1363(data: &[u8], alg: SigningAlg) -> Result<Vec<u8>> {
+    // IMPORTANT: OpenSslMutex::acquire() should have been called by calling fn.
+    // Please don't make this pub or pub(crate) without finding a way to ensure
+    // that precondition.
+
     // P1363 format: r | s
 
     let (_, p) = parse_ec_sig(data).map_err(|_err| Error::InvalidEcdsaSignature)?;

sdk/src/openssl/ec_validator.rs

@@ -27,6 +27,8 @@ impl EcValidator {
 
 impl CoseValidator for EcValidator {
     fn validate(&self, sig: &[u8], data: &[u8], pkey: &[u8]) -> Result<bool> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let public_key = EcKey::public_key_from_der(pkey).map_err(|_err| Error::CoseSignature)?;
         let key = PKey::from_ec_key(public_key).map_err(wrap_openssl_err)?;
 

sdk/src/openssl/ed_signer.rs

@@ -39,6 +39,8 @@ impl ConfigurableSigner for EdSigner {
         alg: SigningAlg,
         tsa_url: Option<String>,
     ) -> Result<Self> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let certs_size = signcert.len();
         let signcerts = X509::stack_from_pem(signcert).map_err(Error::OpenSslError)?;
         let pkey = PKey::private_key_from_pem(pkey).map_err(Error::OpenSslError)?;
@@ -67,6 +69,8 @@ impl ConfigurableSigner for EdSigner {
 
 impl Signer for EdSigner {
     fn sign(&self, data: &[u8]) -> Result<Vec<u8>> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let mut signer =
             openssl::sign::Signer::new_without_digest(&self.pkey).map_err(Error::OpenSslError)?;
 
@@ -80,6 +84,8 @@ impl Signer for EdSigner {
     }
 
     fn certs(&self) -> Result<Vec<Vec<u8>>> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let mut certs: Vec<Vec<u8>> = Vec::new();
 
         for c in &self.signcerts {

sdk/src/openssl/ed_validator.rs

@@ -27,6 +27,8 @@ impl EdValidator {
 
 impl CoseValidator for EdValidator {
     fn validate(&self, sig: &[u8], data: &[u8], pkey: &[u8]) -> Result<bool> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let public_key = PKey::public_key_from_der(pkey).map_err(|_err| Error::CoseSignature)?;
 
         let mut verifier = openssl::sign::Verifier::new_without_digest(&public_key)

sdk/src/openssl/ffi_mutex.rs

@@ -0,0 +1,62 @@
+// Copyright 2024 Adobe. All rights reserved.
+// This file is licensed to you under the Apache License,
+// Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
+// or the MIT license (http://opensource.org/licenses/MIT),
+// at your option.
+
+// Unless required by applicable law or agreed to in writing,
+// this software is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR REPRESENTATIONS OF ANY KIND, either express or
+// implied. See the LICENSE-MIT and LICENSE-APACHE files for the
+// specific language governing permissions and limitations under
+// each license.
+
+// OpenSSL code is not re-entrant. Use this to guard against race conditions.
+use crate::Result;
+
+#[cfg(feature = "openssl_ffi_mutex")]
+static FFI_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
+pub(crate) struct OpenSslMutex<'a> {
+    // Dead code here is intentional. We don't need to read the () contents
+    // of this guard. We only need to ensure that the guard is dropped when
+    // this struct is dropped.
+    #[allow(dead_code)]
+    #[cfg(feature = "openssl_ffi_mutex")]
+    guard: std::sync::MutexGuard<'a, ()>,
+
+    #[allow(dead_code)]
+    #[cfg(not(feature = "openssl_ffi_mutex"))]
+    guard: &'a str,
+}
+
+impl<'a> OpenSslMutex<'a> {
+    /// Acquire a mutex on OpenSSL FFI code.
+    ///
+    /// WARNING: Calling code MUST NOT PANIC inside this function or
+    /// anything called by it, even in test code. This will poison the FFI mutex
+    /// and leave OpenSSL unusable for the remainder of the process lifetime.
+    pub(crate) fn acquire() -> Result<Self> {
+        // Useful for debugging.
+        // eprintln!(
+        //     "ACQUIRING FFI MUTEX at\n{}",
+        //     std::backtrace::Backtrace::force_capture()
+        // );
+
+        #[cfg(feature = "openssl_ffi_mutex")]
+        match FFI_MUTEX.lock() {
+            Ok(guard) => Ok(Self { guard }),
+            Err(_) => Err(crate::Error::OpenSslMutexError),
+        }
+
+        #[cfg(not(feature = "openssl_ffi_mutex"))]
+        Ok(Self { guard: &"foo" })
+    }
+}
+
+// Useful for debugging.
+// impl<'a> Drop for OpenSslMutex<'a> {
+//     fn drop(&mut self) {
+//         eprintln!("Releasing FFI mutex\n\n\n");
+//     }
+// }

sdk/src/openssl/mod.rs

@@ -51,6 +51,9 @@ pub(crate) use openssl_trust_handler::verify_trust;
 #[cfg(feature = "openssl")]
 pub(crate) use openssl_trust_handler::OpenSSLTrustHandlerConfig;
 
+mod ffi_mutex;
+pub(crate) use ffi_mutex::OpenSslMutex;
+
 #[cfg(test)]
 pub(crate) mod temp_signer_async;
 
@@ -60,8 +63,13 @@ use openssl::x509::X509;
 #[allow(unused_imports)]
 #[cfg(feature = "openssl")]
 pub(crate) use temp_signer_async::AsyncSignerAdapter;
+
 #[cfg(feature = "openssl")]
-pub(crate) fn check_chain_order(certs: &[X509]) -> bool {
+fn check_chain_order(certs: &[X509]) -> bool {
+    // IMPORTANT: ffi_mutex::acquire() should have been called by calling fn. Please
+    // don't make this pub or pub(crate) without finding a way to ensure that
+    // precondition.
+
     {
         if certs.len() > 1 {
             for (i, c) in certs.iter().enumerate() {
@@ -85,13 +93,17 @@ pub(crate) fn check_chain_order(certs: &[X509]) -> bool {
 }
 
 #[cfg(not(feature = "openssl"))]
-pub(crate) fn check_chain_order(certs: &[X509]) -> bool {
+fn check_chain_order(certs: &[X509]) -> bool {
     true
 }
 
 #[cfg(feature = "openssl")]
 #[allow(dead_code)]
-pub(crate) fn check_chain_order_der(cert_ders: &[Vec<u8>]) -> bool {
+fn check_chain_order_der(cert_ders: &[Vec<u8>]) -> bool {
+    // IMPORTANT: ffi_mutex::acquire() should have been called by calling fn. Please
+    // don't make this pub or pub(crate) without finding a way to ensure that
+    // precondition.
+
     let mut certs: Vec<X509> = Vec::new();
     for cert_der in cert_ders {
         if let Ok(cert) = X509::from_der(cert_der) {
@@ -105,6 +117,6 @@ pub(crate) fn check_chain_order_der(cert_ders: &[Vec<u8>]) -> bool {
 }
 
 #[cfg(not(feature = "openssl"))]
-pub(crate) fn check_chain_order_der(cert_ders: &[Vec<u8>]) -> bool {
+fn check_chain_order_der(cert_ders: &[Vec<u8>]) -> bool {
     true
 }

sdk/src/openssl/openssl_trust_handler.rs

@@ -27,6 +27,10 @@ use crate::{
 };
 
 fn certs_der_to_x509(ders: &[Vec<u8>]) -> Result<Vec<openssl::x509::X509>> {
+    // IMPORTANT: ffi_mutex::acquire() should have been called by calling fn. Please
+    // don't make this pub or pub(crate) without finding a way to ensure that
+    // precondition.
+
     let mut certs: Vec<openssl::x509::X509> = Vec::new();
 
     for d in ders {
@@ -38,6 +42,7 @@ fn certs_der_to_x509(ders: &[Vec<u8>]) -> Result<Vec<openssl::x509::X509>> {
 }
 
 fn load_trust_from_pem_data(trust_data: &[u8]) -> Result<Vec<openssl::x509::X509>> {
+    let _openssl = super::OpenSslMutex::acquire()?;
     openssl::x509::X509::stack_from_pem(trust_data).map_err(Error::OpenSslError)
 }
 
@@ -69,6 +74,8 @@ impl OpenSSLTrustHandlerConfig {
     }
 
     fn update_store(&mut self) -> Result<()> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let mut builder =
             openssl::x509::store::X509StoreBuilder::new().map_err(Error::OpenSslError)?;
 
@@ -134,13 +141,16 @@ impl TrustHandlerConfig for OpenSSLTrustHandlerConfig {
         let mut buffer = Vec::new();
         allowed_list.read_to_end(&mut buffer)?;
 
-        if let Ok(cert_list) = openssl::x509::X509::stack_from_pem(&buffer) {
-            for cert in &cert_list {
-                let cert_der = cert.to_der().map_err(Error::OpenSslError)?;
-                let cert_sha256 = hash_sha256(&cert_der);
-                let cert_hash_base64 = base64::encode(&cert_sha256);
+        {
+            let _openssl = super::OpenSslMutex::acquire()?;
+            if let Ok(cert_list) = openssl::x509::X509::stack_from_pem(&buffer) {
+                for cert in &cert_list {
+                    let cert_der = cert.to_der().map_err(Error::OpenSslError)?;
+                    let cert_sha256 = hash_sha256(&cert_der);
+                    let cert_hash_base64 = base64::encode(&cert_sha256);
 
-                self.allowed_cert_set.insert(cert_hash_base64);
+                    self.allowed_cert_set.insert(cert_hash_base64);
+                }
             }
         }
 
@@ -237,6 +247,8 @@ pub(crate) fn verify_trust(
         return Ok(true);
     }
 
+    let _openssl = super::OpenSslMutex::acquire()?;
+
     let mut cert_chain = openssl::stack::Stack::new().map_err(Error::OpenSslError)?;
     let mut store_ctx = openssl::x509::X509StoreContext::new().map_err(Error::OpenSslError)?;
 
@@ -266,6 +278,7 @@ pub(crate) fn verify_trust(
         Err(_) => Ok(false),
     }
 }
+
 #[cfg(test)]
 pub mod tests {
     #![allow(clippy::expect_used)]

sdk/src/openssl/rsa_signer.rs

@@ -44,6 +44,10 @@ impl RsaSigner {
     // production use since there is no caching in the SDK and fetching is expensive. This is behind the
     // feature flag 'psxxx_ocsp_stapling_experimental'
     fn update_ocsp(&self) {
+        // IMPORTANT: ffi_mutex::acquire() should have been called by calling fn. Please
+        // don't make this pub or pub(crate) without finding a way to ensure that
+        // precondition.
+
         // do we need an update
         let now = chrono::offset::Utc::now();
 
@@ -54,7 +58,7 @@ impl RsaSigner {
         if now > next_update {
             #[cfg(feature = "psxxx_ocsp_stapling_experimental")]
             {
-                if let Ok(certs) = self.certs() {
+                if let Ok(certs) = self.certs_internal() {
                     if let Some(ocsp_rsp) = crate::ocsp_utils::fetch_ocsp_response(&certs) {
                         self.ocsp_size.set(ocsp_rsp.len());
                         let mut validation_log =
@@ -71,6 +75,21 @@ impl RsaSigner {
             }
         }
     }
+
+    fn certs_internal(&self) -> Result<Vec<Vec<u8>>> {
+        // IMPORTANT: ffi_mutex::acquire() should have been called by calling fn. Please
+        // don't make this pub or pub(crate) without finding a way to ensure that
+        // precondition.
+
+        let mut certs: Vec<Vec<u8>> = Vec::new();
+
+        for c in &self.signcerts {
+            let cert = c.to_der().map_err(wrap_openssl_err)?;
+            certs.push(cert);
+        }
+
+        Ok(certs)
+    }
 }
 
 impl ConfigurableSigner for RsaSigner {
@@ -80,6 +99,8 @@ impl ConfigurableSigner for RsaSigner {
         alg: SigningAlg,
         tsa_url: Option<String>,
     ) -> Result<Self> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let signcerts = X509::stack_from_pem(signcert).map_err(wrap_openssl_err)?;
         let rsa = Rsa::private_key_from_pem(pkey).map_err(wrap_openssl_err)?;
 
@@ -192,14 +213,8 @@ impl Signer for RsaSigner {
     }
 
     fn certs(&self) -> Result<Vec<Vec<u8>>> {
-        let mut certs: Vec<Vec<u8>> = Vec::new();
-
-        for c in &self.signcerts {
-            let cert = c.to_der().map_err(wrap_openssl_err)?;
-            certs.push(cert);
-        }
-
-        Ok(certs)
+        let _openssl = super::OpenSslMutex::acquire()?;
+        self.certs_internal()
     }
 
     fn alg(&self) -> SigningAlg {
@@ -211,6 +226,8 @@ impl Signer for RsaSigner {
     }
 
     fn ocsp_val(&self) -> Option<Vec<u8>> {
+        let _openssl = super::OpenSslMutex::acquire().ok()?;
+
         // update OCSP if needed
         self.update_ocsp();
 

sdk/src/openssl/rsa_validator.rs

@@ -27,6 +27,8 @@ impl RsaValidator {
 
 impl CoseValidator for RsaValidator {
     fn validate(&self, sig: &[u8], data: &[u8], pkey: &[u8]) -> Result<bool> {
+        let _openssl = super::OpenSslMutex::acquire()?;
+
         let rsa = Rsa::public_key_from_der(pkey)?;
 
         // rebuild RSA keys to eliminate incompatible values