Harden OpenSSL crypto error checking and resource management (#7817)

Copilot · maxtropets · web-flow · commit 55e5977f37f3 · 2026-04-16T13:59:31.000+01:00
Co-authored-by: copilot-swe-agent[bot] &lt;198982749+Copilot@users.noreply.github.com&gt;
Co-authored-by: maxtropets &lt;16566519+maxtropets@users.noreply.github.com&gt;
Co-authored-by: Max &lt;maxtropets@microsoft.com&gt;
Co-authored-by: Max Tropets &lt;maxtropets@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Removed
 
+- Removed `CHECK0()` from `ccf::crypto::OpenSSL` in the public header `openssl_wrappers.h` (#7817).
 - Removed `aes_gcm_encrypt()`, `aes_gcm_decrypt()`, and `default_iv` from `ccf::crypto` (#7811).
 - Removed `get_responder()` from the public `ccf::RpcContext` API and made `http_responder.h` a private header (#7818).
 - Removed the `/node/memory` endpoint. This endpoint was originally useful for monitoring SGX enclave memory usage, which is no longer relevant now that SGX support has been removed.
diff --git a/include/ccf/crypto/openssl/openssl_wrappers.h b/include/ccf/crypto/openssl/openssl_wrappers.h
@@ -50,25 +50,14 @@ namespace ccf::crypto::OpenSSL
     return "unknown error";
   }
 
-  /// Throws if rc is not 1 and has error
+  /// Throws if rc is not 1
   inline void CHECK1(int rc)
   {
-    unsigned long ec = ERR_get_error();
-    if (rc != 1 && ec != 0)
+    if (rc != 1)
     {
-      throw std::runtime_error(
-        fmt::format("OpenSSL error: {}", error_string(ec)));
-    }
-  }
-
-  /// Throws if rc is 0 and has error
-  inline void CHECK0(int rc)
-  {
-    unsigned long ec = ERR_get_error();
-    if (rc == 0 && ec != 0)
-    {
-      throw std::runtime_error(
-        fmt::format("OpenSSL error: {}", error_string(ec)));
+      unsigned long ec = ERR_get_error();
+      throw std::runtime_error(fmt::format(
+        "OpenSSL error (rc={}, ec={}): {}", rc, ec, error_string(ec)));
     }
   }
 
@@ -87,8 +76,8 @@ namespace ccf::crypto::OpenSSL
     if (expect != actual)
     {
       unsigned long ec = ERR_get_error();
-      throw std::runtime_error(
-        fmt::format("OpenSSL error: {}", error_string(ec)));
+      throw std::runtime_error(fmt::format(
+        "OpenSSL error (rc={}, ec={}): {}", actual, ec, error_string(ec)));
     }
   }
 
@@ -97,7 +86,9 @@ namespace ccf::crypto::OpenSSL
   {
     if (val <= 0)
     {
-      throw std::runtime_error("OpenSSL error: expected positive value");
+      unsigned long ec = ERR_get_error();
+      throw std::runtime_error(fmt::format(
+        "OpenSSL error (rc={}, ec={}): {}", val, ec, error_string(ec)));
     }
   }
 
diff --git a/src/crypto/ecdsa.cpp b/src/crypto/ecdsa.cpp
@@ -37,10 +37,10 @@ namespace ccf::crypto
     (void)r_bn.release();
     (void)s_bn.release();
     auto der_size = i2d_ECDSA_SIG(sig, nullptr);
-    OpenSSL::CHECK0(der_size);
+    OpenSSL::CHECKPOSITIVE(der_size);
     std::vector<uint8_t> der_sig(der_size);
     auto* der_sig_buf = der_sig.data();
-    OpenSSL::CHECK0(i2d_ECDSA_SIG(sig, &der_sig_buf));
+    OpenSSL::CHECKPOSITIVE(i2d_ECDSA_SIG(sig, &der_sig_buf));
     return der_sig;
   }
 
diff --git a/src/crypto/openssl/ec_key_pair.cpp b/src/crypto/openssl/ec_key_pair.cpp
@@ -260,7 +260,7 @@ namespace ccf::crypto
 
     if (key != nullptr)
     {
-      OpenSSL::CHECK1(X509_REQ_sign(req, key, EVP_sha512()));
+      OpenSSL::CHECKPOSITIVE(X509_REQ_sign(req, key, EVP_sha512()));
     }
 
     return req;
diff --git a/src/crypto/openssl/hash.cpp b/src/crypto/openssl/hash.cpp
@@ -104,32 +104,30 @@ namespace ccf::crypto
       const std::span<const uint8_t>& info)
     {
       const auto* md = get_md_type(md_type);
-      EVP_PKEY_CTX* pctx = nullptr;
       std::vector<uint8_t> r(length);
-      pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr);
+      Unique_EVP_PKEY_CTX pctx(EVP_PKEY_HKDF);
       CHECK1(EVP_PKEY_derive_init(pctx));
-      CHECK1(EVP_PKEY_CTX_set_hkdf_md(pctx, md));
+      CHECKPOSITIVE(EVP_PKEY_CTX_set_hkdf_md(pctx, md));
       if (salt.size() > std::numeric_limits<int>::max())
       {
         throw std::logic_error("Salt size is too large");
       }
       int salt_size = static_cast<int>(salt.size());
-      CHECK1(EVP_PKEY_CTX_set1_hkdf_salt(pctx, salt.data(), salt_size));
+      CHECKPOSITIVE(EVP_PKEY_CTX_set1_hkdf_salt(pctx, salt.data(), salt_size));
       if (ikm.size() > std::numeric_limits<int>::max())
       {
         throw std::logic_error("IKM size is too large");
       }
       int ikm_size = static_cast<int>(ikm.size());
-      CHECK1(EVP_PKEY_CTX_set1_hkdf_key(pctx, ikm.data(), ikm_size));
+      CHECKPOSITIVE(EVP_PKEY_CTX_set1_hkdf_key(pctx, ikm.data(), ikm_size));
       if (info.size() > std::numeric_limits<int>::max())
       {
         throw std::logic_error("Info size is too large");
       }
       int info_size = static_cast<int>(info.size());
-      CHECK1(EVP_PKEY_CTX_add1_hkdf_info(pctx, info.data(), info_size));
+      CHECKPOSITIVE(EVP_PKEY_CTX_add1_hkdf_info(pctx, info.data(), info_size));
       size_t outlen = length;
       CHECK1(EVP_PKEY_derive(pctx, r.data(), &outlen));
-      EVP_PKEY_CTX_free(pctx);
       r.resize(outlen);
       return r;
     }
diff --git a/src/crypto/openssl/rsa_key_pair.cpp b/src/crypto/openssl/rsa_key_pair.cpp
@@ -6,6 +6,7 @@
 #include "ccf/crypto/openssl/openssl_wrappers.h"
 #include "crypto/openssl/hash.h"
 
+#include <climits>
 #include <openssl/core_names.h>
 
 namespace ccf::crypto
@@ -134,9 +135,9 @@ namespace ccf::crypto
 
     Unique_EVP_PKEY_CTX ctx(key);
     CHECK1(EVP_PKEY_decrypt_init(ctx));
-    EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING);
-    EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256());
-    EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256());
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()));
 
     if (label_ != nullptr)
     {
@@ -222,9 +223,14 @@ namespace ccf::crypto
     auto hash = OpenSSLHashProvider().hash(d.data(), d.size(), md_type);
     Unique_EVP_PKEY_CTX pctx(key);
     CHECK1(EVP_PKEY_sign_init(pctx));
-    CHECK1(EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING));
-    CHECK1(EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, salt_length));
-    CHECK1(EVP_PKEY_CTX_set_signature_md(pctx, get_md_type(md_type)));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING));
+    if (salt_length > INT_MAX)
+    {
+      throw std::invalid_argument(fmt::format(
+        "salt_length {} exceeds maximum ({})", salt_length, INT_MAX));
+    }
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, salt_length));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_signature_md(pctx, get_md_type(md_type)));
     size_t olen = r.size();
     CHECK1(EVP_PKEY_sign(pctx, r.data(), &olen, hash.data(), hash.size()));
     r.resize(olen);
diff --git a/src/crypto/openssl/rsa_public_key.cpp b/src/crypto/openssl/rsa_public_key.cpp
@@ -5,6 +5,7 @@
 #include "crypto/openssl/hash.h"
 #include "crypto/openssl/rsa_key_pair.h"
 
+#include <climits>
 #include <openssl/core_names.h>
 #include <openssl/encoder.h>
 
@@ -123,9 +124,9 @@ namespace ccf::crypto
   {
     Unique_EVP_PKEY_CTX ctx(key);
     OpenSSL::CHECK1(EVP_PKEY_encrypt_init(ctx));
-    EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING);
-    EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256());
-    EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256());
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()));
 
     if (label != nullptr && label_size > 0)
     {
@@ -228,12 +229,17 @@ namespace ccf::crypto
 
     Unique_EVP_PKEY_CTX pctx(key);
     CHECK1(EVP_PKEY_verify_init(pctx));
-    CHECK1(EVP_PKEY_CTX_set_rsa_padding(pctx, ossl_padding->second));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_padding(pctx, ossl_padding->second));
     if (ossl_padding->first == RSAPadding::PKCS_PSS)
     {
-      CHECK1(EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, salt_length));
+      if (salt_length > INT_MAX)
+      {
+        throw std::invalid_argument(fmt::format(
+          "salt_length {} exceeds maximum ({})", salt_length, INT_MAX));
+      }
+      CHECKPOSITIVE(EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, salt_length));
     }
-    CHECK1(EVP_PKEY_CTX_set_signature_md(pctx, get_md_type(md_type)));
+    CHECKPOSITIVE(EVP_PKEY_CTX_set_signature_md(pctx, get_md_type(md_type)));
     return EVP_PKEY_verify(pctx, signature, signature_size, hash, hash_size) ==
       1;
   }
diff --git a/src/crypto/openssl/verifier.cpp b/src/crypto/openssl/verifier.cpp
@@ -138,7 +138,7 @@ namespace ccf::crypto
         return false;
       }
 
-      CHECK1(sk_X509_push(chain_stack, cert));
+      CHECKPOSITIVE(sk_X509_push(chain_stack, cert));
       CHECK1(X509_up_ref(cert));
     }
 
diff --git a/src/tls/cert.h b/src/tls/cert.h
@@ -64,7 +64,7 @@ namespace tls
             Unique_BIO certbio(*it);
             Unique_X509 cert(certbio, true);
 
-            CHECK1(sk_X509_push(chain, cert));
+            CHECKPOSITIVE(sk_X509_push(chain, cert));
             CHECK1(X509_up_ref(cert));
           }
         }

Original file line number	Diff line number	Diff line change
`@@ -50,25 +50,14 @@ namespace ccf::crypto::OpenSSL`
`50`	`50`	`return "unknown error";`
`51`	`51`	`}`
`52`	`52`
`53`		`- /// Throws if rc is not 1 and has error`
	`53`	`+ /// Throws if rc is not 1`
`54`	`54`	`inline void CHECK1(int rc)`
`55`	`55`	`{`
`56`		`- unsigned long ec = ERR_get_error();`
`57`		`- if (rc != 1 && ec != 0)`
	`56`	`+ if (rc != 1)`
`58`	`57`	`{`
`59`		`- throw std::runtime_error(`
`60`		`- fmt::format("OpenSSL error: {}", error_string(ec)));`
`61`		`- }`
`62`		`- }`
`63`		`-`
`64`		`- /// Throws if rc is 0 and has error`
`65`		`- inline void CHECK0(int rc)`
`66`		`- {`
`67`		`- unsigned long ec = ERR_get_error();`
`68`		`- if (rc == 0 && ec != 0)`
`69`		`- {`
`70`		`- throw std::runtime_error(`
`71`		`- fmt::format("OpenSSL error: {}", error_string(ec)));`
	`58`	`+ unsigned long ec = ERR_get_error();`
	`59`	`+ throw std::runtime_error(fmt::format(`
	`60`	`+ "OpenSSL error (rc={}, ec={}): {}", rc, ec, error_string(ec)));`
`72`	`61`	`}`
`73`	`62`	`}`
`74`	`63`
`@@ -87,8 +76,8 @@ namespace ccf::crypto::OpenSSL`
`87`	`76`	`if (expect != actual)`
`88`	`77`	`{`
`89`	`78`	`unsigned long ec = ERR_get_error();`
`90`		`- throw std::runtime_error(`
`91`		`- fmt::format("OpenSSL error: {}", error_string(ec)));`
	`79`	`+ throw std::runtime_error(fmt::format(`
	`80`	`+ "OpenSSL error (rc={}, ec={}): {}", actual, ec, error_string(ec)));`
`92`	`81`	`}`
`93`	`82`	`}`
`94`	`83`
`@@ -97,7 +86,9 @@ namespace ccf::crypto::OpenSSL`
`97`	`86`	`{`
`98`	`87`	`if (val <= 0)`
`99`	`88`	`{`
`100`		`- throw std::runtime_error("OpenSSL error: expected positive value");`
	`89`	`+ unsigned long ec = ERR_get_error();`
	`90`	`+ throw std::runtime_error(fmt::format(`
	`91`	`+ "OpenSSL error (rc={}, ec={}): {}", val, ec, error_string(ec)));`
`101`	`92`	`}`
`102`	`93`	`}`
`103`	`94`
Original file line number	Diff line number	Diff line change
`@@ -260,7 +260,7 @@ namespace ccf::crypto`
`260`	`260`
`261`	`261`	`if (key != nullptr)`
`262`	`262`	`{`
`263`		`- OpenSSL::CHECK1(X509_REQ_sign(req, key, EVP_sha512()));`
	`263`	`+ OpenSSL::CHECKPOSITIVE(X509_REQ_sign(req, key, EVP_sha512()));`
`264`	`264`	`}`
`265`	`265`
`266`	`266`	`return req;`
Original file line number	Diff line number	Diff line change
`@@ -104,32 +104,30 @@ namespace ccf::crypto`
`104`	`104`	`const std::span<const uint8_t>& info)`
`105`	`105`	`{`
`106`	`106`	`const auto* md = get_md_type(md_type);`
`107`		`- EVP_PKEY_CTX* pctx = nullptr;`
`108`	`107`	`std::vector<uint8_t> r(length);`
`109`		`- pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr);`
	`108`	`+ Unique_EVP_PKEY_CTX pctx(EVP_PKEY_HKDF);`
`110`	`109`	`CHECK1(EVP_PKEY_derive_init(pctx));`
`111`		`- CHECK1(EVP_PKEY_CTX_set_hkdf_md(pctx, md));`
	`110`	`+ CHECKPOSITIVE(EVP_PKEY_CTX_set_hkdf_md(pctx, md));`
`112`	`111`	`if (salt.size() > std::numeric_limits<int>::max())`
`113`	`112`	`{`
`114`	`113`	`throw std::logic_error("Salt size is too large");`
`115`	`114`	`}`
`116`	`115`	`int salt_size = static_cast<int>(salt.size());`
`117`		`- CHECK1(EVP_PKEY_CTX_set1_hkdf_salt(pctx, salt.data(), salt_size));`
	`116`	`+ CHECKPOSITIVE(EVP_PKEY_CTX_set1_hkdf_salt(pctx, salt.data(), salt_size));`
`118`	`117`	`if (ikm.size() > std::numeric_limits<int>::max())`
`119`	`118`	`{`
`120`	`119`	`throw std::logic_error("IKM size is too large");`
`121`	`120`	`}`
`122`	`121`	`int ikm_size = static_cast<int>(ikm.size());`
`123`		`- CHECK1(EVP_PKEY_CTX_set1_hkdf_key(pctx, ikm.data(), ikm_size));`
	`122`	`+ CHECKPOSITIVE(EVP_PKEY_CTX_set1_hkdf_key(pctx, ikm.data(), ikm_size));`
`124`	`123`	`if (info.size() > std::numeric_limits<int>::max())`
`125`	`124`	`{`
`126`	`125`	`throw std::logic_error("Info size is too large");`
`127`	`126`	`}`
`128`	`127`	`int info_size = static_cast<int>(info.size());`
`129`		`- CHECK1(EVP_PKEY_CTX_add1_hkdf_info(pctx, info.data(), info_size));`
	`128`	`+ CHECKPOSITIVE(EVP_PKEY_CTX_add1_hkdf_info(pctx, info.data(), info_size));`
`130`	`129`	`size_t outlen = length;`
`131`	`130`	`CHECK1(EVP_PKEY_derive(pctx, r.data(), &outlen));`
`132`		`- EVP_PKEY_CTX_free(pctx);`
`133`	`131`	`r.resize(outlen);`
`134`	`132`	`return r;`
`135`	`133`	`}`
Original file line number	Diff line number	Diff line change
`@@ -138,7 +138,7 @@ namespace ccf::crypto`
`138`	`138`	`return false;`
`139`	`139`	`}`
`140`	`140`
`141`		`- CHECK1(sk_X509_push(chain_stack, cert));`
	`141`	`+ CHECKPOSITIVE(sk_X509_push(chain_stack, cert));`
`142`	`142`	`CHECK1(X509_up_ref(cert));`
`143`	`143`	`}`
`144`	`144`
Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ namespace tls`
`64`	`64`	`Unique_BIO certbio(*it);`
`65`	`65`	`Unique_X509 cert(certbio, true);`
`66`	`66`
`67`		`- CHECK1(sk_X509_push(chain, cert));`
	`67`	`+ CHECKPOSITIVE(sk_X509_push(chain, cert));`
`68`	`68`	`CHECK1(X509_up_ref(cert));`
`69`	`69`	`}`
`70`	`70`	`}`