diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e9a0b2b0..3f6ee1ed 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -118,6 +118,20 @@ jobs: with: name: ${{ matrix.target }} path: ${{ env.DEST }} + sanitizers: + name: Run unit tests with ASan+UBSan + runs-on: ubuntu-24.04 + steps: + - name: Checkout + uses: actions/checkout@v6 + - name: Install dependencies + run: sudo apt update -qq && sudo apt install --no-install-recommends -y ninja-build libboost-test-dev ${UBUNTU_DEPS} + - name: Configure + run: cmake --preset sanitize + - name: Build + run: cmake --build --preset sanitize --target unittests + - name: Test + run: ctest --test-dir build/sanitize --output-on-failure windows: name: Build on Windows runs-on: ${{ matrix.image }} diff --git a/CMakePresets.json b/CMakePresets.json index 744a600e..51bd80dd 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -124,10 +124,29 @@ "CMAKE_TOOLCHAIN_FILE": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake", "VCPKG_TARGET_TRIPLET": "$env{PLATFORM}-windows-static-md" } + }, + { + "name": "sanitize", + "displayName": "Sanitizers (ASan+UBSan)", + "description": "Debug build with AddressSanitizer and UndefinedBehaviorSanitizer for running the unit tests (system dependencies, no vcpkg; requires boost-test, openssl, libxml2, zlib and flatbuffers development packages)", + "generator": "Ninja", + "binaryDir": "${sourceDir}/build/${presetName}", + "cacheVariables": { + "CMAKE_BUILD_TYPE": "Debug", + "CMAKE_CXX_FLAGS": "-fsanitize=address,undefined -fno-omit-frame-pointer", + "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=address,undefined", + "CMAKE_SHARED_LINKER_FLAGS": "-fsanitize=address,undefined", + "CMAKE_DISABLE_FIND_PACKAGE_SWIG": "YES", + "CMAKE_DISABLE_FIND_PACKAGE_Doxygen": "YES" + } } ], "buildPresets": [ + { + "name": "sanitize", + "configurePreset": "sanitize" + }, { "name": "macos", "configurePreset": "macos" diff --git a/cdoc/CDoc1Reader.cpp b/cdoc/CDoc1Reader.cpp index cb724f0d..e2f81902 100644 --- a/cdoc/CDoc1Reader.cpp +++ b/cdoc/CDoc1Reader.cpp @@ -19,6 +19,7 @@ #include "CDoc1Reader.h" #include "Certificate.h" +#include "Configuration.h" #include "Crypto.h" #include "CryptoBackend.h" #include "DDocReader.h" @@ -28,7 +29,9 @@ #include "utils/memory.h" #include +#include +#include #include #include @@ -38,8 +41,11 @@ constexpr std::string_view MIME_ZLIB = "http://www.isi.edu/in-noes/iana/assignme constexpr std::string_view MIME_DDOC = "http://www.sk.ee/DigiDoc/v1.3.0/digidoc.xsd"; constexpr std::string_view MIME_DDOC_OLD = "http://www.sk.ee/DigiDoc/1.3.0/digidoc.xsd"; +// CDoc 1.0 used AES-CBC; CDoc 1.1 switched to AES-GCM and all CDoc 1.0 +// containers have long since expired. AES-CBC also has no authentication, +// which makes it a poor fit for the "reject at the body" FMK-oracle +// defence documented below. We therefore do not accept CBC containers. constexpr std::array SUPPORTED_METHODS { - libcdoc::Crypto::AES128CBC_MTH, libcdoc::Crypto::AES192CBC_MTH, libcdoc::Crypto::AES256CBC_MTH, libcdoc::Crypto::AES128GCM_MTH, libcdoc::Crypto::AES192GCM_MTH, libcdoc::Crypto::AES256GCM_MTH }; @@ -64,6 +70,11 @@ struct CDoc1Reader::Private int64_t f_pos = -1; std::unique_ptr src; + // N10: RSA oracle throttle state. Set during getFMK when the lock is + // RSA; empty otherwise (no throttle for ECC/AES-wrap paths). + bool is_rsa = false; + std::string throttle_key_id; + ~Private() { if (src_owned) delete dsrc; @@ -115,10 +126,10 @@ CDoc1Reader::getFMK(std::vector& fmk, unsigned int 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 + // body uses AES-128/192/256 in GCM mode (CBC was only used by CDoc 1.0, + // which we no longer accept), 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. @@ -127,9 +138,7 @@ CDoc1Reader::getFMK(std::vector& fmk, unsigned int lock_idx) // 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. + // only bit of authentication we can rely on. 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)); @@ -163,9 +172,21 @@ CDoc1Reader::getFMK(std::vector& fmk, unsigned int lock_idx) // 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. + // + // N10: mark this lock as RSA and compute a stable key identifier + // for the oracle throttle. The identifier is a SHA-256 hash of the + // recipient's public key, so different RSA keys have independent + // throttle intervals and no cross-tenant DoS is possible. + d->is_rsa = true; + std::array hash{}; + SHA256(lock.getBytes(Lock::Params::RCPT_KEY).data(), + lock.getBytes(Lock::Params::RCPT_KEY).size(), hash.data()); + d->throttle_key_id = toHex(hash); } else { - std::vector key; - int result = crypto->deriveConcatKDF(key, + d->is_rsa = false; + d->throttle_key_id.clear(); + SecureTarget key; + int result = crypto->deriveConcatKDF(key.getTarget(), lock.getBytes(Lock::Params::KEY_MATERIAL), lock.getString(Lock::Params::CONCAT_DIGEST), lock.getBytes(Lock::Params::ALGORITHM_ID), @@ -173,13 +194,12 @@ CDoc1Reader::getFMK(std::vector& fmk, unsigned int lock_idx) lock.getBytes(Lock::Params::PARTY_VINFO), lock_idx); if (result < 0) { - libcdoc::cleanse(key); setLastError(FAIL_MSG); LOG_ERROR("{}", last_error); return libcdoc::CRYPTO_ERROR; } fmk = libcdoc::Crypto::AESWrap(key, lock.encrypted_fmk, false); - libcdoc::cleanse(key); + key.cleanse(); // 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 @@ -399,7 +419,6 @@ CDoc1Reader::isCDoc1File(libcdoc::DataSource *src) result_t CDoc1Reader::decryptData(const std::vector& fmk, const std::function& f) { - setLastError({}); if (fmk.empty()) { setLastError("FMK is missing"); return libcdoc::WRONG_ARGUMENTS; @@ -414,9 +433,10 @@ result_t CDoc1Reader::decryptData(const std::vector& fmk, return libcdoc::WORKFLOW_ERROR; } if (auto result = d->dsrc->seek(0); result != libcdoc::OK) { - LOG_ERROR("{}", d->src->getLastErrorStr(result)); + LOG_ERROR("{}", d->dsrc->getLastErrorStr(result)); return result; } + setLastError({}); std::vector b64; XMLReader reader(*d->dsrc); @@ -442,16 +462,14 @@ result_t CDoc1Reader::decryptData(const std::vector& fmk, 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"; + // N10: the RSA oracle throttle is keyed by a hash of the recipient's + // public key, and only fires for RSA locks (set in getFMK). ECC/ + // AES-wrap locks have no Bleichenbacher-style oracle to protect, so + // they skip the throttle entirely. auto report_failure = [&]{ - libcdoc::Crypto::rsaOracleThrottleOnFailure(THROTTLE_SCOPE); + if (d->is_rsa && !d->throttle_key_id.empty()) { + libcdoc::Crypto::rsaOracleThrottle(d->throttle_key_id); + } }; VectorSource src(b64); @@ -463,7 +481,12 @@ result_t CDoc1Reader::decryptData(const std::vector& fmk, } libcdoc::result_t inner_rv = libcdoc::OK; if (d->mime == MIME_ZLIB) { - libcdoc::ZSource zsrc(&dec); + // N7: cap decompressed size to prevent decompression bombs. + // CDoc1 buffers whole files in memory, so a smaller default (2 GiB) + // is used compared to CDoc2's streaming default (20 GiB). + static constexpr int64_t DEFAULT_MAX = 2LL * 1024 * 1024 * 1024; + int64_t max_size = conf ? conf->getInt64(libcdoc::Configuration::CDOC1_MAX_DECOMPRESSED_SIZE, DEFAULT_MAX) : DEFAULT_MAX; + libcdoc::ZSource zsrc(&dec, false, max_size); inner_rv = f(zsrc, d->properties["OriginalMimeType"]); } else { inner_rv = f(dec, d->mime); @@ -482,6 +505,5 @@ result_t CDoc1Reader::decryptData(const std::vector& fmk, report_failure(); return close_rv; } - libcdoc::Crypto::rsaOracleThrottleOnSuccess(THROTTLE_SCOPE); return libcdoc::OK; } diff --git a/cdoc/CDoc2Reader.cpp b/cdoc/CDoc2Reader.cpp index caf6a75b..a15350d3 100644 --- a/cdoc/CDoc2Reader.cpp +++ b/cdoc/CDoc2Reader.cpp @@ -66,8 +66,7 @@ libcdoc::CDoc2::getSaltForExpand(const std::vector& key_material, const } struct CDoc2Reader::Private { - Private(libcdoc::DataSource *src, bool take_ownership) : _src(src), _owned(take_ownership) { - } + Private(libcdoc::DataSource *src, bool take_ownership) : _src(src), _owned(take_ownership) {} ~Private() { if (_owned) delete _src; @@ -117,10 +116,9 @@ CDoc2Reader::getLockForCert(const std::vector& cert){ std::vector other_key = libcdoc::Certificate(cert).getPublicKey(); if (other_key.empty()) return libcdoc::NOT_FOUND; - LOG_DBG("Cert public key: {}", toHex(other_key)); + LOG_TRACE("Cert public key: {}", toHex(other_key)); int lock_idx = 0; for (const Lock &ll : priv->locks) { - LOG_DBG("Lock {} type {}", lock_idx, (int) ll.type); if (ll.isPKI() && ll.getBytes(libcdoc::Lock::RCPT_KEY) == other_key) { return lock_idx; } @@ -147,17 +145,15 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) // 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 kek; - libcdoc::Cleanser kek_guard(kek); + SecureTarget kek; if (lock.type == Lock::Type::PASSWORD) { // Password - LOG_DBG("password"); + LOG_DBG("Password-based lock"); std::string info_str = libcdoc::CDoc2::getSaltForExpand(lock.label); - LOG_DBG("info: {}", toHex(info_str)); - std::vector 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) { + LOG_TRACE("info: {}", toHex(info_str)); + SecureTarget kek_pm; + if (auto rv = crypto->extractHKDF(kek_pm.getTarget(), 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); return rv; @@ -167,12 +163,11 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) kek = libcdoc::Crypto::expand(kek_pm, info_str, 32); } else if (lock.type == Lock::Type::SYMMETRIC_KEY) { // Symmetric key - LOG_DBG("symmetric"); + LOG_DBG("Symmetric-key based lock"); std::string info_str = libcdoc::CDoc2::getSaltForExpand(lock.label); - LOG_DBG("info: {}", toHex(info_str)); - std::vector 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) { + LOG_TRACE("info: {}", toHex(info_str)); + SecureTarget kek_pm; + if (auto rv = crypto->extractHKDF(kek_pm.getTarget(), lock.getBytes(Lock::SALT), {}, 0, lock_idx); rv != libcdoc::OK) { setLastError(crypto->getLastErrorStr(rv)); LOG_ERROR("{}", last_error); return rv; @@ -181,12 +176,12 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) LOG_TRACE_KEY("kek_pm: {}", kek_pm); kek = libcdoc::Crypto::expand(kek_pm, info_str, 32); } else if ((lock.type == Lock::Type::PUBLIC_KEY) || (lock.type == Lock::Type::SERVER)) { + LOG_DBG("Public/private key based lock"); // Public/private key - std::vector key_material; + SecureTarget 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"); @@ -206,7 +201,7 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) return libcdoc::CONFIGURATION_ERROR; } std::string transaction_id = lock.getString(Lock::Params::TRANSACTION_ID); - int result = network->fetchKey(key_material, fetch_url, transaction_id); + int result = network->fetchKey(key_material.getTarget(), fetch_url, transaction_id); if (result < 0) { setLastError(network->getLastErrorStr(result)); return result; @@ -219,16 +214,15 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) LOG_TRACE_KEY("Key material: {}", key_material); if (lock.isRSA()) { - int result = crypto->decryptRSA(kek, key_material, true, lock_idx); + int result = crypto->decryptRSA(kek.getTarget(), key_material, true, lock_idx); if (result < 0) { setLastError(crypto->getLastErrorStr(result)); LOG_ERROR("{}", last_error); return result; } } else { - std::vector kek_pm; - libcdoc::Cleanser kek_pm_guard(kek_pm); - int result = crypto->deriveHMACExtract(kek_pm, key_material, toUint8Vector(libcdoc::CDoc2::KEKPREMASTER), lock_idx); + SecureTarget kek_pm; + int result = crypto->deriveHMACExtract(kek_pm.getTarget(), key_material, toUint8Vector(libcdoc::CDoc2::KEKPREMASTER), lock_idx); if (result < 0) { setLastError(crypto->getLastErrorStr(result)); LOG_ERROR("{}", last_error); @@ -236,11 +230,12 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) } LOG_TRACE_KEY("Key kekPm: {}", kek_pm); std::string info_str = libcdoc::CDoc2::getSaltForExpand(key_material, lock.getBytes(Lock::Params::RCPT_KEY)); - LOG_DBG("info: {}", toHex(info_str)); + LOG_TRACE("info: {}", toHex(info_str)); kek = libcdoc::Crypto::expand(kek_pm, info_str, libcdoc::CDoc2::KEY_LEN); } #ifdef HAS_KEYSHARES } else if (lock.type == Lock::Type::SHARE_SERVER) { + LOG_DBG("Share server based lock"); /* SALT */ std::vector salt = lock.getBytes(Lock::SALT); /* RECIPIENT_ID */ @@ -264,7 +259,7 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) } std::string url = parts[0]; std::string id = parts[1]; - LOG_DBG("Share {} url {}", id, url); + LOG_TRACE("Share {} url {}", id, url); std::vector nonce; result_t result = network->fetchNonce(nonce, url, id); @@ -273,7 +268,7 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) LOG_ERROR("Cannot fetch nonce from server {}", url); return result; } - LOG_DBG("Nonce: {}", std::string(nonce.cbegin(), nonce.cend())); + LOG_TRACE("Nonce: {}", std::string(nonce.cbegin(), nonce.cend())); ShareData acc(url, id, std::string(nonce.cbegin(), nonce.cend())); shares.push_back(std::move(acc)); } @@ -282,7 +277,7 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) std::vector cert; result_t result = NOT_IMPLEMENTED; std::string signer = conf->getValue(Configuration::SHARE_SIGNER); - LOG_DBG("Signer: {}", signer); + LOG_TRACE("Signer: {}", signer); if (signer == "SMART_ID") { // "https://sid.demo.sk.ee/smart-id-rp/v2" std::string url = conf->getValue(Configuration::SID_DOMAIN, Configuration::BASE_URL); @@ -365,8 +360,7 @@ CDoc2Reader::getFMK(std::vector& fmk, unsigned int lock_idx) fmk.clear(); return err; } - std::vector hhk = libcdoc::Crypto::expand(fmk, libcdoc::CDoc2::HMAC); - libcdoc::Cleanser hhk_guard(hhk); + SecureTarget hhk = libcdoc::Crypto::expand(fmk, libcdoc::CDoc2::HMAC); LOG_TRACE_KEY("xor: {}", lock.encrypted_fmk); LOG_TRACE_KEY("fmk: {}", fmk); @@ -420,6 +414,7 @@ CDoc2Reader::decrypt(const std::vector& fmk, libcdoc::MultiDataConsumer libcdoc::result_t CDoc2Reader::beginDecryption(const std::vector& fmk) { + LOG_DBG("CDoc2Reader::beginDecryption"); if(fmk.size() != 32) { setLastError("No decryption key provided or invalid key length"); LOG_ERROR("{}", last_error); @@ -446,7 +441,12 @@ CDoc2Reader::beginDecryption(const std::vector& fmk) } } - priv->zsrc = std::make_unique(priv->dec.get(), false); + // N7: cap decompressed size to prevent decompression bombs. + // CDoc2 streams through TarSource to the consumer, so a larger default + // (20 GiB) is used compared to CDoc1's in-memory default (2 GiB). + static constexpr int64_t DEFAULT_MAX = 20LL * 1024 * 1024 * 1024; + int64_t max_size = conf ? conf->getInt64(libcdoc::Configuration::CDOC2_MAX_DECOMPRESSED_SIZE, DEFAULT_MAX) : DEFAULT_MAX; + priv->zsrc = std::make_unique(priv->dec.get(), false, max_size); priv->tar = std::make_unique(priv->zsrc.get(), false); return libcdoc::OK; @@ -455,20 +455,24 @@ CDoc2Reader::beginDecryption(const std::vector& fmk) libcdoc::result_t CDoc2Reader::nextFile(std::string& name, int64_t& size) { + LOG_DBG("CDoc2Reader::nextFile"); if (!priv->tar) { setLastError("nextFile() called before beginDecryption()"); LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } + return libcdoc::WORKFLOW_ERROR; + } result_t result = priv->tar->next(name, size); if (result < 0) { + // According to specification payload integrity should be reported even if there are parsing errors result_t sr = priv->decryptAllAndClose(); if (sr != OK) { + LOG_WARN("Crypto payload integrity check failed"); setLastError("Crypto payload integrity check failed"); return sr; } setLastError(priv->tar->getLastErrorStr(result)); } + LOG_DBG("CDoc2Reader::nextFile: result: {}, name: {} size: {}", result, name, size); return result; } @@ -482,19 +486,23 @@ CDoc2Reader::readData(uint8_t *dst, size_t size) } result_t result = priv->tar->read(dst, size); if (result < 0) { + // According to specification payload integrity should be reported even if there are parsing errors result_t sr = priv->decryptAllAndClose(); if (sr != OK) { + LOG_WARN("Crypto payload integrity check failed"); setLastError("Crypto payload integrity check failed"); return sr; } setLastError(priv->tar->getLastErrorStr(result)); } + LOG_DBG("CDoc2Reader::readData: result {}", result); return result; } libcdoc::result_t CDoc2Reader::finishDecryption() { + LOG_DBG("CDoc2Reader::finishDecryption"); if (!priv->tar) { setLastError("finishDecryption() called before beginDecryption()"); LOG_ERROR("{}", last_error); @@ -521,16 +529,14 @@ CDoc2Reader::Private::buildLock(Lock& lock, const cdoc20::header::RecipientRecor using namespace cdoc20::recipients; using namespace cdoc20::header; - lock.label = recipient.key_label()->str(); - lock.encrypted_fmk = toUint8Vector(recipient.encrypted_fmk()); - - if(recipient.fmk_encryption_method() != cdoc20::header::FMKEncryptionMethod::XOR) - { + if(recipient.fmk_encryption_method() != cdoc20::header::FMKEncryptionMethod::XOR) { LOG_WARN("Unsupported FMK encryption method"); return; } - switch(recipient.capsule_type()) - { + lock.label = recipient.key_label()->str(); + lock.encrypted_fmk = toUint8Vector(recipient.encrypted_fmk()); + + switch(recipient.capsule_type()) { case Capsule::recipients_ECCPublicKeyCapsule: if(const auto *capsule = recipient.capsule_as_recipients_ECCPublicKeyCapsule()) { lock.type = Lock::Type::PUBLIC_KEY; @@ -547,7 +553,7 @@ CDoc2Reader::Private::buildLock(Lock& lock, const cdoc20::header::RecipientRecor } lock.setBytes(Lock::Params::RCPT_KEY, toUint8Vector(capsule->recipient_public_key())); lock.setBytes(Lock::Params::KEY_MATERIAL, toUint8Vector(capsule->sender_public_key())); - LOG_DBG("Load PK: {}", toHex(lock.getBytes(Lock::Params::RCPT_KEY))); + LOG_TRACE("Load PK: {}", toHex(lock.getBytes(Lock::Params::RCPT_KEY))); } return; case Capsule::recipients_RSAPublicKeyCapsule: @@ -611,7 +617,17 @@ CDoc2Reader::Private::buildLock(Lock& lock, const cdoc20::header::RecipientRecor lock.type = Lock::PASSWORD; lock.setBytes(Lock::SALT, toUint8Vector(capsule->salt())); lock.setBytes(Lock::PW_SALT, toUint8Vector(capsule->password_salt())); - lock.setInt(Lock::KDF_ITER, capsule->kdf_iterations()); + // N8: the container's kdf_iterations is attacker-controlled + // int32. Reject out-of-range values at parse time to prevent + // CPU-exhaustion DoS (2^31-1 iterations = hours of PBKDF2) + // and sign-wrap confusion (values > INT32_MAX wrap negative + // and would silently take the raw symmetric-key path). + int32_t kdf_iter = capsule->kdf_iterations(); + if (kdf_iter < 1 || kdf_iter > libcdoc::CryptoBackend::KDF_ITER_MAX_DECRYPT) { + LOG_ERROR("Invalid PBKDF2 iteration count: {}", kdf_iter); + return; + } + lock.setInt(Lock::KDF_ITER, kdf_iter); } return; #ifdef HAS_KEYSHARES @@ -631,15 +647,15 @@ CDoc2Reader::Private::buildLock(Lock& lock, const cdoc20::header::RecipientRecor std::string id = cshare->share_id()->str(); std::string url = cshare->server_base_url()->str(); std::string str = url + ',' + id; - LOG_DBG("Keyshare: {}", str); - strs.push_back(std::move(str)); - } - std::string urls = join(strs, ";"); - LOG_DBG("Keyshare urls: {}", urls); - std::vector salt = toUint8Vector(capsule->salt()); - LOG_TRACE_KEY("Keyshare salt: {}", salt); - std::string recipient_id = capsule->recipient_id()->str(); - LOG_DBG("Keyshare recipient id: {}", recipient_id); + LOG_TRACE("Keyshare: {}", str); + strs.push_back(std::move(str)); + } + std::string urls = join(strs, ";"); + LOG_TRACE("Keyshare urls: {}", urls); + std::vector salt = toUint8Vector(capsule->salt()); + LOG_TRACE_KEY("Keyshare salt: {}", salt); + std::string recipient_id = capsule->recipient_id()->str(); + LOG_TRACE("Keyshare recipient id: {}", recipient_id); lock.type = Lock::SHARE_SERVER; lock.setString(Lock::SHARE_URLS, urls); lock.setBytes(Lock::SALT, salt); diff --git a/cdoc/CDocCipher.cpp b/cdoc/CDocCipher.cpp index b98b938e..6855aaff 100644 --- a/cdoc/CDocCipher.cpp +++ b/cdoc/CDocCipher.cpp @@ -204,24 +204,29 @@ struct ToolCrypto : public libcdoc::CryptoBackend { auto ctx = make_unique_ptr(EVP_PKEY_CTX_new(key.get(), nullptr)); if (!ctx) return libcdoc::CRYPTO_ERROR; - EVP_PKEY *params = nullptr; - if ((EVP_PKEY_paramgen_init(ctx.get()) < 0) || - (EVP_PKEY_CTX_set_ec_param_enc(ctx.get(), OPENSSL_EC_NAMED_CURVE) < 0) || - (EVP_PKEY_paramgen(ctx.get(), ¶ms) < 0)) + EVP_PKEY *params_raw = nullptr; + // EVP functions return 1 on success, 0 or negative on failure; use + // != 1 (not < 0) so that a 0 return is also treated as an error. + if ((EVP_PKEY_paramgen_init(ctx.get()) != 1) || + (EVP_PKEY_CTX_set_ec_param_enc(ctx.get(), OPENSSL_EC_NAMED_CURVE) != 1) || + (EVP_PKEY_paramgen(ctx.get(), ¶ms_raw) != 1)) return libcdoc::CRYPTO_ERROR; + auto params = make_unique_ptr(params_raw); p = public_key.data(); - auto pubkey = make_unique_ptr(d2i_PublicKey(EVP_PKEY_EC, ¶ms, &p, long(public_key.size()))); + auto pubkey = make_unique_ptr(d2i_PublicKey(EVP_PKEY_EC, ¶ms_raw, &p, long(public_key.size()))); if (!pubkey) return libcdoc::CRYPTO_ERROR; + // d2i_PublicKey consumed the params reference on success. + params.release(); - size_t dlen; - if ((EVP_PKEY_derive_init(ctx.get()) < 0) || - (EVP_PKEY_derive_set_peer(ctx.get(), pubkey.get()) < 0) || - (EVP_PKEY_derive(ctx.get(), nullptr, &dlen) < 0)) + size_t dlen = 0; + if ((EVP_PKEY_derive_init(ctx.get()) != 1) || + (EVP_PKEY_derive_set_peer(ctx.get(), pubkey.get()) != 1) || + (EVP_PKEY_derive(ctx.get(), nullptr, &dlen) != 1)) return libcdoc::CRYPTO_ERROR; dst.resize(dlen); - if (EVP_PKEY_derive(ctx.get(), dst.data(), &dlen) < 0) + if (EVP_PKEY_derive(ctx.get(), dst.data(), &dlen) != 1) return libcdoc::CRYPTO_ERROR; dst.resize(dlen); @@ -381,7 +386,7 @@ fill_recipients_from_rcpt_info(ToolConf& conf, ToolCrypto& crypto, std::vector& rdr, unsigned int lock_idx, const string& base_pathname) { - vector fmk; + // N18: the FMK is key material; store it in a SecureTarget (self-cleaning) + // and explicitly cleanse after the reader has consumed it. + libcdoc::SecureTarget fmk; LOG_DBG("Fetching FMK, idx=", lock_idx); - int result = rdr->getFMK(fmk, lock_idx); + int result = rdr->getFMK(fmk.getTarget(), lock_idx); LOG_DBG("Got FMK"); if (result != libcdoc::OK) { LOG_ERROR("Error on extracting FMK: {} {}", result, rdr->getLastErrorStr()); @@ -523,6 +530,8 @@ int CDocCipher::Decrypt(const unique_ptr& rdr, unsigned int lock_idx /* Do pull */ result = rdr->beginDecryption(fmk); + // The reader has consumed the FMK; cleanse it immediately. + fmk.cleanse(); if (result != libcdoc::OK) { LOG_ERROR("Error while decrypting files: {} {}", result, rdr->getLastErrorStr()); return 1; @@ -687,9 +696,11 @@ CDocCipher::ReEncrypt(ToolConf& conf, RcptInfo& dec_info, std::vector wrtr(CDocWriter::createWriter(conf.cdocVersion, conf.out, &conf, &crypto, &network)); // Begin - vector fmk; + // N18: the FMK is key material; store it in a SecureTarget (self-cleaning) + // and explicitly cleanse after the reader has consumed it. + libcdoc::SecureTarget fmk; LOG_DBG("Fetching FMK, idx={}", lock_idx); - int64_t result = rdr->getFMK(fmk, lock_idx); + int64_t result = rdr->getFMK(fmk.getTarget(), lock_idx); LOG_DBG("Got FMK"); if (result != libcdoc::OK) { LOG_ERROR("Error on extracting FMK: {} {}", result, rdr->getLastErrorStr()); @@ -704,6 +715,8 @@ CDocCipher::ReEncrypt(ToolConf& conf, RcptInfo& dec_info, std::vectorbeginDecryption(fmk); + // The reader has consumed the FMK; cleanse it immediately. + fmk.cleanse(); if (result != libcdoc::OK) { LOG_ERROR("Error while decrypting files: {} {}", result, rdr->getLastErrorStr()); return 1; diff --git a/cdoc/Certificate.cpp b/cdoc/Certificate.cpp index 1c047a41..aaefd1f1 100644 --- a/cdoc/Certificate.cpp +++ b/cdoc/Certificate.cpp @@ -109,6 +109,10 @@ Certificate::getEIDType() const if(len == NID_undef) { continue; } + // N14: OBJ_obj2txt returns the full needed length which may exceed + // the buffer when the policy OID stringifies long (crafted arcs). + // Clamp to the buffer size to prevent string_view OOB stack read. + len = std::min(len, PolicyBufferLen); std::string_view policy(buf, size_t(len)); if (policy.starts_with("2.999.")) { // Zetes TEST OID prefix diff --git a/cdoc/Configuration.cpp b/cdoc/Configuration.cpp index 256172a8..7af3fb96 100644 --- a/cdoc/Configuration.cpp +++ b/cdoc/Configuration.cpp @@ -41,7 +41,25 @@ libcdoc::Configuration::getInt(std::string_view param, int def_val) const { std::string val = getValue(param); if (val.empty()) return def_val; - return std::stoi(val); + // N15: std::stoi throws on malformed config values. Use from_chars + // and fall back to the default instead of crashing. + int result = 0; + auto [ptr, ec] = std::from_chars(val.data(), val.data() + val.size(), result); + if (ec != std::errc{} || ptr != val.data() + val.size()) + return def_val; + return result; +} + +int64_t +libcdoc::Configuration::getInt64(std::string_view param, int64_t def_val) const +{ + std::string val = getValue(param); + if (val.empty()) return def_val; + int64_t result = 0; + auto [ptr, ec] = std::from_chars(val.data(), val.data() + val.size(), result); + if (ec != std::errc{} || ptr != val.data() + val.size()) + return def_val; + return result; } struct JSONConfiguration::Private { diff --git a/cdoc/Configuration.h b/cdoc/Configuration.h index 4ca72e60..389a55e1 100644 --- a/cdoc/Configuration.h +++ b/cdoc/Configuration.h @@ -76,6 +76,16 @@ struct CDOC_EXPORT Configuration { */ static constexpr char const *PHONE_NUMBER = "PHONE_NUMBER"; #endif + /** + * @brief Maximum decompressed payload size for CDoc1 zlib content (bytes). + * Default: 2 GiB. + */ + static constexpr char const *CDOC1_MAX_DECOMPRESSED_SIZE = "CDOC1_MAX_DECOMPRESSED_SIZE"; + /** + * @brief Maximum decompressed payload size for CDoc2 zlib content (bytes). + * Default: 20 GiB. + */ + static constexpr char const *CDOC2_MAX_DECOMPRESSED_SIZE = "CDOC2_MAX_DECOMPRESSED_SIZE"; Configuration() = default; virtual ~Configuration() noexcept = default; @@ -113,6 +123,13 @@ struct CDOC_EXPORT Configuration { * @return the key value */ int getInt(std::string_view param, int def_val = 0) const; + /** + * @brief get 64-bit integer value of configuration parameter from the default domain + * @param param the parameter name + * @param def_val the default value to return if parameter is not set + * @return the key value + */ + int64_t getInt64(std::string_view param, int64_t def_val = 0) const; }; /** diff --git a/cdoc/Crypto.cpp b/cdoc/Crypto.cpp index bcf6314e..dfce5023 100644 --- a/cdoc/Crypto.cpp +++ b/cdoc/Crypto.cpp @@ -88,9 +88,9 @@ std::vector Crypto::AESWrap(const std::vector &key, const std: const EVP_CIPHER *Crypto::cipher(const std::string &algo) { - if(algo == AES128CBC_MTH) return EVP_aes_128_cbc(); - if(algo == AES192CBC_MTH) return EVP_aes_192_cbc(); - if(algo == AES256CBC_MTH) return EVP_aes_256_cbc(); + // AES-CBC was only used by CDoc 1.0; all CDoc 1.0 containers have + // expired and we no longer accept it. It is intentionally absent here + // so that unknown-method errors surface early at the CDoc1 reader. if(algo == AES128GCM_MTH) return EVP_aes_128_gcm(); if(algo == AES192GCM_MTH) return EVP_aes_192_gcm(); if(algo == AES256GCM_MTH) return EVP_aes_256_gcm(); @@ -201,7 +201,12 @@ std::vector Crypto::decodeBase64(const uint8_t *data) return result; } - if(SSL_FAILED(EVP_DecodeFinal(ctx.get(), result.data(), &size2), "EVP_DecodeFinal")) + // N13: EVP_DecodeFinal must write at result.data() + size1, not + // result.data(). For clean input OpenSSL consumes everything in + // DecodeUpdate (size2 == 0), but embedded whitespace/line breaks + // can leave work for DecodeFinal; writing at offset 0 would + // silently overwrite the first size2 bytes. + if(SSL_FAILED(EVP_DecodeFinal(ctx.get(), result.data() + size1, &size2), "EVP_DecodeFinal")) result.clear(); else result.resize(size_t(size1 + size2)); @@ -468,43 +473,46 @@ void Crypto::LogSslError(const char* funcName, const char* file, int line) namespace { -// Per-scope consecutive-failure counter. Process-wide. The mutex protects a -// small map keyed by scope string; lock contention is negligible because -// throttle invocations only happen on the failed-decrypt path which is -// already an attacker-budget-limited code path. +// Per-key last-failure timestamp. Process-wide. The mutex protects a +// small map keyed by the recipient's public-key hash; lock contention is +// negligible because throttle invocations only happen on the failed-decrypt +// path which is already an attacker-budget-limited code path. std::mutex g_throttle_mutex; -std::unordered_map g_throttle_failures; +std::unordered_map g_throttle_failures; -constexpr std::chrono::milliseconds kThrottleBase{50}; -constexpr std::chrono::milliseconds kThrottleCap{5000}; +constexpr std::chrono::milliseconds kMinFailureInterval{1000}; } // anonymous namespace -void Crypto::rsaOracleThrottleOnFailure(const std::string& scope) +void Crypto::rsaOracleThrottle(const std::string& key_id) { - unsigned int failures = 0; + const auto now = std::chrono::steady_clock::now(); + std::chrono::milliseconds delay{0}; { std::lock_guard lk(g_throttle_mutex); - failures = ++g_throttle_failures[scope]; + // Erase entries older than the minimum interval to bound map growth. + for (auto it = g_throttle_failures.begin(); it != g_throttle_failures.end(); ) { + if (now - it->second >= kMinFailureInterval) { + it = g_throttle_failures.erase(it); + } else { + ++it; + } + } + auto [entry, inserted] = g_throttle_failures.try_emplace(key_id, now); + if (!inserted) { + const auto elapsed = now - entry->second; + if (elapsed < kMinFailureInterval) { + delay = std::chrono::duration_cast(kMinFailureInterval - elapsed); + } + entry->second = now; + } } - - // delay = base * 2^(failures-1), capped at kThrottleCap. Computed on a - // wider integer to avoid overflow for very large failure counts. - auto delay = kThrottleBase; - for (unsigned int i = 1; i < failures && delay < kThrottleCap; ++i) { - delay *= 2; + // Sleep outside the mutex to avoid holding the lock during the delay. + if (delay.count() > 0) { + LOG_WARN("RSA decrypt failure (key={}); throttling for {} ms", + key_id, delay.count()); + std::this_thread::sleep_for(delay); } - if (delay > kThrottleCap) delay = kThrottleCap; - - LOG_WARN("RSA decrypt failure (scope={}, consecutive={}); throttling for {} ms", - scope, failures, delay.count()); - std::this_thread::sleep_for(delay); -} - -void Crypto::rsaOracleThrottleOnSuccess(const std::string& scope) -{ - std::lock_guard lk(g_throttle_mutex); - g_throttle_failures.erase(scope); } namespace { @@ -594,8 +602,11 @@ void unpadPKCS1v15CT(const std::vector &em, // latch the first index at which is_zero is set uint8_t latch = uint8_t(is_zero & ~found_zero); // "if latch then first_zero_idx = i". We can't branch; do it - // arithmetically. (i fits comfortably in size_t.) - const size_t mask_size = (latch == 0xFF) ? ~size_t(0) : size_t(0); + // arithmetically. N25: the ternary form below may compile to a + // secret-dependent branch; use pure arithmetic instead. + // latch is 0x00 or 0xFF, so latch & 1 is 0 or 1, and + // size_t(0) - 0 = 0 (all zeros), size_t(0) - 1 = ~0 (all ones). + const size_t mask_size = size_t(0) - size_t(latch & 1); first_zero_idx = (i & mask_size) | (first_zero_idx & ~mask_size); found_zero = uint8_t(found_zero | is_zero); } @@ -628,8 +639,15 @@ void unpadPKCS1v15CT(const std::vector &em, // range since em.size() >= 11+expected_len > 0). The clamped value // is replaced by synth[i] below when good == 0, so the actual // bytes read here never reach the caller. - size_t in_range = size_t(ge_size(em.size() - 1, src_idx)); // 0 or 0xFF - size_t mask = in_range & ~size_t(0); + // ge_size() returns a single-byte mask (0x00 or 0xFF). It must be + // widened to a full-width size_t mask before splicing indices; + // using the byte mask directly would mix the low byte of src_idx + // with the high bits of (em.size() - 1) and index past the end of + // em for modulus lengths that are not a multiple of 256 bytes + // (e.g. 384-byte EM of a 3072-bit RSA key). The widening is + // branch-free arithmetic: 0x00 -> 0, 0xFF -> ~size_t(0). + size_t in_range = size_t(ge_size(em.size() - 1, src_idx)); // 0x00 or 0xFF + size_t mask = size_t(0) - (in_range & size_t(0x01)); // 0 or ~size_t(0) size_t safe_idx = (src_idx & mask) | ((em.size() - 1) & ~mask); uint8_t real = em[safe_idx]; uint8_t synthetic = synth[i]; @@ -720,8 +738,7 @@ int Crypto::decryptRSAv15_implicitReject(std::vector& dst, EVP_PKEY_CTX_set_rsa_padding(ctx.get(), RSA_PKCS1_PADDING) == 1) { unsigned int impl_reject = 1; OSSL_PARAM params[] = { - OSSL_PARAM_construct_uint(OSSL_ASYM_CIPHER_PARAM_IMPLICIT_REJECTION, - &impl_reject), + OSSL_PARAM_construct_uint(OSSL_ASYM_CIPHER_PARAM_IMPLICIT_REJECTION, &impl_reject), OSSL_PARAM_END }; if (EVP_PKEY_CTX_set_params(ctx.get(), params) == 1) { @@ -741,6 +758,23 @@ int Crypto::decryptRSAv15_implicitReject(std::vector& dst, libcdoc::cleanse(tmp); // Length didn't match - fall through to software path so we // produce a synthetic plaintext of the correct length. + // + // N12 (accepted residual): the OpenSSL >= 3.2 fast path + // returns after one RSA operation when padding is valid + // AND the message length equals expected_len; all other + // cases fall through to the software path below (second + // RSA op + DER encode + HMAC/HKDF). An attacker with + // precise timing can therefore distinguish + // "PKCS#1-conformant with a 32-byte message" from + // everything else - a narrow Bleichenbacher-style oracle + // covering roughly 1/246 of conformant messages for + // 2048-bit keys. We accept this residual: the N10 + // per-key minimum-interval throttle limits the attacker + // to one query per second per RSA key, so extracting a + // usable oracle signal requires days of wall-clock time + // and is further constrained by the same countermeasures + // that protect the software path (constant-time unpad, + // synthetic plaintext, AES-GCM body authentication). } } } @@ -841,7 +875,7 @@ EncryptionConsumer::close() noexcept try { if(SSL_FAILED(EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_GCM_GET_TAG, int(tag.size()), tag.data()), "EVP_CIPHER_CTX_ctrl")) return CRYPTO_ERROR; - LOG_DBG("tag: {}", toHex(tag)); + LOG_TRACE_KEY("tag: {}", tag); if (dst.write(tag.data(), tag.size()) != tag.size()) return IO_ERROR; } @@ -849,7 +883,7 @@ EncryptionConsumer::close() noexcept try { if(SSL_FAILED(EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, int(tag.size()), tag.data()), "EVP_CIPHER_CTX_ctrl")) return CRYPTO_ERROR; - LOG_DBG("tag: {}", toHex(tag)); + LOG_TRACE_KEY("tag: {}", tag); if (dst.write(tag.data(), tag.size()) != tag.size()) return IO_ERROR; } @@ -959,14 +993,14 @@ result_t DecryptionSource::close() return error; if (EVP_CIPHER_CTX_mode(ctx.get()) == EVP_CIPH_GCM_MODE) { - LOG_DBG("tag: {}", toHex(tag)); + LOG_TRACE_KEY("tag: {}", tag); if (SSL_FAILED(EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_GCM_SET_TAG, int(tag.size()), tag.data()), "EVP_CIPHER_CTX_ctrl")) { return error = CRYPTO_ERROR; } } else if(EVP_CIPHER_CTX_flags(ctx.get()) & EVP_CIPH_FLAG_AEAD_CIPHER) { - LOG_DBG("tag: {}", toHex(tag)); + LOG_TRACE_KEY("tag: {}", tag); if (SSL_FAILED(EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_SET_TAG, int(tag.size()), tag.data()), "EVP_CIPHER_CTX_ctrl")) { return error = CRYPTO_ERROR; } diff --git a/cdoc/Crypto.h b/cdoc/Crypto.h index 58a7e346..b23cf768 100644 --- a/cdoc/Crypto.h +++ b/cdoc/Crypto.h @@ -44,9 +44,9 @@ class Crypto static constexpr std::string_view KWAES256_MTH = "http://www.w3.org/2001/04/xmlenc#kw-aes256"; static const std::string SHA256_MTH, SHA384_MTH, SHA512_MTH; - static constexpr std::string_view AES128CBC_MTH = "http://www.w3.org/2001/04/xmlenc#aes128-cbc"; - static constexpr std::string_view AES192CBC_MTH = "http://www.w3.org/2001/04/xmlenc#aes192-cbc"; - static constexpr std::string_view AES256CBC_MTH = "http://www.w3.org/2001/04/xmlenc#aes256-cbc"; + // AES-CBC was only used by CDoc 1.0; all CDoc 1.0 containers have + // expired and we no longer accept it. The constants are removed so + // that new code cannot accidentally re-introduce CBC support. static constexpr std::string_view AES128GCM_MTH = "http://www.w3.org/2009/xmlenc11#aes128-gcm"; static constexpr std::string_view AES192GCM_MTH = "http://www.w3.org/2009/xmlenc11#aes192-gcm"; static constexpr std::string_view AES256GCM_MTH = "http://www.w3.org/2009/xmlenc11#aes256-gcm"; @@ -135,33 +135,26 @@ class Crypto size_t expected_len); /** - * @brief Apply a delay proportional to the number of consecutive - * decrypt failures recorded for a given (process, key) pair. + * @brief Enforce a minimum interval between RSA decrypt failures + * from the same recipient key. * - * Bleichenbacher / cross-protocol attacks against RSA-PKCS#1 v1.5 require - * a large number of adaptive queries against the same victim - * ciphertext-key pair. This helper introduces an exponentially-growing - * sleep on consecutive decrypt failures, which dramatically increases - * the wall-clock cost of a remote oracle attack while remaining - * essentially invisible during normal use (one or two failures only). + * Bleichenbacher / cross-protocol attacks against RSA-PKCS#1 v1.5 + * require a large number of adaptive queries against the same victim + * key. This helper enforces a 1-second minimum interval between + * consecutive failures for a given key, dramatically increasing the + * wall-clock cost of a remote oracle campaign without penalising + * legitimate single-shot use. * - * The throttle is per-process and is designed to be advisory: long- - * running services that decrypt many containers should additionally - * implement per-recipient rate limits in their host application. + * The throttle is keyed by a hash of the recipient's public key + * (not the container ciphertext), so different RSA keys have + * independent intervals and no cross-tenant DoS is possible. * - * @param scope an arbitrary string that scopes the failure counter; use - * the recipient identifier or "default" if you don't have - * one. Different scopes have independent counters. - */ - static void rsaOracleThrottleOnFailure(const std::string& scope); - - /** - * @brief Reset the consecutive-failure counter for the given scope. + * On each call, entries older than the minimum interval are erased, + * keeping the map bounded. * - * Should be called after any successful authenticated decrypt to - * prevent the throttle from punishing legitimate retries. + * @param key_id a hex-encoded hash of the recipient's public key */ - static void rsaOracleThrottleOnSuccess(const std::string& scope); + static void rsaOracleThrottle(const std::string& key_id); /** * @brief Constant-time PKCS#1 v1.5 unpadding from a pre-decrypted EM block. diff --git a/cdoc/CryptoBackend.cpp b/cdoc/CryptoBackend.cpp index 17694cfe..0b1602fa 100644 --- a/cdoc/CryptoBackend.cpp +++ b/cdoc/CryptoBackend.cpp @@ -19,6 +19,7 @@ #include "Crypto.h" #include "CryptoBackend.h" #include "Utils.h" +#include "utils/memory.h" #define OPENSSL_SUPPRESS_DEPRECATED @@ -66,20 +67,25 @@ CryptoBackend::deriveConcatKDF(std::vector& dst, const std::vector &algorithmID, const std::vector &partyUInfo, const std::vector &partyVInfo, unsigned int idx) { - std::vector shared_secret; - int result = deriveECDH1(shared_secret, publicKey, idx); + // N22: ECDH shared_secret is key material; use SecureTarget so it is + // automatically cleansed after use instead of sitting in a plain vector. + SecureTarget shared_secret; + int result = deriveECDH1(shared_secret.getTarget(), publicKey, idx); if (result != OK) return result; dst = libcdoc::Crypto::concatKDF(digest, ECC_KEY_LEN, shared_secret, algorithmID, partyUInfo, partyVInfo); + shared_secret.cleanse(); return (dst.empty()) ? OPENSSL_ERROR : OK; } libcdoc::result_t CryptoBackend::deriveHMACExtract(std::vector& dst, const std::vector &public_key, const std::vector &salt, unsigned int idx) { - std::vector shared_secret; - int result = deriveECDH1(shared_secret, public_key, idx); + // N22: same cleansing for the HKDF-extract path. + SecureTarget shared_secret; + int result = deriveECDH1(shared_secret.getTarget(), public_key, idx); if (result != OK) return result; dst = libcdoc::Crypto::extract(shared_secret, salt); + shared_secret.cleanse(); return (dst.empty()) ? OPENSSL_ERROR : OK; } @@ -116,6 +122,12 @@ CryptoBackend::extractHKDF(std::vector& kek_pm, const std::vector INT32_MAX) would silently take the + // raw-key path below, turning a password lock into a (failing) + // symmetric-key lock. Reject them here so the failure is explicit. + if (kdf_iter < 0) return INVALID_PARAMS; if ((kdf_iter > 0) && pw_salt.empty()) return INVALID_PARAMS; std::vector key_material; int result = getKeyMaterial(key_material, pw_salt, kdf_iter, idx); diff --git a/cdoc/CryptoBackend.h b/cdoc/CryptoBackend.h index d3ce3c62..c5b0c400 100644 --- a/cdoc/CryptoBackend.h +++ b/cdoc/CryptoBackend.h @@ -45,6 +45,22 @@ struct CDOC_EXPORT CryptoBackend { static constexpr int ECC_KEY_LEN = 32; + // N8: PBKDF2 iteration bounds. + // + // The container carries an attacker-controlled int32 kdf_iterations + // field. Without bounds a malicious container can either: + // - set kdf_iterations = 2^31-1 → CPU DoS (hours of PBKDF2 per attempt) + // - set kdf_iterations > INT32_MAX → sign-wrap to negative → the raw + // symmetric-key path is taken instead of the password path + // + // Encryption limits (writer side): passwords must use at least + // KDF_ITER_MIN_ENCRYPT iterations and at most KDF_ITER_MAX_ENCRYPT. + // Decryption limit (reader side): containers with more than + // KDF_ITER_MAX_DECRYPT iterations are rejected outright. + static constexpr int32_t KDF_ITER_MIN_ENCRYPT = 100000; + static constexpr int32_t KDF_ITER_MAX_ENCRYPT = 10000000; + static constexpr int32_t KDF_ITER_MAX_DECRYPT = 100000000; + enum HashAlgorithm : uint32_t { SHA_224, SHA_256, diff --git a/cdoc/Io.cpp b/cdoc/Io.cpp index 5106212c..2f342067 100644 --- a/cdoc/Io.cpp +++ b/cdoc/Io.cpp @@ -144,7 +144,10 @@ result_t FileListConsumer::open(const std::string &name, int64_t size) { } ofs.open(target, std::ios_base::binary); - return ofs.bad() ? OUTPUT_STREAM_ERROR : OK; + // N23: ofs.bad() misses failbit set by a failed open() (e.g. permission + // denied, read-only filesystem). Check fail() instead so extraction + // reports the error instead of "succeeding" with nothing written. + return ofs.fail() ? OUTPUT_STREAM_ERROR : OK; } FileListSource::FileListSource(const std::string& base, const std::vector& files) diff --git a/cdoc/KeyShares.cpp b/cdoc/KeyShares.cpp index 23789012..2ef3d4fd 100644 --- a/cdoc/KeyShares.cpp +++ b/cdoc/KeyShares.cpp @@ -68,7 +68,7 @@ struct JWTSigner { JWTSigner(Signer *_parent) : parent(_parent) {} std::string sign(const std::string& data, std::error_code& ec) const { - LOG_DBG("Sign JWT: {}", data); + LOG_TRACE("Sign JWT: {}", data); std::vector digest(32); SHA256((uint8_t *) data.c_str(), data.size(), digest.data()); std::vector dst; @@ -170,11 +170,11 @@ Signer::generateTickets(std::vector& dst, std::vector& s std::vector disclosures; for (auto share : shares) { Disclosure &d = disclosures.emplace_back(std::string{}, share.getURL()); - LOG_DBG("Disclosure for {}: {}", share.base_url, d.json); + LOG_TRACE("Disclosure for {}: {}", share.base_url, d.json); } // Create disclosure of the whole list Disclosure aud("aud", disclosures); - LOG_DBG("Full disclosure: {}", aud.json); + LOG_TRACE("Full disclosure: {}", aud.json); // Create and sign JWT container error = {}; @@ -186,9 +186,9 @@ Signer::generateTickets(std::vector& dst, std::vector& s .set_payload_claim("_sd", picojson::value(_sd)) .set_payload_claim("_sd_alg", picojson::value("sha-256")) .sign(jwtsig); - LOG_DBG("Token: {}", token); + LOG_TRACE("Token: {}", token); if (result != OK) { - LOG_DBG("Jwt signing failed with code {}", result); + LOG_TRACE("Jwt signing failed with code {}", result); return result; } @@ -199,7 +199,7 @@ Signer::generateTickets(std::vector& dst, std::vector& s for (unsigned int i = 0; i < disclosures.size(); i++) { std::string disclosed = jwt + "~" + toBase64URL(disclosures[i].json) + "~"; dst.push_back(disclosed); - LOG_DBG("Ticket for {}: {}", shares[i].base_url, disclosed); + LOG_TRACE("Ticket for {}: {}", shares[i].base_url, disclosed); } return OK; @@ -215,9 +215,9 @@ SIDSigner::signDigest(std::vector& dst, const std::vector& dig error = network->getLastErrorStr(result); } - LOG_DBG("SID dignature:{}", toHex(dst)); - LOG_DBG("SID signatureB64:{}", toBase64URL(dst)); - LOG_DBG("SID certificateB64:{}", toBase64(cert)); + LOG_TRACE("SID dignature:{}", toHex(dst)); + LOG_TRACE("SID signatureB64:{}", toBase64URL(dst)); + LOG_TRACE("SID certificateB64:{}", toBase64(cert)); return result; } @@ -233,9 +233,9 @@ libcdoc::MIDSigner::signDigest(std::vector& dst, const std::vectorgetLastErrorStr(result); } - LOG_DBG("MID signature:{}", toHex(dst)); - LOG_DBG("MID signatureB64:{}", toBase64URL(dst)); - LOG_DBG("MID certificateB64:{}", toBase64(cert)); + LOG_TRACE("MID signature:{}", toHex(dst)); + LOG_TRACE("MID signatureB64:{}", toBase64URL(dst)); + LOG_TRACE("MID certificateB64:{}", toBase64(cert)); return result; } diff --git a/cdoc/Lock.cpp b/cdoc/Lock.cpp index 983e7c3c..8ca54604 100644 --- a/cdoc/Lock.cpp +++ b/cdoc/Lock.cpp @@ -83,8 +83,16 @@ Lock::parseLabel(const std::string& label) base64IndPos != std::string::npos) { std::string base64_label(label_wo_prefix.substr(base64IndPos + CDoc2::LABELBASE64IND.size())); - decodedBase64 = jwt::base::decode(base64_label); - label_to_prcss = decodedBase64; + // jwt::base::decode throws std::runtime_error on malformed base64. + // The label comes from the (untrusted) container, so a malformed + // label must not crash the process - treat it as unparseable. + try { + decodedBase64 = jwt::base::decode(base64_label); + label_to_prcss = decodedBase64; + } catch (const std::exception &e) { + LOG_WARN("The label '{}' contains invalid base64: {}", label, e.what()); + return parsed_label; + } } else if (label_wo_prefix.starts_with(",")) { label_to_prcss = label_wo_prefix.substr(1); } else { diff --git a/cdoc/NetworkBackend.cpp b/cdoc/NetworkBackend.cpp index 27ccbb7c..7ff8f5fb 100644 --- a/cdoc/NetworkBackend.cpp +++ b/cdoc/NetworkBackend.cpp @@ -206,7 +206,7 @@ setPeerCertificates(httplib::SSLClient& cli, libcdoc::NetworkBackend *network, c error = FORMAT("Cannot get peer certificate list: {}", result); return result; } - libcdoc::LOG_DBG("Num TLS certs {}", certs.size()); + LOG_DBG("Num TLS certs {}", certs.size()); if (!certs.empty()) { SSL_CTX *ctx = cli.ssl_context(); SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); @@ -274,7 +274,7 @@ static libcdoc::result_t post(httplib::SSLClient& cli, const std::string& path, httplib::Headers& hdrs, const std::string& req, httplib::Response& rsp) { // Capture TLS and HTTP errors - libcdoc::LOG_DBG("POST: {} {}", path, req); + LOG_TRACE("POST: {} {}", path, req); httplib::Result res = cli.Post(path, hdrs, req, "application/json"); if (!res) { error = FORMAT("Cannot connect to https://{}:{}{}", cli.host(), cli.port(), path); @@ -316,7 +316,7 @@ get(httplib::SSLClient& cli, httplib::Headers& hdrs, const std::string& path, pi libcdoc::result_t libcdoc::NetworkBackend::sendKey (CapsuleInfo& dst, const std::string& url, const std::vector& rcpt_key, const std::vector &key_material, const std::string& type, uint64_t expiry_ts) { - LOG_DBG("Sendkey"); + LOG_DBG("NetworkBackend::Sendkey"); picojson::object obj = { {"recipient_id", picojson::value(libcdoc::toBase64(rcpt_key))}, {"ephemeral_key_material", picojson::value(libcdoc::toBase64(key_material))}, @@ -367,7 +367,13 @@ libcdoc::NetworkBackend::sendKey (CapsuleInfo& dst, const std::string& url, cons dst.expiry_time = expiry_ts; LOG_DBG("Given expiry timestamp: {}", dst.expiry_time); } else { - dst.expiry_time = uint64_t(timeFromISO(expiry_str)); + double parsed = timeFromISO(expiry_str); + if (parsed < 0) { + LOG_WARN("Invalid server expiry '{}', using client-supplied expiry", expiry_str); + dst.expiry_time = expiry_ts; + } else { + dst.expiry_time = uint64_t(parsed); + } LOG_DBG("Server expiry timestamp: {}", dst.expiry_time); } @@ -385,8 +391,8 @@ libcdoc::NetworkBackend::sendShare(std::vector& dst, const std::string& }; picojson::value req_json(obj); std::string req_str = req_json.serialize(); - LOG_DBG("POST keyshare to: {}", url); - LOG_DBG("{}", req_str); + LOG_TRACE("POST keyshare to: {}", url); + LOG_TRACE("{}", req_str); std::string host, path; int port; @@ -418,7 +424,7 @@ libcdoc::NetworkBackend::sendShare(std::vector& dst, const std::string& error = {}; dst.assign(location.cbegin() + prefix.size(), location.cend()); - LOG_DBG("Share: {}", std::string((const char *) dst.data(), dst.size())); + LOG_TRACE("Share: {}", std::string((const char *) dst.data(), dst.size())); return OK; } @@ -458,6 +464,10 @@ libcdoc::NetworkBackend::fetchKey (std::vector& dst, const std::string& error = {}; std::string ks = v.get(); dst = fromBase64(ks); + if (dst.empty()) { + error = FORMAT("Invalid base64 in 'ephemeral_key_material'"); + return NETWORK_ERROR; + } return libcdoc::OK; } @@ -466,14 +476,14 @@ libcdoc::NetworkBackend::fetchKey (std::vector& dst, const std::string& libcdoc::result_t libcdoc::NetworkBackend::fetchNonce(std::vector& dst, const std::string& url, const std::string& share_id) { - LOG_DBG("Get nonce from: {}", url); + LOG_TRACE("Get nonce from: {}", url); std::string host, path; int port; int result = libcdoc::parseURL(url, host, port, path); if (result != libcdoc::OK) return result; - LOG_DBG("Starting client: {} {}", host, port); + LOG_TRACE("Starting client: {} {}", host, port); httplib::SSLClient cli(host, port); if (result = applySSLTimeout(cli, this); result != OK) return result; result = setPeerCertificates(cli, this, buildURL(host, port)); @@ -486,7 +496,7 @@ libcdoc::NetworkBackend::fetchNonce(std::vector& dst, const std::string result = post(cli, full, hdrs, "", rsp); if (result != libcdoc::OK) return result; - LOG_DBG("Response: {}", rsp.body); + LOG_TRACE("Response: {}", rsp.body); picojson::value rsp_json; std::string parse_err = picojson::parse(rsp_json, rsp.body); if (!parse_err.empty()) { @@ -507,14 +517,14 @@ libcdoc::NetworkBackend::fetchNonce(std::vector& dst, const std::string libcdoc::result_t libcdoc::NetworkBackend::fetchShare(ShareInfo& share, const std::string& url, const std::string& share_id, const std::string& ticket, const std::vector& cert) { - LOG_DBG("Get share from: {}", url); + LOG_TRACE("Get share from: {}", url); std::string host, path; int port; int result = libcdoc::parseURL(url, host, port, path); if (result != libcdoc::OK) return result; - LOG_DBG("Starting client: {} {}", host, port); + LOG_TRACE("Starting client: {} {}", host, port); httplib::SSLClient cli(host, port); if (result = applySSLTimeout(cli, this); result != OK) return result; @@ -523,7 +533,7 @@ libcdoc::NetworkBackend::fetchShare(ShareInfo& share, const std::string& url, co if (result = setProxy(cli, this); result != OK) return result; std::string full = path + "/key-shares/" + share_id; - LOG_DBG("Share url: {}", full); + LOG_TRACE("Share url: {}", full); httplib::Headers hdrs; hdrs.insert({"x-cdoc2-auth-ticket", ticket}); hdrs.insert({"x-cdoc2-auth-x5c", std::string("-----BEGIN CERTIFICATE-----") + toBase64(cert) + "-----END CERTIFICATE-----"}); @@ -537,7 +547,7 @@ libcdoc::NetworkBackend::fetchShare(ShareInfo& share, const std::string& url, co return NETWORK_ERROR; } std::string share64 = v.get(); - LOG_DBG("Share64: {}", share64); + LOG_TRACE("Share64: {}", share64); v = rsp_json.get("recipient"); if (!v.is()) { error = FORMAT("No 'recipient' in response"); @@ -549,7 +559,7 @@ libcdoc::NetworkBackend::fetchShare(ShareInfo& share, const std::string& url, co error = FORMAT("Invalid share size: expected 32, got {}", shareval.size()); return NETWORK_ERROR; } - LOG_DBG("Share: {}", toHex(shareval)); + LOG_TRACE("Share: {}", toHex(shareval)); share = {std::move(shareval), std::move(recipient)}; return OK; } @@ -636,7 +646,7 @@ waitForResult(SIDResponse& dst, httplib::SSLClient& cli, const std::string& path double end = libcdoc::getTime() + seconds; std::string full = path + session_id + "?timeoutMs=" + std::to_string((int) (seconds * 1000)); - LOG_DBG("SID/MID session query path: {}", full); + LOG_TRACE("SID/MID session query path: {}", full); while (libcdoc::getTime() < end) { picojson::value rsp; result_t result = get(cli, hdrs, full, rsp); @@ -760,18 +770,18 @@ libcdoc::NetworkBackend::signSID(std::vector& dst, std::vector {"nonce", picojson::value(nonce)} }; picojson::value query(obj); - LOG_DBG("JSON:{}", query.serialize()); + LOG_TRACE("JSON:{}", query.serialize()); std::string host, path; int port; int result = libcdoc::parseURL(url, host, port, path); if (result != libcdoc::OK) return result; - LOG_DBG("URL:{}", url); - LOG_DBG("HOST:{}", host); - LOG_DBG("PORT:{}", port); - LOG_DBG("PATH:{}", path); + LOG_TRACE("URL:{}", url); + LOG_TRACE("HOST:{}", host); + LOG_TRACE("PORT:{}", port); + LOG_TRACE("PATH:{}", path); - LOG_DBG("Starting client: {} {}", host, port); + LOG_TRACE("Starting client: {} {}", host, port); httplib::SSLClient cli(host, port); if (result = applySSLTimeout(cli, this); result != OK) return result; result = setPeerCertificates(cli, this, buildURL(host, port)); @@ -782,14 +792,14 @@ libcdoc::NetworkBackend::signSID(std::vector& dst, std::vector // Let user choose certificate (if multiple) // std::string full = path + "/certificatechoice/" + rcpt_id; - LOG_DBG("SmartID path: {}", full); + LOG_TRACE("SmartID path: {}", full); httplib::Headers hdrs; httplib::Response rsp; result = post(cli, full, hdrs, query.serialize(), rsp); if (result != libcdoc::OK) return result; - LOG_DBG("Response: {}", rsp.body); + LOG_TRACE("Response: {}", rsp.body); picojson::value v; std::string parse_err = picojson::parse(v, rsp.body); if (!parse_err.empty()) { @@ -809,12 +819,12 @@ libcdoc::NetworkBackend::signSID(std::vector& dst, std::vector return NetworkBackend::NETWORK_ERROR; } std::string sessionID = w.get(); - LOG_DBG("SessionID: {}", sessionID); + LOG_TRACE("SessionID: {}", sessionID); SIDResponse sidrsp; result = waitForResult(sidrsp, cli, path + "/session/", sessionID, 60, true); if (result != OK) return result; - LOG_DBG("Certificate: {}", sidrsp.cert); + LOG_TRACE("Certificate: {}", sidrsp.cert); // // Sign @@ -857,15 +867,15 @@ libcdoc::NetworkBackend::signSID(std::vector& dst, std::vector } }; query = picojson::value(qobj); - LOG_DBG("JSON:{}", query.serialize()); + LOG_TRACE("JSON:{}", query.serialize()); // // Sign digest // full = path + "/authentication/" + rcpt_id; - LOG_DBG("SmartID path: {}", full); + LOG_TRACE("SmartID path: {}", full); result = post(cli, full, hdrs, query.serialize(), rsp); if (result != libcdoc::OK) return result; - LOG_DBG("Response: {}", rsp.body); + LOG_TRACE("Response: {}", rsp.body); parse_err = picojson::parse(v, rsp.body); if (!parse_err.empty()) { error = FORMAT("JSON parse error: {}", parse_err); @@ -884,13 +894,13 @@ libcdoc::NetworkBackend::signSID(std::vector& dst, std::vector return NetworkBackend::NETWORK_ERROR; } sessionID = w.get(); - LOG_DBG("SessionID: {}", sessionID); + LOG_TRACE("SessionID: {}", sessionID); sidrsp = {}; result = waitForResult(sidrsp, cli, path + "/session/", sessionID, 60, true); if (result != OK) return result; - LOG_DBG("Certificate: {}", sidrsp.cert); - LOG_DBG("Signature: {}", sidrsp.signature); + LOG_TRACE("Certificate: {}", sidrsp.cert); + LOG_TRACE("Signature: {}", sidrsp.signature); dst = fromBase64(sidrsp.signature); cert = fromBase64(sidrsp.cert); @@ -938,12 +948,12 @@ libcdoc::NetworkBackend::signMID(std::vector& dst, std::vector int port; int result = libcdoc::parseURL(url, host, port, path); if (result != libcdoc::OK) return result; - LOG_DBG("URL:{}", url); - LOG_DBG("HOST:{}", host); - LOG_DBG("PORT:{}", port); - LOG_DBG("PATH:{}", path); + LOG_TRACE("URL:{}", url); + LOG_TRACE("HOST:{}", host); + LOG_TRACE("PORT:{}", port); + LOG_TRACE("PATH:{}", path); - LOG_DBG("Starting client: {} {}", host, port); + LOG_TRACE("Starting client: {} {}", host, port); httplib::SSLClient cli(host, port); if (result = applySSLTimeout(cli, this); result != OK) return result; result = setPeerCertificates(cli, this, buildURL(host, port)); @@ -978,17 +988,17 @@ libcdoc::NetworkBackend::signMID(std::vector& dst, std::vector {"displayTextFormat", picojson::value("GSM-7")} }; picojson::value query = picojson::value(qobj); - LOG_DBG("JSON:{}", query.serialize()); + LOG_TRACE("JSON:{}", query.serialize()); // // Sign digest // std::string full = path + "/authentication"; - LOG_DBG("Mobile ID path: {}", full); + LOG_TRACE("Mobile ID path: {}", full); httplib::Headers hdrs; httplib::Response rsp; result = post(cli, full, hdrs, query.serialize(), rsp); if (result != libcdoc::OK) return result; - LOG_DBG("Response: {}", rsp.body); + LOG_TRACE("Response: {}", rsp.body); picojson::value v; parse_err = picojson::parse(v, rsp.body); @@ -1009,14 +1019,14 @@ libcdoc::NetworkBackend::signMID(std::vector& dst, std::vector return NetworkBackend::NETWORK_ERROR; } std::string sessionID = w.get(); - LOG_DBG("SessionID: {}", sessionID); + LOG_TRACE("SessionID: {}", sessionID); SIDResponse sidrsp; result = waitForResult(sidrsp, cli, path + "/authentication/session/", sessionID, 60, false); if (result != OK) return result; - LOG_DBG("Certificate: {}", sidrsp.cert); - LOG_DBG("Signature: {}", sidrsp.signature); + LOG_TRACE("Certificate: {}", sidrsp.cert); + LOG_TRACE("Signature: {}", sidrsp.signature); dst = fromBase64(sidrsp.signature); cert = fromBase64(sidrsp.cert); diff --git a/cdoc/PKCS11Backend.cpp b/cdoc/PKCS11Backend.cpp index 7a2bf1c7..ac7894c1 100644 --- a/cdoc/PKCS11Backend.cpp +++ b/cdoc/PKCS11Backend.cpp @@ -112,7 +112,12 @@ libcdoc::PKCS11Backend::Private::login(int slot, const std::vector& pin case CKR_CANCEL: case CKR_FUNCTION_CANCELED: LOG_DBG("PKCS11:C_Login CANCELED"); - break; + // N21: CKR_CANCEL/CKR_FUNCTION_CANCELED mean the login was + // aborted; the session is not authenticated. Close it and + // return an error instead of falling through as success. + f->C_CloseSession(session); + session = CK_INVALID_HANDLE; + return PKCS11_ERROR; default: LOG_DBG("PKCS11:C_Login {}", result); f->C_CloseSession(session); @@ -246,6 +251,7 @@ libcdoc::PKCS11Backend::PKCS11Backend(const std::string &driver) libcdoc::PKCS11Backend::~PKCS11Backend() { + if (!d) return; if(d->f) { d->f->C_Finalize(nullptr); d->f = nullptr; diff --git a/cdoc/Recipient.cpp b/cdoc/Recipient.cpp index 89e4f4c1..cc4c6e2c 100644 --- a/cdoc/Recipient.cpp +++ b/cdoc/Recipient.cpp @@ -21,6 +21,7 @@ #include "CDoc2.h" #include "Certificate.h" #include "Crypto.h" +#include "CryptoBackend.h" #include "Lock.h" #include "Utils.h" @@ -233,7 +234,17 @@ Recipient::validate() const switch(type) { case SYMMETRIC_KEY: // Either user-defined label or LABEL property is required - return !label.empty() || lbl_parts.contains(std::string(CDoc2::Label::LABEL)); + if (label.empty() && !lbl_parts.contains(std::string(CDoc2::Label::LABEL))) + return false; + // N8: enforce PBKDF2 iteration bounds on the writer side. + // kdf_iter == 0 means a raw symmetric key (no PBKDF2); any + // password lock must use a bounded iteration count to avoid + // both trivially weak and CPU-exhausting containers. + if (kdf_iter != 0 && + (kdf_iter < CryptoBackend::KDF_ITER_MIN_ENCRYPT || + kdf_iter > CryptoBackend::KDF_ITER_MAX_ENCRYPT)) + return false; + return true; case PUBLIC_KEY: // Public key should not be empty return !rcpt_key.empty(); diff --git a/cdoc/Tar.cpp b/cdoc/Tar.cpp index 383f49e8..05353729 100644 --- a/cdoc/Tar.cpp +++ b/cdoc/Tar.cpp @@ -124,7 +124,12 @@ struct libcdoc::Header { } std::string getName() const { - return std::string(name.data(), std::min(name.size(), strlen(name.data()))); + // N20: strlen over-reads into adjacent header fields when the + // 100-byte name field lacks a NUL terminator. Use memchr with an + // explicit bound instead. + const char *nul = static_cast(memchr(name.data(), '\0', name.size())); + size_t len = nul ? size_t(nul - name.data()) : name.size(); + return std::string(name.data(), len); } constexpr int64_t getSize() const noexcept { diff --git a/cdoc/Utils.cpp b/cdoc/Utils.cpp index 9495e2d0..cc34228c 100644 --- a/cdoc/Utils.cpp +++ b/cdoc/Utils.cpp @@ -42,8 +42,18 @@ toBase64(const uint8_t *data, size_t len) std::vector fromBase64(std::string_view data) { - std::string str = jwt::base::details::decode(data, jwt::alphabet::base64::rdata(), "="); - return std::vector(str.cbegin(), str.cend()); + // jwt::base::details::decode throws std::runtime_error on malformed + // input (characters outside the alphabet, bad padding, bad length). + // The decoded data comes from remote servers and containers, i.e. it + // is untrusted, so a decode failure must not crash the process. An + // empty result signals a format error to the callers. + try { + std::string str = jwt::base::details::decode(data, jwt::alphabet::base64::rdata(), "="); + return std::vector(str.cbegin(), str.cend()); + } catch (const std::exception &e) { + LOG_WARN("fromBase64: invalid base64 input: {}", e.what()); + return {}; + } } double @@ -56,12 +66,16 @@ getTime() #define timegm _mkgmtime #endif +// N17: return -1 on parse failure so callers can distinguish garbage +// from a valid timestamp. Previously a malformed ISO string produced +// an undefined time_t via std::get_time without checking in.fail(). double timeFromISO(const std::string& iso) { std::istringstream in{iso}; std::tm t = {}; in >> std::get_time(&t, "%Y-%m-%dT%TZ"); + if (in.fail()) return -1; return timegm(&t); } @@ -223,6 +237,7 @@ sanitiseExtractedFilename(std::string_view name) // truncate at NUL while the filesystem treats the full name, which // has historically been used to mask malicious extensions. if (name.empty()) return {}; + if (!isValidUtf8(std::string(name))) return {}; for (unsigned char c : name) { if (c == 0u) return {}; if (c < 0x20u && c != '\t') return {}; // strip ASCII control bytes @@ -247,6 +262,11 @@ sanitiseExtractedFilename(std::string_view name) base = base.substr(2); } + // 3a. Reject any remaining ':' (NTFS Alternate Data Stream separator). + // A name like "file.txt:evil.exe" would create the ADS "evil.exe" + // on the file "file.txt" instead of a normal file. + if (base.find(':') != std::string_view::npos) return {}; + // 4. Trim trailing dots and whitespace. Windows silently strips these // when creating files, so "evil.exe.." resolves to "evil.exe" and // can collide with or hide a legitimate file. Trim leading @@ -287,8 +307,18 @@ sanitiseExtractedFilename(std::string_view name) // A name longer than that would fail filesystem operations anyway; // truncating up-front gives a uniform error mode. We truncate from // the end while keeping the file extension if there is one. + // Truncation respects UTF-8 character boundaries so a multi-byte + // codepoint is never split in half. constexpr size_t MAX_BYTES = 255; if (base.size() > MAX_BYTES) { + // Find the last UTF-8 codepoint boundary at or before MAX_BYTES. + // A continuation byte has the pattern 10xxxxxx (0x80-0xBF). + auto utf8_boundary = [](std::string_view sv, size_t max_pos) -> size_t { + size_t pos = std::min(max_pos, sv.size()); + while (pos > 0 && (uint8_t(sv[pos - 1]) & 0xC0) == 0x80) + --pos; + return pos; + }; size_t dot = base.find_last_of('.'); if (dot != std::string_view::npos && dot > 0 && @@ -296,14 +326,15 @@ sanitiseExtractedFilename(std::string_view name) // Preserve a short extension; truncate the stem. std::string_view ext = base.substr(dot); std::string_view stem = base.substr(0, dot); - size_t keep_stem = MAX_BYTES - ext.size(); + size_t keep_stem = utf8_boundary(stem, MAX_BYTES - ext.size()); std::string out; - out.reserve(MAX_BYTES); + out.reserve(keep_stem + ext.size()); out.assign(stem.data(), keep_stem); out.append(ext.data(), ext.size()); return out; } - return std::string(base.substr(0, MAX_BYTES)); + size_t keep = utf8_boundary(base, MAX_BYTES); + return std::string(base.substr(0, keep)); } return std::string(base); diff --git a/cdoc/Utils.h b/cdoc/Utils.h index d2e9d570..5de076fc 100644 --- a/cdoc/Utils.h +++ b/cdoc/Utils.h @@ -162,9 +162,12 @@ std::string buildURL(const std::string& host, int port); * - "." and ".." segments, * - NUL bytes and other ASCII control characters, * - leading/trailing whitespace and dots (Windows trims these silently), - * - reserved Windows device names (CON, PRN, AUX, NUL, COM1..COM9, LPT1..LPT9), - * - excessively long names (capped at 255 bytes after sanitisation, the - * practical filename limit on every filesystem libcdoc supports). + * - reserved Windows device names (CON, PRN, AUX, NUL, COM1..COM9, LPT1..LPT9), + * - NTFS Alternate Data Stream separator ':', + * - malformed UTF-8, + * - excessively long names (capped at 255 bytes after sanitisation, the + * practical filename limit on every filesystem libcdoc supports; + * truncation respects UTF-8 character boundaries). * * The returned string is a relative file name (no slashes), or empty if no * safe name could be derived. A caller that gets an empty return value MUST @@ -315,15 +318,15 @@ static inline void LogFormat(LogLevel level, std::string_view file, int line, st #define LOG_INFO(...) LogFormat(libcdoc::LEVEL_INFO, __FILE__, __LINE__, __VA_ARGS__) #define LOG_DBG(...) LogFormat(libcdoc::LEVEL_DEBUG, __FILE__, __LINE__, __VA_ARGS__) -#ifdef NDEBUG -#define LOG_TRACE(...) -#else -#define LOG_TRACE(...) LogFormat(libcdoc::LEVEL_TRACE, __FILE__, __LINE__, __VA_ARGS__) -#endif - +// LOG_TRACE and LOG_TRACE_KEY are compile-gated by LIBCDOC_CRYPTO_TRACE +// (default OFF). They are intended for debugging cryptographic material and +// other potentially sensitive data. Never use LOG_DBG for secrets — it is +// runtime-gated only and may be enabled in production deployments. #ifdef LIBCDOC_CRYPTO_TRACE +#define LOG_TRACE(...) LogFormat(libcdoc::LEVEL_TRACE, __FILE__, __LINE__, __VA_ARGS__) #define LOG_TRACE_KEY(MSG, KEY) LogFormat(libcdoc::LEVEL_TRACE, __FILE__, __LINE__, MSG, toHex(KEY)) #else +#define LOG_TRACE(...) #define LOG_TRACE_KEY(MSG, KEY) #endif diff --git a/cdoc/XmlReader.cpp b/cdoc/XmlReader.cpp index 1fc8a382..76a52beb 100644 --- a/cdoc/XmlReader.cpp +++ b/cdoc/XmlReader.cpp @@ -43,6 +43,22 @@ static std::string tostring(pcxmlChar tmp) return result; } +#if LIBXML_VERSION < 21300 +static xmlParserInputPtr +nullExternalEntityLoader(const char *, const char *, xmlParserCtxtPtr) +{ + return nullptr; +} + +struct XmlInit { + XmlInit() { + xmlSetExternalEntityLoader(nullExternalEntityLoader); + xmlSubstituteEntitiesDefault(0); + } +}; +static XmlInit xmlInit; +#endif + XMLReader::XMLReader(libcdoc::DataSource &src) : d(xmlReaderForIO([](void *context, char *buffer, int len) -> int { auto *src = reinterpret_cast(context); diff --git a/cdoc/ZStream.h b/cdoc/ZStream.h index eecfc5b5..f4fa823f 100644 --- a/cdoc/ZStream.h +++ b/cdoc/ZStream.h @@ -92,8 +92,12 @@ struct ZSource : public DataSource { int64_t _error = OK; std::vector buf; int flush = Z_NO_FLUSH; - ZSource(DataSource *src, bool take_ownership = false) - : _src(src), _owned(take_ownership) { + /// Maximum total bytes this source will produce. 0 means unlimited. + /// Prevents decompression bombs (N7). + int64_t max_decompressed_size = 0; + int64_t total_inflated = 0; + ZSource(DataSource *src, bool take_ownership = false, int64_t max_size = 0) + : _src(src), _owned(take_ownership), max_decompressed_size(max_size) { if (inflateInit2(&_s, MAX_WBITS) != Z_OK) { _error = ZLIB_ERROR; } @@ -117,6 +121,7 @@ struct ZSource : public DataSource { if (n_read > 0) { buf.insert(buf.end(), in.begin(), in.begin() + n_read); } else if (n_read != 0) { + inflateEnd(&_s); _error = n_read; return _error; } @@ -132,12 +137,19 @@ struct ZSource : public DataSource { buf.clear(); break; default: + inflateEnd(&_s); _error = ZLIB_ERROR; return _error; } } size_t produced = chunk - _s.avail_out; total_produced += produced; + total_inflated += produced; + if (max_decompressed_size > 0 && total_inflated > max_decompressed_size) { + inflateEnd(&_s); + _error = IO_ERROR; + return _error; + } if (produced == 0) break; // no progress (EOF or stream end) } return total_produced; diff --git a/cdoc/cdoc-tool.cpp b/cdoc/cdoc-tool.cpp index 3e655b74..290ae835 100644 --- a/cdoc/cdoc-tool.cpp +++ b/cdoc/cdoc-tool.cpp @@ -101,8 +101,15 @@ print_usage(ostream& ofs) static std::vector fromB64(const std::string& data) { - std::string str = jwt::base::details::decode(data, jwt::alphabet::base64::rdata(), "="); - return std::vector(str.cbegin(), str.cend()); + // jwt::base::details::decode throws std::runtime_error on malformed + // base64; an invalid --accept certificate file must not crash the tool. + try { + std::string str = jwt::base::details::decode(data, jwt::alphabet::base64::rdata(), "="); + return std::vector(str.cbegin(), str.cend()); + } catch (const std::exception &e) { + LOG_WARN("Invalid base64: {}", e.what()); + return {}; + } } static void @@ -115,7 +122,11 @@ load_certs(ToolConf& conf, const std::string& filename) for (auto part : parts) { if (part.size() > 3) { std::vector v = fromB64(part); - conf.accept_certs.push_back(v); + if (v.empty()) { + LOG_WARN("Skipping invalid base64 line in {}", filename); + continue; + } + conf.accept_certs.push_back(std::move(v)); } } } else { diff --git a/cdoc/utils/memory.h b/cdoc/utils/memory.h index c441a5a2..74a97f73 100644 --- a/cdoc/utils/memory.h +++ b/cdoc/utils/memory.h @@ -191,6 +191,70 @@ class SecureBytes { } }; +// +// A self-cleaning writable container for temporary secrets. +// +// We allow getting a reference to the actual content vector to be used in library calls, but +// all existing contents will be cleansed first in that case. +// + +class SecureTarget { + std::vector data_; +public: + using iterator = std::vector::iterator; + using const_iterator = std::vector::const_iterator; + + SecureTarget() noexcept = default; + + ~SecureTarget() { + cleanse(); + } + + SecureTarget(const SecureTarget& other) = delete; + SecureTarget(SecureTarget&& other) = delete; + SecureTarget& operator=(const SecureTarget& other) = delete; + SecureTarget& operator=(SecureTarget&& other) = delete; + // Need a plain constructor for declaration-initialisation case + SecureTarget(std::vector v) noexcept : data_(std::move(v)) {} + // Assignment should first cleanse and then copy/move + SecureTarget& operator=(const std::vector& v) { + cleanse(); + data_ = v; + return *this; + } + SecureTarget& operator=(std::vector&& v) { + cleanse(); + data_ = std::move(v); + return *this; + } + + [[nodiscard]] bool empty() const noexcept { return data_.empty(); } + [[nodiscard]] size_t size() const noexcept { return data_.size(); } + [[nodiscard]] const uint8_t* data() const noexcept { return data_.data(); } + [[nodiscard]] const_iterator cbegin() const noexcept { return data_.cbegin(); } + [[nodiscard]] const_iterator cend() const noexcept { return data_.cend(); } + [[nodiscard]] const_iterator begin() const noexcept { return data_.begin(); } + [[nodiscard]] const_iterator end() const noexcept { return data_.end(); } + + [[nodiscard]] operator const std::vector&() const noexcept { return data_; } + + // Get writable vector + // Any secret, if present, is cleansed first to avoid leaking previous contents + std::vector& getTarget() { + cleanse(); + return data_; + } + std::vector& getTarget(size_t size) { + cleanse(size); + return data_; + } + + void cleanse(size_t size = 0) noexcept { + ::libcdoc::cleanse(data_); + data_.resize(size); + } +}; + /** * @brief Scope guard that wipes a contiguous secret on destruction. * diff --git a/libcdoc.i b/libcdoc.i index 6bc01d3f..0f6cf611 100644 --- a/libcdoc.i +++ b/libcdoc.i @@ -73,6 +73,8 @@ %ignore libcdoc::Configuration::RP_UUID; %ignore libcdoc::Configuration::RP_NAME; %ignore libcdoc::Configuration::PHONE_NUMBER; +%ignore libcdoc::Configuration::CDOC1_MAX_DECOMPRESSED_SIZE; +%ignore libcdoc::Configuration::CDOC2_MAX_DECOMPRESSED_SIZE; %ignore libcdoc::PKCS11Backend::Handle; %ignore libcdoc::PKCS11Backend::findCertificates(const std::string& label); @@ -654,6 +656,8 @@ static std::vector SWIG_JavaArrayToVectorUnsignedChar(JNIEnv *jen public static final String RP_UUID = "RP_UUID"; public static final String RP_NAME = "RP_NAME"; public static final String PHONE_NUMBER = "PHONE_NUMBER"; + public static final String CDOC1_MAX_DECOMPRESSED_SIZE = "CDOC1_MAX_DECOMPRESSED_SIZE"; + public static final String CDOC2_MAX_DECOMPRESSED_SIZE = "CDOC2_MAX_DECOMPRESSED_SIZE"; %} %typemap(javaimports) ArrayList %{ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 56b1ae3e..b33bef39 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -3,6 +3,7 @@ add_executable(unittests ../cdoc/Crypto.cpp ../cdoc/Tar.cpp ../cdoc/XmlReader.cpp + ../cdoc/Utils.cpp ) target_link_libraries(unittests @@ -10,6 +11,7 @@ target_link_libraries(unittests LibXml2::LibXml2 cdoc Boost::unit_test_framework + ZLIB::ZLIB ) add_test(NAME runtest diff --git a/test/libcdoc_boost.cpp b/test/libcdoc_boost.cpp index 544c00b9..8769c044 100644 --- a/test/libcdoc_boost.cpp +++ b/test/libcdoc_boost.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -720,6 +721,9 @@ BOOST_FIXTURE_TEST_CASE_WITH_DECOR(EncryptWithPasswordAndLabel, FixtureBase, * u } BOOST_TEST(reader->nextFile(fi) == libcdoc::END_OF_STREAM); BOOST_TEST(reader->finishDecryption() == libcdoc::OK); + + delete writer; + delete reader; } BOOST_AUTO_TEST_SUITE_END() @@ -811,6 +815,51 @@ BOOST_AUTO_TEST_CASE(LabelParsingEmptyLabel) } } +// N3 regression: the base64 decoder (jwt::base::decode) throws +// std::runtime_error on malformed input. A crafted container label must +// not crash the process; the label is reported as unparseable instead. +BOOST_AUTO_TEST_CASE(Base64LabelParsingInvalidBase64) +{ + // Characters outside the base64 alphabet. + BOOST_CHECK(libcdoc::Lock::parseLabel("data:;base64,###").empty()); + // Valid alphabet but impossible length (single character). + BOOST_CHECK(libcdoc::Lock::parseLabel("data:;base64,A").empty()); + // Too much padding. + BOOST_CHECK(libcdoc::Lock::parseLabel("data:;base64,QQ===").empty()); + // Same, with a media type part in front. + BOOST_CHECK(libcdoc::Lock::parseLabel("data:application/x-www-form-urlencoded;base64,###").empty()); + // Trailing garbage after otherwise valid base64. + BOOST_CHECK(libcdoc::Lock::parseLabel("data:;base64,dj0x###").empty()); +} + +BOOST_AUTO_TEST_SUITE_END() + +// N3 regression: libcdoc::fromBase64 decodes untrusted data (key server +// and share server responses). Malformed input must yield an empty vector, +// not an exception. +BOOST_AUTO_TEST_SUITE(FromBase64) + +BOOST_AUTO_TEST_CASE(ValidInput) +{ + // "hello world" + std::vector expected {'h', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd'}; + BOOST_CHECK(libcdoc::fromBase64("aGVsbG8gd29ybGQ=") == expected); + BOOST_CHECK(libcdoc::fromBase64("").empty()); +} + +BOOST_AUTO_TEST_CASE(InvalidInputReturnsEmpty) +{ + // Characters outside the alphabet. + BOOST_CHECK(libcdoc::fromBase64("###").empty()); + BOOST_CHECK(libcdoc::fromBase64("aGVsbG8###").empty()); + // Impossible lengths (not a multiple of 4 after padding rules). + BOOST_CHECK(libcdoc::fromBase64("A").empty()); + // Excess padding. + BOOST_CHECK(libcdoc::fromBase64("QQ===").empty()); + // Padding in the middle. + BOOST_CHECK(libcdoc::fromBase64("QQ==QQ==").empty()); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE(TarPaxHeader) @@ -982,6 +1031,61 @@ BOOST_AUTO_TEST_CASE(AllowsReasonablePaxHeaderSize) BOOST_CHECK_NE(rv, libcdoc::DATA_FORMAT_ERROR); } +// Regression for SecurityReview_Kilo_2026-07 N20: Header::getName ran +// strlen on a 100-byte field that may contain no NUL, over-reading into +// adjacent header fields. The fix uses memchr with an explicit bound. +BOOST_AUTO_TEST_CASE(NameWithoutNulDoesNotOverread) +{ + // Build a tar header whose 100-byte name field has NO NUL terminator. + std::vector block(512, 0); + // Fill name field with 'A' - no NUL anywhere in the 100 bytes. + std::fill(block.begin(), block.begin() + 100, uint8_t('A')); + + // mode, uid, gid, size, mtime (valid octal, as in makeTarHeader) + auto write_octal_field = [&](size_t offset, size_t width, int64_t value) { + std::string s(width - 1, '0'); + for (size_t i = 0; i < width - 1 && value > 0; ++i) { + s[width - 2 - i] = char('0' + (value & 7)); + value >>= 3; + } + std::copy(s.begin(), s.end(), block.begin() + offset); + }; + write_octal_field(100, 8, 0600); + write_octal_field(108, 8, 0); + write_octal_field(116, 8, 0); + write_octal_field(124, 12, 0); + write_octal_field(136, 12, 0); + + // chksum: spaces during calculation + std::fill(block.begin() + 148, block.begin() + 156, uint8_t(' ')); + block[156] = uint8_t('0'); // regular file + constexpr std::string_view magic{"ustar\0", 6}; + std::copy(magic.begin(), magic.end(), block.begin() + 257); + block[263] = '0'; + block[264] = '0'; + + int64_t sum = 0; + for (uint8_t b : block) sum += b; + std::string chk(7, '0'); + for (size_t i = 0; i < 6 && sum > 0; ++i) { + chk[5 - i] = char('0' + (sum & 7)); + sum >>= 3; + } + chk[6] = '\0'; + std::copy(chk.begin(), chk.end(), block.begin() + 148); + block[155] = ' '; + + libcdoc::VectorSource src(block); + libcdoc::TarSource tar_src(&src, false); + std::string name; + int64_t size = 0; + auto rv = tar_src.next(name, size); + // The name must be exactly 100 bytes (the full field), not longer. + BOOST_CHECK_EQUAL(rv, libcdoc::OK); + BOOST_CHECK_EQUAL(name.size(), 100); + BOOST_CHECK(name.find_first_not_of('A') == std::string::npos); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE(StreamingDecryption) @@ -1031,6 +1135,174 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(constructor, Buf, BufTypes) BOOST_AUTO_TEST_SUITE_END() +// Regression for SecurityReview_Kilo_2026-07 N7: ZSource had no limit on +// total decompressed output, so a few-KB compressed container could expand +// to gigabytes of memory (CDoc1) or disk (CDoc2). The reader must cap the +// total inflated size and fail with IO_ERROR past the limit. +BOOST_AUTO_TEST_SUITE(ZSourceLimit) + +// A zlib stream of repeating zeros compresses to almost nothing; without +// a cap the reader would happily inflate unlimited attacker-controlled +// output. +BOOST_AUTO_TEST_CASE(EnforcesMaxDecompressedSize) +{ + // Build a compressed stream that expands well past the cap. + // 1 MiB of zeros compresses to ~1 KiB. + const size_t PLAIN_SIZE = 1024 * 1024; + std::vector plain(PLAIN_SIZE, 0); + std::vector compressed; + libcdoc::VectorConsumer dst(compressed); + libcdoc::ZConsumer enc(&dst, false); + libcdoc::VectorSource src(plain); + src.readAll(enc); + BOOST_REQUIRE_EQUAL(enc.close(), libcdoc::OK); + BOOST_REQUIRE(compressed.size() < plain.size() / 100); // sanity: it compressed + + // Read with a 64 KiB cap — must fail with IO_ERROR, not return the full MiB. + libcdoc::VectorSource comp_src(compressed); + libcdoc::ZSource zsrc(&comp_src, false, 64 * 1024); + std::vector buf(4096); + libcdoc::result_t total = 0; + while (true) { + auto rv = zsrc.read(buf.data(), buf.size()); + if (rv < 0) { + BOOST_CHECK_EQUAL(rv, libcdoc::IO_ERROR); + break; + } + if (rv == 0) break; + total += rv; + } + BOOST_CHECK(zsrc.isError()); + // The limited read should have produced less than the full payload. + BOOST_CHECK(total < PLAIN_SIZE); +} + +// Without a cap (max_size = 0) the stream must succeed as before. +BOOST_AUTO_TEST_CASE(UnlimitedWhenCapIsZero) +{ + const std::vector plain = {'h', 'e', 'l', 'l', 'o'}; + std::vector compressed; + libcdoc::VectorConsumer dst(compressed); + libcdoc::ZConsumer enc(&dst, false); + libcdoc::VectorSource src(plain); + src.readAll(enc); + BOOST_REQUIRE_EQUAL(enc.close(), libcdoc::OK); + + libcdoc::VectorSource comp_src(compressed); + libcdoc::ZSource zsrc(&comp_src, false, 0); // no cap + std::vector buf(plain.size()); + auto rv = zsrc.read(buf.data(), buf.size()); + BOOST_CHECK_EQUAL(rv, plain.size()); + BOOST_CHECK_EQUAL_COLLECTIONS(buf.begin(), buf.end(), plain.begin(), plain.end()); +} + +BOOST_AUTO_TEST_SUITE_END() + +// Regression for SecurityReview_Kilo_2026-07 N2: AES-CBC was only used by +// CDoc 1.0, which is long expired, and the DecryptionSource CBC path was +// broken anyway (the `size != out` invariant fails for padded CBC). +// Support has been removed entirely; the Crypto::cipher lookup and the +// CDoc1 reader's SUPPORTED_METHODS list must not let CBC methods through. +BOOST_AUTO_TEST_SUITE(AesCbcRemoved) + +BOOST_AUTO_TEST_CASE(CipherLookupRejectsCbc) +{ + constexpr std::array cbcMethods { + "http://www.w3.org/2001/04/xmlenc#aes128-cbc", + "http://www.w3.org/2001/04/xmlenc#aes192-cbc", + "http://www.w3.org/2001/04/xmlenc#aes256-cbc", + }; + for (const char *m : cbcMethods) { + BOOST_CHECK(libcdoc::Crypto::cipher(m) == nullptr); + } +} + +BOOST_AUTO_TEST_CASE(CipherLookupStillAcceptsGcm) +{ + BOOST_CHECK(libcdoc::Crypto::cipher(std::string(libcdoc::Crypto::AES128GCM_MTH)) != nullptr); + BOOST_CHECK(libcdoc::Crypto::cipher(std::string(libcdoc::Crypto::AES192GCM_MTH)) != nullptr); + BOOST_CHECK(libcdoc::Crypto::cipher(std::string(libcdoc::Crypto::AES256GCM_MTH)) != nullptr); +} + +BOOST_AUTO_TEST_SUITE_END() + +// Regression for SecurityReview_Kilo_2026-07 N8: PBKDF2 kdf_iterations +// from the container is attacker-controlled int32. Without bounds it +// enables both CPU-exhaustion DoS (huge iteration counts) and sign-wrap +// confusion (values > INT32_MAX wrap negative and silently take the raw +// symmetric-key path). The writer must enforce [100k, 10M] and the +// reader must reject > 100M. +BOOST_AUTO_TEST_SUITE(Pbkdf2IterationBounds) + +BOOST_AUTO_TEST_CASE(WriterRejectsTooFewIterations) +{ + auto rcpt = libcdoc::Recipient::makeSymmetric("test", 99'999); + BOOST_CHECK(!rcpt.validate()); +} + +BOOST_AUTO_TEST_CASE(WriterAcceptsMinimumIterations) +{ + auto rcpt = libcdoc::Recipient::makeSymmetric("test", 100'000); + BOOST_CHECK(rcpt.validate()); +} + +BOOST_AUTO_TEST_CASE(WriterAcceptsMaximumIterations) +{ + auto rcpt = libcdoc::Recipient::makeSymmetric("test", 10'000'000); + BOOST_CHECK(rcpt.validate()); +} + +BOOST_AUTO_TEST_CASE(WriterRejectsTooManyIterations) +{ + auto rcpt = libcdoc::Recipient::makeSymmetric("test", 10'000'001); + BOOST_CHECK(!rcpt.validate()); +} + +BOOST_AUTO_TEST_CASE(WriterAcceptsZeroIterations) +{ + // kdf_iter == 0 means a raw symmetric key (no PBKDF2); valid. + auto rcpt = libcdoc::Recipient::makeSymmetric("test", 0); + BOOST_CHECK(rcpt.validate()); +} + +BOOST_AUTO_TEST_CASE(ReaderRejectsNegativeIterations) +{ + // Negative kdf_iter (possible from sign-wrap when the container's + // unsigned 4-byte field is read as signed int32) must be rejected + // before it silently takes the raw-key path. + TestCrypto crypto; + crypto.password = "test"; + std::vector kek_pm; + std::vector salt(16, 0); + std::vector pw_salt(16, 0); + auto rv = crypto.extractHKDF(kek_pm, salt, pw_salt, -1, 0); + BOOST_CHECK_EQUAL(rv, libcdoc::CryptoBackend::INVALID_PARAMS); +} + +BOOST_AUTO_TEST_SUITE_END() + +// Regression for SecurityReview_Kilo_2026-07 N17: timeFromISO did not +// check std::get_time failure, so a malformed server expiry produced +// garbage time_t that was cast to uint64_t. The function now returns -1 +// on parse failure and the caller falls back to the client-supplied +// expiry with a warning. +BOOST_AUTO_TEST_SUITE(TimeFromISO) + +BOOST_AUTO_TEST_CASE(ParsesValidISO) +{ + double rv = libcdoc::timeFromISO("2026-08-18T12:00:00Z"); + BOOST_CHECK(rv > 0); +} + +BOOST_AUTO_TEST_CASE(RejectsInvalidISO) +{ + BOOST_CHECK_EQUAL(libcdoc::timeFromISO("not-a-date"), -1); + BOOST_CHECK_EQUAL(libcdoc::timeFromISO(""), -1); + BOOST_CHECK_EQUAL(libcdoc::timeFromISO("2026-13-45T99:99:99Z"), -1); +} + +BOOST_AUTO_TEST_SUITE_END() + // Regression coverage for libcdoc::sanitiseExtractedFilename(). All inputs // here come from attacker-controlled archive headers (tar / DDoc); the // helper is the single chokepoint that decides whether an entry can ever @@ -1140,6 +1412,46 @@ BOOST_AUTO_TEST_CASE(TruncatesOverlongNames) BOOST_CHECK_EQUAL(truncated.size(), 255u); } +// N24: UTF-8 validation, boundary-aware truncation, and NTFS ADS ':' rejection. +BOOST_AUTO_TEST_CASE(RejectsMalformedUtf8) +{ + // Lone continuation byte + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename(std::string("\x80.txt")), ""); + // Truncated multi-byte sequence + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename(std::string("\xC3.txt")), ""); + // Invalid lead byte + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename(std::string("\xFE.txt")), ""); + // Valid UTF-8 still passes + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename("\xC3\xB5.txt"), "\xC3\xB5.txt"); +} + +BOOST_AUTO_TEST_CASE(TruncatesAtUtf8Boundary) +{ + // Build a name with 254 bytes of 'a' + a 2-byte UTF-8 char at position 254-255. + // Naive truncation at 255 would split the codepoint. + std::string name(254, 'a'); + name += "\xC3\xB5"; // 'õ' - 2-byte UTF-8 + name += ".txt"; + auto result = libcdoc::sanitiseExtractedFilename(name); + // The 2-byte character at the boundary must not be split. + // Result should be <= 255 and the last bytes before .txt should not + // be a lone continuation byte. + BOOST_CHECK_LE(result.size(), 255u); + BOOST_CHECK(result.ends_with(".txt")); + // Extract the stem and verify it ends at a codepoint boundary + std::string stem = result.substr(0, result.size() - 4); + BOOST_CHECK(libcdoc::isValidUtf8(stem)); +} + +BOOST_AUTO_TEST_CASE(RejectsNtfsAdsColon) +{ + // NTFS ADS: "file.txt:stream" creates an alternate data stream on file.txt + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename("file.txt:evil.exe"), ""); + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename("file.txt:stream"), ""); + // Drive-relative is still handled (stripped, not rejected) + BOOST_CHECK_EQUAL(libcdoc::sanitiseExtractedFilename("C:foo.txt"), "foo.txt"); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE(XMLReaderEntityHandling) @@ -1333,8 +1645,9 @@ BOOST_AUTO_TEST_CASE(RejectsNonDigitNationalId) { BOOST_CHECK(!libcdoc::parseEtsiRecipientId("etsi/PNOEE-30303039 14").valid()); BOOST_CHECK(!libcdoc::parseEtsiRecipientId("etsi/PNOEE-3030303991a").valid()); - // Embedded NUL. - BOOST_CHECK(!libcdoc::parseEtsiRecipientId(std::string("etsi/PNOEE-3030\0039914", 22)).valid()); + // Embedded NUL. (sizeof - 1: the literal is 20 chars; a hard-coded + // length of 22 read 2 bytes past it - caught by ASan.) + BOOST_CHECK(!libcdoc::parseEtsiRecipientId(std::string("etsi/PNOEE-3030\0039914", sizeof("etsi/PNOEE-3030\0039914") - 1)).valid()); } BOOST_AUTO_TEST_CASE(RejectsOversizedNationalId) @@ -1350,3 +1663,104 @@ BOOST_AUTO_TEST_CASE(RejectsOversizedNationalId) } BOOST_AUTO_TEST_SUITE_END() + +// Regression coverage for the constant-time PKCS#1 v1.5 unpadding used by +// the RSA implicit-rejection path (N1 in SecurityReview_Kilo_2026-07.md). +// The index-clamping mask in unpadPKCS1v15CT was a single byte (0x00/0xFF) +// instead of a full-width size_t mask, which spliced the low byte of the +// source index with the high bits of (em.size() - 1) and read past the end +// of the EM buffer for modulus lengths that are not a multiple of 256 +// bytes (e.g. the 384-byte EM of a 3072-bit RSA key, up to 128 bytes OOB). +BOOST_AUTO_TEST_SUITE(RsaImplicitRejectUnpad) + +// Sweep the zero separator across the whole EM block: output must be the +// real message exactly when the padding is valid (00 02 || PS>=8 || 00 || +// M of expected_len) and the synthetic plaintext in every other case. +// Under ASAN this also fails on any out-of-bounds EM access. +static void sweepSeparatorPositions(size_t em_len) +{ + constexpr size_t expected_len = 32; + std::vector synth(expected_len); + for (size_t i = 0; i < expected_len; i++) + synth[i] = uint8_t(0xA0 + i); + + for (size_t sep = 2; sep < em_len; sep++) { + std::vector em(em_len, 0x55); + em[0] = 0x00; + em[1] = 0x02; + em[sep] = 0x00; + + std::vector dst; + BOOST_REQUIRE_EQUAL(libcdoc::Crypto::rsaImplicitRejectFromEM(dst, em, {0x01}, synth, expected_len), libcdoc::OK); + BOOST_REQUIRE_EQUAL(dst.size(), expected_len); + + const size_t msg_len = em_len - sep - 1; + const bool expect_real = (sep >= 10) && (msg_len == expected_len); + for (size_t i = 0; i < expected_len; i++) { + const uint8_t want = expect_real ? em[sep + 1 + i] : synth[i]; + BOOST_CHECK_EQUAL(dst[i], want); + } + } +} + +BOOST_AUTO_TEST_CASE(SeparatorSweepAllModulusSizes) +{ + sweepSeparatorPositions(192); // 1536-bit RSA + sweepSeparatorPositions(256); // 2048-bit RSA + sweepSeparatorPositions(384); // 3072-bit RSA (read up to +128 bytes OOB before the fix) + sweepSeparatorPositions(512); // 4096-bit RSA +} + +BOOST_AUTO_TEST_CASE(ValidPaddingReturnsMessage3072) +{ + // Valid-padding 3072-bit case (message at the end of the EM block); + // the old byte-wide mask happened to compute these indices correctly. + // The actual OOB reproducer is the separator sweep above: for 384-byte + // EMs, separator positions 127..254 made the old mask splice read past + // the buffer (padding is invalid there, so only ASAN observes it). + constexpr size_t em_len = 384; + constexpr size_t expected_len = 32; + constexpr size_t sep = em_len - expected_len - 1; + std::vector em(em_len, 0x55); + em[0] = 0x00; + em[1] = 0x02; + em[sep] = 0x00; + std::vector synth(expected_len, 0xAA); + + std::vector dst; + BOOST_REQUIRE_EQUAL(libcdoc::Crypto::rsaImplicitRejectFromEM(dst, em, {0x01}, synth, expected_len), libcdoc::OK); + BOOST_REQUIRE_EQUAL(dst.size(), expected_len); + for (size_t i = 0; i < expected_len; i++) + BOOST_CHECK_EQUAL(dst[i], em[sep + 1 + i]); +} + +BOOST_AUTO_TEST_CASE(BadHeaderReturnsSynthetic) +{ + constexpr size_t em_len = 384; + constexpr size_t expected_len = 32; + std::vector em(em_len, 0x55); + em[0] = 0x01; // wrong leading byte + em[1] = 0x02; + em[em_len - expected_len - 1] = 0x00; + std::vector synth(expected_len, 0xAA); + + std::vector dst; + BOOST_REQUIRE_EQUAL(libcdoc::Crypto::rsaImplicitRejectFromEM(dst, em, {0x01}, synth, expected_len), libcdoc::OK); + BOOST_CHECK(dst == synth); +} + +BOOST_AUTO_TEST_CASE(NoSeparatorReturnsSynthetic) +{ + constexpr size_t em_len = 384; + constexpr size_t expected_len = 32; + std::vector em(em_len, 0x55); + em[0] = 0x00; + em[1] = 0x02; + std::vector synth(expected_len, 0xAA); + + std::vector dst; + BOOST_REQUIRE_EQUAL(libcdoc::Crypto::rsaImplicitRejectFromEM(dst, em, {0x01}, synth, expected_len), libcdoc::OK); + BOOST_CHECK(dst == synth); +} + +BOOST_AUTO_TEST_SUITE_END()