Ai security fixes

cdoc/CDoc1Reader.cpp

@@ -25,6 +25,9 @@
 #include "Lock.h"
 #include "Utils.h"
 #include "ZStream.h"
+#include "utils/memory.h"
+
+#include <openssl/evp.h>
 
 #include <map>
 #include <span>
@@ -110,14 +113,57 @@ CDoc1Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
     if (lock_idx >= d->locks.size()) return libcdoc::WRONG_ARGUMENTS;
     const Lock &lock = d->locks.at(lock_idx);
     setLastError({});
+
+    // Determine the FMK length from the container's body cipher. The CDoc1
+    // body uses AES-128/192/256 in CBC or GCM mode, so the FMK is 16, 24
+    // or 32 bytes long. We pin this length up-front and pass it to the RSA
+    // decrypt path so that an attacker observing this function cannot
+    // distinguish between
+    //   (a) RSA padding failed
+    //   (b) RSA padding succeeded but the resulting length was wrong
+    //   (c) a wholly different recipient was used to derive a wrong key.
+    //
+    // All three cases must look the same: the function returns OK with a
+    // candidate FMK of the right length, and the eventual AES decrypt at
+    // the container body level either authenticates that FMK (success) or
+    // rejects it. CDoc1 has no header HMAC, so the AES-GCM tag is the
+    // only bit of authentication we can rely on. AES-CBC containers
+    // therefore retain a residual oracle (PKCS#7 stripping); using GCM
+    // when re-encrypting with libcdoc is strongly preferred.
+    size_t expected_fmk_len = 0;
+    if (const EVP_CIPHER *c = libcdoc::Crypto::cipher(d->method); c) {
+        expected_fmk_len = size_t(EVP_CIPHER_key_length(c));
+    }
+    if (expected_fmk_len != 16 && expected_fmk_len != 24 && expected_fmk_len != 32) {
+        // Method-level error - independent of key bits, so does NOT feed
+        // an oracle.
+        setLastError("Failed to derive FMK");
+        LOG_ERROR("Unsupported CDoc1 encryption method: {}", d->method);
+        return libcdoc::CRYPTO_ERROR;
+    }
+
+    // From this point on, every error path returns the SAME error code and
+    // SAME last-error string, so that the only bit of information leaking
+    // back to the caller is "this lock did/did not produce a usable FMK".
+    constexpr auto FAIL_MSG = "Failed to derive FMK";
+
     if (lock.isRSA()) {
+        // If OAEP = false and and fmk.size() != 0, the decryptRSA always
+        // returns OK with synthetic bytes on padding failure; only a
+        // fundamental error (e.g. ct size mismatch with modulus) yields a non-OK result.
+        fmk.resize(expected_fmk_len);
         int result = crypto->decryptRSA(fmk, lock.encrypted_fmk, false, lock_idx);
-		if (result < 0) {
-			setLastError(crypto->getLastErrorStr(result));
+        if (result != libcdoc::OK) {
+            libcdoc::cleanse(fmk);
+            fmk.clear();
+            setLastError(FAIL_MSG);
             LOG_ERROR("{}", last_error);
-			return libcdoc::CRYPTO_ERROR;
-		}
-	} else {
+            return libcdoc::CRYPTO_ERROR;
+        }
+        // Even on "OK" the contents may be synthetic - that is the point.
+        // The downstream AES decrypt at the body level is what tells
+        // success from failure.
+    } else {
         std::vector<uint8_t> key;
         int result = crypto->deriveConcatKDF(key,
             lock.getBytes(Lock::Params::KEY_MATERIAL),
@@ -126,18 +172,31 @@ CDoc1Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
             lock.getBytes(Lock::Params::PARTY_UINFO),
             lock.getBytes(Lock::Params::PARTY_VINFO),
             lock_idx);
-		if (result < 0) {
-			setLastError(crypto->getLastErrorStr(result));
+        if (result < 0) {
+            libcdoc::cleanse(key);
+            setLastError(FAIL_MSG);
             LOG_ERROR("{}", last_error);
-			return libcdoc::CRYPTO_ERROR;
-		}
+            return libcdoc::CRYPTO_ERROR;
+        }
         fmk = libcdoc::Crypto::AESWrap(key, lock.encrypted_fmk, false);
-	}
-	if (fmk.empty()) {
-		setLastError("Failed to decrypt/derive fmk");
+        libcdoc::cleanse(key);
+        // AESWrap returns {} on failure. Pad the candidate to expected
+        // length so the failure shape matches the RSA path; the bytes
+        // are arbitrary because the body decrypt is going to reject
+        // them anyway.
+        if (fmk.size() != expected_fmk_len) {
+            libcdoc::cleanse(fmk);
+            fmk.assign(expected_fmk_len, 0);
+        }
+    }
+
+    if (fmk.size() != expected_fmk_len) {
+        libcdoc::cleanse(fmk);
+        fmk.clear();
+        setLastError(FAIL_MSG);
         LOG_ERROR("{}", last_error);
-		return libcdoc::CRYPTO_ERROR;
-	}
+        return libcdoc::CRYPTO_ERROR;
+    }
     return libcdoc::OK;
 }
 
@@ -382,18 +441,47 @@ result_t CDoc1Reader::decryptData(const std::vector<uint8_t>& fmk,
         setLastError("Failed to decode base64 data");
         return libcdoc::IO_ERROR;
     }
+
+    // Treat any post-FMK decrypt error - including AES-CBC PKCS#7 stripping
+    // failures and AES-GCM tag mismatches - as the same "container body
+    // decrypt failed" event. This is the single bit of information an
+    // attacker can extract per submission of a tampered CDoc1, and we
+    // rate-limit it. A per-process exponential backoff turns a remote
+    // Bleichenbacher campaign of 2^20+ queries into hours/days of
+    // wall-clock cost without penalising legitimate single-shot use.
+    constexpr auto THROTTLE_SCOPE = "cdoc1-rsa-decrypt";
+    auto report_failure = [&]{
+        libcdoc::Crypto::rsaOracleThrottleOnFailure(THROTTLE_SCOPE);
+    };
+
     VectorSource src(b64);
     libcdoc::DecryptionSource dec(src, d->method, fmk);
     if(dec.isError()) {
-        setLastError("Failed to decrypt data, verify if FMK is correct");
+        setLastError("Failed to decrypt data");
+        report_failure();
         return CRYPTO_ERROR;
     }
+    libcdoc::result_t inner_rv = libcdoc::OK;
     if (d->mime == MIME_ZLIB) {
         libcdoc::ZSource zsrc(&dec);
-        if(auto rv = f(zsrc, d->properties["OriginalMimeType"]); rv < OK)
-            return rv;
+        inner_rv = f(zsrc, d->properties["OriginalMimeType"]);
+    } else {
+        inner_rv = f(dec, d->mime);
+    }
+    if (inner_rv < OK) {
+        // Body parse/decrypt failure. Could be a real I/O glitch, or a
+        // tampered container - we cannot tell, and on principle we treat
+        // both alike to deny the attacker a distinguisher.
+        setLastError("Failed to decrypt data");
+        report_failure();
+        return inner_rv;
     }
-    else if(auto rv = f(dec, d->mime); rv < OK)
-        return rv;
-    return dec.close();
+    libcdoc::result_t close_rv = dec.close();
+    if (close_rv != libcdoc::OK) {
+        setLastError("Failed to decrypt data");
+        report_failure();
+        return close_rv;
+    }
+    libcdoc::Crypto::rsaOracleThrottleOnSuccess(THROTTLE_SCOPE);
+    return libcdoc::OK;
 }

cdoc/CDoc2Reader.cpp

@@ -32,6 +32,7 @@
 
 #include "header_generated.h"
 
+// TODO: Port to new OpenSSL
 #define OPENSSL_SUPPRESS_DEPRECATED
 
 #include <openssl/evp.h>
@@ -141,13 +142,21 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
     LOG_DBG("CDoc2Reader::num locks: {}", priv->locks.size());
     const Lock& lock = priv->locks.at(lock_idx);
     LOG_DBG("Label: {}", lock.label);
+
+    // RAII-cleanse `kek` on every exit from this function (including
+    // exceptions). All early returns below previously had to remember to
+    // call libcdoc::cleanse(kek) - which several of them did not. With the
+    // guard the wipe is unconditional.
     std::vector<uint8_t> kek;
+    libcdoc::Cleanser kek_guard(kek);
+
     if (lock.type == Lock::Type::PASSWORD) {
         // Password
         LOG_DBG("password");
         std::string info_str = libcdoc::CDoc2::getSaltForExpand(lock.label);
         LOG_DBG("info: {}", toHex(info_str));
         std::vector<uint8_t> kek_pm;
+        libcdoc::Cleanser kek_pm_guard(kek_pm);
         if (auto rv = crypto->extractHKDF(kek_pm, lock.getBytes(Lock::SALT), lock.getBytes(Lock::PW_SALT), lock.getInt(Lock::KDF_ITER), lock_idx); rv != libcdoc::OK) {
             setLastError(crypto->getLastErrorStr(rv));
             LOG_ERROR("{}", last_error);
@@ -162,6 +171,7 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
         std::string info_str = libcdoc::CDoc2::getSaltForExpand(lock.label);
         LOG_DBG("info: {}", toHex(info_str));
         std::vector<uint8_t> kek_pm;
+        libcdoc::Cleanser kek_pm_guard(kek_pm);
         if (auto rv = crypto->extractHKDF(kek_pm, lock.getBytes(Lock::SALT), {}, 0, lock_idx); rv != libcdoc::OK) {
             setLastError(crypto->getLastErrorStr(rv));
             LOG_ERROR("{}", last_error);
@@ -173,6 +183,10 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
     } else if ((lock.type == Lock::Type::PUBLIC_KEY) || (lock.type == Lock::Type::SERVER)) {
         // Public/private key
         std::vector<uint8_t> key_material;
+        // SERVER path fetches key_material over the network; PUBLIC_KEY
+        // takes it from the lock. Either way it gets fed into ECDH or RSA
+        // and is sensitive enough to wipe in-scope.
+        libcdoc::Cleanser key_material_guard(key_material);
         if(lock.type == Lock::Type::SERVER) {
             if(!conf) {
                 setLastError("Configuration is missing");
@@ -201,8 +215,8 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
             key_material = lock.getBytes(Lock::Params::KEY_MATERIAL);
         }
 
-        LOG_DBG("Public key: {}", toHex(lock.getBytes(Lock::Params::RCPT_KEY)));
-        LOG_DBG("Key material: {}", toHex(key_material));
+        LOG_TRACE_KEY("Public key: {}", lock.getBytes(Lock::Params::RCPT_KEY));
+        LOG_TRACE_KEY("Key material: {}", key_material);
 
         if (lock.isRSA()) {
             int result = crypto->decryptRSA(kek, key_material, true, lock_idx);
@@ -213,6 +227,7 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
             }
         } else {
             std::vector<uint8_t> kek_pm;
+            libcdoc::Cleanser kek_pm_guard(kek_pm);
             int result = crypto->deriveHMACExtract(kek_pm, key_material, toUint8Vector(libcdoc::CDoc2::KEKPREMASTER), lock_idx);
             if (result < 0) {
                 setLastError(crypto->getLastErrorStr(result));
@@ -317,6 +332,10 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
                 LOG_ERROR("Cannot fetch share {}", i);
                 return result;
             }
+            // Each individual share is itself sensitive: combined with the
+            // remaining shares it reconstructs the KEK. Wipe it after
+            // XOR-ing it into kek so it does not linger on the heap.
+            libcdoc::Cleanser share_guard(share.share);
             if (auto err = libcdoc::Crypto::xor_data(kek, kek, share.share); err != libcdoc::OK) {
                 setLastError("Failed to derive kek");
                 LOG_ERROR("Failed to derive kek");
@@ -341,18 +360,27 @@ CDoc2Reader::getFMK(std::vector<uint8_t>& fmk, unsigned int lock_idx)
     if (auto err = libcdoc::Crypto::xor_data(fmk, lock.encrypted_fmk, kek); err != libcdoc::OK) {
         setLastError(t_("Failed to decrypt/derive fmk"));
         LOG_ERROR("{}", last_error);
+        // Wipe any partial XOR result before surfacing the error.
+        libcdoc::cleanse(fmk);
+        fmk.clear();
         return err;
     }
     std::vector<uint8_t> hhk = libcdoc::Crypto::expand(fmk, libcdoc::CDoc2::HMAC);
+    libcdoc::Cleanser hhk_guard(hhk);
 
     LOG_TRACE_KEY("xor: {}", lock.encrypted_fmk);
     LOG_TRACE_KEY("fmk: {}", fmk);
     LOG_TRACE_KEY("hhk: {}", hhk);
     LOG_TRACE_KEY("hmac: {}", priv->headerHMAC);
 
-    if(libcdoc::Crypto::sign_hmac(hhk, priv->header_data) != priv->headerHMAC) {
+    if(!libcdoc::constant_time_compare(libcdoc::Crypto::sign_hmac(hhk, priv->header_data), priv->headerHMAC)) {
         setLastError(t_("Wrong decryption key (user key)"));
         LOG_ERROR("{}", last_error);
+        // Authentication failed: the FMK we computed is for the wrong
+        // recipient. Wipe it before returning so the caller cannot leak
+        // it (e.g. via a logging hook that sees "fmk" in scope).
+        libcdoc::cleanse(fmk);
+        fmk.clear();
         return libcdoc::WRONG_KEY;
     }
     setLastError({});
@@ -609,7 +637,7 @@ CDoc2Reader::Private::buildLock(Lock& lock, const cdoc20::header::RecipientRecor
             std::string urls = join(strs, ";");
             LOG_DBG("Keyshare urls: {}", urls);
             std::vector<uint8_t> salt = toUint8Vector(capsule->salt());
-            LOG_DBG("Keyshare salt: {}", toHex(salt));
+            LOG_TRACE_KEY("Keyshare salt: {}", salt);
             std::string recipient_id = capsule->recipient_id()->str();
             LOG_DBG("Keyshare recipient id: {}", recipient_id);
             lock.type = Lock::SHARE_SERVER;
@@ -647,7 +675,7 @@ CDoc2Reader::CDoc2Reader(libcdoc::DataSource *src, bool take_ownership)
         LOG_ERROR("{}", last_error);
         return;
     }
-    uint32_t header_len = (c[0] << 24) | (c[1] << 16) | c[2] << 8 | c[3];
+    uint32_t header_len = (uint32_t(c[0]) << 24) | (uint32_t(c[1]) << 16) | uint32_t(c[2]) << 8 | c[3];
     if (constexpr uint32_t MAX_LEN = (1 << 20); header_len > MAX_LEN) {
         LOG_ERROR("{}", last_error);
         return;