From ad7a18e80fbaa8ec81d4feff83330f92e1fb4fe0 Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Tue, 31 Mar 2026 16:15:04 +0300 Subject: [PATCH 1/2] Check recipient validity in addRecipient --- cdoc/CDoc1Writer.cpp | 7 +- cdoc/CDoc2Writer.cpp | 270 +++++++++++++++-------------------------- cdoc/CDoc2Writer.h | 21 ++-- cdoc/CDocCipher.cpp | 6 + cdoc/Recipient.cpp | 19 +++ cdoc/Recipient.h | 7 ++ test/libcdoc_boost.cpp | 4 +- 7 files changed, 148 insertions(+), 186 deletions(-) diff --git a/cdoc/CDoc1Writer.cpp b/cdoc/CDoc1Writer.cpp index a7726aa3..534304d5 100644 --- a/cdoc/CDoc1Writer.cpp +++ b/cdoc/CDoc1Writer.cpp @@ -273,7 +273,12 @@ CDoc1Writer::addRecipient(const libcdoc::Recipient& rcpt) { if(d) return WORKFLOW_ERROR; - rcpts.push_back(rcpt); + if (!rcpt.isCertificate()) { + setLastError("Invalid recipient type"); + LOG_ERROR("{}", last_error); + return WRONG_ARGUMENTS; + } + rcpts.push_back(rcpt); return libcdoc::OK; } diff --git a/cdoc/CDoc2Writer.cpp b/cdoc/CDoc2Writer.cpp index 7e7c39d5..da3fe466 100644 --- a/cdoc/CDoc2Writer.cpp +++ b/cdoc/CDoc2Writer.cpp @@ -31,6 +31,8 @@ #include +#define FAIL(m,r) return fail(m, r) + using namespace libcdoc; CDoc2Writer::CDoc2Writer(libcdoc::DataConsumer *dst, bool take_ownership) @@ -43,11 +45,8 @@ CDoc2Writer::~CDoc2Writer() noexcept = default; libcdoc::result_t CDoc2Writer::writeHeader(const std::vector &recipients) { - if(recipients.empty()) { - setLastError("No recipients specified"); - LOG_ERROR("{}", last_error); - return libcdoc::WRONG_ARGUMENTS; - } + if(recipients.empty()) + FAIL("No recipients specified", libcdoc::WRONG_ARGUMENTS); std::vector rnd; if(auto rv = crypto->random(rnd, libcdoc::CDoc2::KEY_LEN); rv < 0) return rv; @@ -205,36 +204,21 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector key_material, kek; std::string send_url; if(rcpt.isKeyServer()) { - if(!conf) { - setLastError("Configuration is missing"); - LOG_ERROR("{}", last_error); - return libcdoc::CONFIGURATION_ERROR; - } - if(!network) { - setLastError("Network backend is missing"); - LOG_ERROR("{}", last_error); - return libcdoc::CONFIGURATION_ERROR; - } + if(!conf) + FAIL("Configuration is missing", libcdoc::CONFIGURATION_ERROR); + if(!network) + FAIL("Network backend is missing", libcdoc::CONFIGURATION_ERROR); send_url = conf->getValue(rcpt.server_id, libcdoc::Configuration::KEYSERVER_SEND_URL); - if (send_url.empty()) { - setLastError("Missing keyserver URL for ID " + rcpt.server_id); - LOG_ERROR("{}", last_error); - return libcdoc::CONFIGURATION_ERROR; - } + if (send_url.empty()) + FAIL("Missing keyserver URL for ID " + rcpt.server_id, libcdoc::CONFIGURATION_ERROR); } if(rcpt.pk_type == libcdoc::PKType::RSA) { crypto->random(kek, libcdoc::CDoc2::KEY_LEN); - if (libcdoc::Crypto::xor_data(xor_key, fmk, kek) != libcdoc::OK) { - setLastError("Internal error"); - LOG_ERROR("{}", last_error); - return libcdoc::CRYPTO_ERROR; - } + if (libcdoc::Crypto::xor_data(xor_key, fmk, kek) != libcdoc::OK) + FAIL("Internal error", libcdoc::CRYPTO_ERROR); auto publicKey = libcdoc::Crypto::fromRSAPublicKeyDer(rcpt.rcpt_key); - if(!publicKey) { - setLastError("Invalid RSA key"); - LOG_ERROR("{}", last_error); - return libcdoc::CRYPTO_ERROR; - } + if(!publicKey) + FAIL("Invalid RSA key", libcdoc::CRYPTO_ERROR); key_material = libcdoc::Crypto::encrypt(publicKey.get(), RSA_PKCS1_OAEP_PADDING, kek); LOG_TRACE_KEY("publicKeyDer: {}", rcpt.rcpt_key); @@ -244,11 +228,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vectorsendKey(cinfo, send_url, rcpt.rcpt_key, key_material, "rsa", rcpt.expiry_ts); - if (result < 0) { - setLastError(network->getLastErrorStr(result)); - LOG_ERROR("{}", last_error); - return libcdoc::IO_ERROR; - } + if (result < 0) + FAIL(network->getLastErrorStr(result), result); LOG_DBG("Keyserver Id: {}", rcpt.server_id); LOG_DBG("Transaction Id: {}", cinfo.transaction_id); @@ -259,11 +240,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector sharedSecret = libcdoc::Crypto::deriveSharedSecret(ephKey.get(), publicKey.get()); key_material = libcdoc::Crypto::toPublicKeyDer(ephKey.get()); @@ -271,11 +249,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector& header, const std::vectorsendKey(cinfo, send_url, rcpt.rcpt_key, key_material, "ecc_secp384r1", rcpt.expiry_ts); - if (result < 0) { - setLastError(network->getLastErrorStr(result)); - LOG_ERROR("{}", last_error); - return libcdoc::IO_ERROR; - } + if (result < 0) + FAIL(network->getLastErrorStr(result), result); LOG_DBG("Keyserver Id: {}", rcpt.server_id); LOG_DBG("Transaction Id: {}", cinfo.transaction_id); @@ -306,21 +278,15 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector kek_pm(libcdoc::CDoc2::KEY_LEN); std::vector salt; int64_t result = crypto->random(salt, libcdoc::CDoc2::KEY_LEN); - if (result < 0) { - setLastError(crypto->getLastErrorStr(result)); - return result; - } + if (result < 0) + FAIL(crypto->getLastErrorStr(result), result); std::vector pw_salt; result = crypto->random(pw_salt, libcdoc::CDoc2::KEY_LEN); - if (result < 0) { - setLastError(crypto->getLastErrorStr(result)); - return result; - } + if (result < 0) + FAIL(crypto->getLastErrorStr(result), result); result = crypto->extractHKDF(kek_pm, salt, pw_salt, rcpt.kdf_iter, rcpt_idx); - if (result < 0) { - setLastError(crypto->getLastErrorStr(result)); - return result; - } + if (result < 0) + FAIL(crypto->getLastErrorStr(result), result); std::vector kek = libcdoc::Crypto::expand(kek_pm, info_str, libcdoc::CDoc2::KEY_LEN); LOG_DBG("Label: {}", rcpt.label); @@ -332,11 +298,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector 0) { fb_rcpts.push_back(createPasswordCapsule(builder, rcpt, salt, pw_salt, xor_key)); } else { @@ -344,18 +307,12 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vectorgetValue(rcpt.server_id, libcdoc::Configuration::SHARE_SERVER_URLS); - if (url_list.empty()) { - setLastError("Missing server list for ID " + rcpt.server_id); - LOG_ERROR("{}", last_error); - return libcdoc::CONFIGURATION_ERROR; - } + if (url_list.empty()) + FAIL("Missing server list for ID " + rcpt.server_id, libcdoc::CONFIGURATION_ERROR); LOG_DBG("Share servers: {}", url_list); std::vector urls = libcdoc::JsonToStringArray(url_list); - if (urls.size() < 1) { - setLastError("No server URLs in " + rcpt.server_id); - LOG_ERROR("{}", last_error); - return libcdoc::CONFIGURATION_ERROR; - } + if (urls.size() < 1) + FAIL("No server URLs in " + rcpt.server_id, libcdoc::CONFIGURATION_ERROR); int N_SHARES = urls.size(); LOG_DBG("Number of shares: {}", N_SHARES); @@ -383,11 +340,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector kek = libcdoc::Crypto::expand(kek_pm, info_str); LOG_TRACE_KEY("kek: {}", kek); if (kek.empty()) return libcdoc::CRYPTO_ERROR; - if (libcdoc::Crypto::xor_data(xor_key, fmk, kek) != libcdoc::OK) { - setLastError("Internal error"); - LOG_ERROR("{}", last_error); - return libcdoc::CRYPTO_ERROR; - } + if (libcdoc::Crypto::xor_data(xor_key, fmk, kek) != libcdoc::OK) + FAIL("Internal error", libcdoc::CRYPTO_ERROR); // # Splitting KEK_i into shares // for j in (2, 3, ..., n): @@ -399,11 +353,8 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector& header, const std::vectorsendShare(transaction_ids[i], send_url, RecipientInfo_i, kek_shares[i]); - if (result < 0) { - setLastError(network->getLastErrorStr(result)); - LOG_ERROR("{}", last_error); - return libcdoc::IO_ERROR; - } + if (result < 0) + FAIL(network->getLastErrorStr(result), result); LOG_DBG("Share {} Transaction Id: {}", i, std::string((const char *) transaction_ids[i].data(), transaction_ids[i].size())); } std::vector> shares; @@ -441,9 +389,7 @@ CDoc2Writer::buildHeader(std::vector& header, const std::vector& header, const std::vector 8ULL * 1024 * 1024 * 1024) { - setLastError("Invalid file size"); - LOG_ERROR("{}", last_error); - return libcdoc::WRONG_ARGUMENTS; - } - if(auto rv = tar->open(name, size); rv < 0) { - setLastError(tar->getLastErrorStr(rv)); - LOG_ERROR("{}", last_error); - return rv; - } + if (finished) + FAIL("Encryption finished", libcdoc::WORKFLOW_ERROR); + if(!tar) + FAIL("Encryption not started", libcdoc::WORKFLOW_ERROR); + if (name.empty() || !libcdoc::isValidUtf8(name)) + FAIL("Invalid file name", libcdoc::DATA_FORMAT_ERROR); + if (size > 8ULL * 1024 * 1024 * 1024) + FAIL("Invalid file size", libcdoc::WRONG_ARGUMENTS); + if(auto rv = tar->open(name, size); rv < 0) + FAIL(tar->getLastErrorStr(rv), rv); return libcdoc::OK; } libcdoc::result_t CDoc2Writer::writeData(const uint8_t *src, size_t size) { - if (finished) { - setLastError("Encryption finished"); - LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } - if(!tar) { - setLastError("No file added"); - LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } - if(auto rv = tar->write(src, size); rv != size) { - setLastError(tar->getLastErrorStr(rv)); - return rv; - } + if (finished) + FAIL("Encryption finished", libcdoc::WORKFLOW_ERROR); + if(!tar) + FAIL("No file added", libcdoc::WORKFLOW_ERROR); + if(auto rv = tar->write(src, size); rv != size) + FAIL(tar->getLastErrorStr(rv), rv); return libcdoc::OK; } libcdoc::result_t CDoc2Writer::finishEncryption() { - if (finished) { - setLastError("Encryption finished"); - LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } - if(!tar) { - setLastError("No file added"); - LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } + if (finished) + FAIL("Encryption finished", libcdoc::WORKFLOW_ERROR); + if(!tar) + FAIL("No file added", libcdoc::WORKFLOW_ERROR); auto rv = tar->close(); if(rv < 0) setLastError(tar->getLastErrorStr(rv)); @@ -573,11 +492,8 @@ CDoc2Writer::finishEncryption() libcdoc::result_t CDoc2Writer::encrypt(libcdoc::MultiDataSource& src, const std::vector& keys) { - if (finished) { - setLastError("Encryption finished"); - LOG_ERROR("{}", last_error); - return libcdoc::WORKFLOW_ERROR; - } + if (finished) + FAIL("Encryption finished", libcdoc::WORKFLOW_ERROR); for (auto rcpt : keys) { if(auto rv = addRecipient(rcpt); rv != libcdoc::OK) return rv; @@ -600,3 +516,11 @@ CDoc2Writer::encrypt(libcdoc::MultiDataSource& src, const std::vector& keys) final; + result_t encrypt(MultiDataSource& src, const std::vector& keys) final; private: - libcdoc::result_t writeHeader(const std::vector &recipients); - libcdoc::result_t buildHeader(std::vector& header, const std::vector& keys, const std::vector& fmk); + result_t writeHeader(const std::vector &recipients); + result_t buildHeader(std::vector& header, const std::vector& keys, const std::vector& fmk); + result_t fail(const std::string& message, result_t result); - std::unique_ptr tar; - std::vector recipients; + std::unique_ptr tar; + std::vector recipients; bool finished = false; }; diff --git a/cdoc/CDocCipher.cpp b/cdoc/CDocCipher.cpp index 31c728e8..0d49a38e 100644 --- a/cdoc/CDocCipher.cpp +++ b/cdoc/CDocCipher.cpp @@ -277,6 +277,8 @@ fill_recipients_from_rcpt_info(ToolConf& conf, ToolCrypto& crypto, std::vector val; bool rsa; @@ -315,6 +319,8 @@ fill_recipients_from_rcpt_info(ToolConf& conf, ToolCrypto& crypto, std::vector extra) const return ofs.str(); } +bool +Recipient::validate() const +{ + switch(type) { + case SYMMETRIC_KEY: + // Either user-defined label or LABEL property is required + return !label.empty() || lbl_parts.contains("CDoc2::Label::LABEL"); + break; + case PUBLIC_KEY: + // Public key should not be empty + if (rcpt_key.empty()) + return false; + break; + default: + return false; + } + return true; +} + } // namespace libcdoc diff --git a/cdoc/Recipient.h b/cdoc/Recipient.h index fb752862..3844f0ee 100644 --- a/cdoc/Recipient.h +++ b/cdoc/Recipient.h @@ -238,6 +238,13 @@ struct CDOC_EXPORT Recipient { lbl_parts[std::string(key)] = value; } + /** + * @brief Validate recipient record + * + * @return true if Recipient is valid + */ + bool validate() const; + bool operator== (const Recipient& other) const = default; protected: Recipient(Type _type) : type(_type) {}; diff --git a/test/libcdoc_boost.cpp b/test/libcdoc_boost.cpp index 5ce21e9e..dc332a98 100644 --- a/test/libcdoc_boost.cpp +++ b/test/libcdoc_boost.cpp @@ -540,7 +540,7 @@ BOOST_FIXTURE_TEST_CASE_WITH_DECOR(EncryptWithPasswordWithoutLabel, EncryptFixtu * utf::description("Encrypting a file with password and without label")) { std::vector rcpts { - {libcdoc::RcptInfo::PASSWORD, {}, {}, std::vector(Password.cbegin(), Password.cend())} + {libcdoc::RcptInfo::PASSWORD, "auto", {}, std::vector(Password.cbegin(), Password.cend())} }; encrypt(2, {checkDataFile(sources[0])}, formTargetFile("PasswordUsageWithoutLabel.cdoc"), rcpts); } @@ -559,7 +559,7 @@ BOOST_FIXTURE_TEST_CASE_WITH_DECOR(EncryptWithAESKey, EncryptFixture, * utf::description("Encrypting a file with symmetric AES key")) { std::vector rcpts { - {libcdoc::RcptInfo::SKEY, {}, {}, libcdoc::fromHex(AESKey)} + {libcdoc::RcptInfo::SKEY, "AES", {}, libcdoc::fromHex(AESKey)} }; encrypt(2, {checkDataFile(sources[0])}, formTargetFile("AESKeyUsage.cdoc"), rcpts); } From afaaa7aceabc6149cb62046eaa5c5567bbf08e84 Mon Sep 17 00:00:00 2001 From: lauris71 Date: Wed, 1 Apr 2026 09:43:44 +0300 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Raul Metsma --- cdoc/Recipient.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cdoc/Recipient.cpp b/cdoc/Recipient.cpp index c7346df8..9d0cdc7f 100644 --- a/cdoc/Recipient.cpp +++ b/cdoc/Recipient.cpp @@ -212,12 +212,9 @@ Recipient::validate() const case SYMMETRIC_KEY: // Either user-defined label or LABEL property is required return !label.empty() || lbl_parts.contains("CDoc2::Label::LABEL"); - break; case PUBLIC_KEY: // Public key should not be empty - if (rcpt_key.empty()) - return false; - break; + return !rcpt_key.empty(); default: return false; }