From 1db3ae43aa63b55ca738e14a79d294848e31ee0c Mon Sep 17 00:00:00 2001 From: Raul Metsma Date: Mon, 8 Jun 2026 13:36:34 +0300 Subject: [PATCH] Clarify intent for static analysis WE2-1248 Signed-off-by: Raul Metsma --- lib/libpcsc-cpp/README.md | 6 +-- lib/libpcsc-cpp/src/SmartCard.cpp | 5 +++ src/electronic-ids/TLV.hpp | 8 +++- src/electronic-ids/common.cpp | 8 ++-- .../ms-cryptoapi/MsCryptoApiElectronicID.cpp | 40 ++++++++++--------- .../listMsCryptoApiElectronicIDs.cpp | 13 ++++-- src/electronic-ids/pcsc/EIDIDEMIA.cpp | 10 +++-- .../pkcs11/PKCS11CardManager.hpp | 22 +++++++--- .../pkcs11/Pkcs11ElectronicID.cpp | 5 +++ 9 files changed, 80 insertions(+), 37 deletions(-) diff --git a/lib/libpcsc-cpp/README.md b/lib/libpcsc-cpp/README.md index 33329fc3..6aa98885 100644 --- a/lib/libpcsc-cpp/README.md +++ b/lib/libpcsc-cpp/README.md @@ -11,10 +11,10 @@ reader and transmit an APDU: auto readers = listReaders(); auto card = readers[0].connectToCard(); - auto command = CommandApdu::fromBytes({0x2, 0x1, 0x3, 0x4}); + auto command = CommandApdu(0x2, 0x1, 0x3, 0x4); - auto transactionGuard = card->beginTransaction(); - auto response = card->transmit(command); + auto session = card.beginSession(); + auto response = session.transmit(command); See more examples in [tests](tests). diff --git a/lib/libpcsc-cpp/src/SmartCard.cpp b/lib/libpcsc-cpp/src/SmartCard.cpp index c3a2aade..41797fd4 100644 --- a/lib/libpcsc-cpp/src/SmartCard.cpp +++ b/lib/libpcsc-cpp/src/SmartCard.cpp @@ -127,6 +127,11 @@ class CardImpl if (id_vendor == VENDOR_HID_GLOBAL && (id_product == OMNIKEY_3x21 || id_product == OMNIKEY_6121)) return false; + // Debug/workaround override: forces host-side PIN entry instead of the secure + // PIN-pad keypad. Moving PIN entry to the host process means the PIN passes + // through host memory and is no longer isolated on the reader hardware. Use only + // for testing or when a reader falsely reports PIN-pad capability but does not + // implement it correctly. if (getenv("SMARTCARDPP_NOPINPAD")) return false; return feature(FEATURE_VERIFY_PIN_START) != features.cend() diff --git a/src/electronic-ids/TLV.hpp b/src/electronic-ids/TLV.hpp index e39a6539..09d05ecc 100644 --- a/src/electronic-ids/TLV.hpp +++ b/src/electronic-ids/TLV.hpp @@ -34,6 +34,12 @@ namespace electronic_id * The constructor parses the tag and length from the provided byte range, * then adjusts its iterators so that `begin` and `end` reference only the value bytes. * If the TLV is empty, `operator bool()` returns false. + * + * Tag encoding: the `tag` field stores the raw BER-TLV tag bytes packed into a uint32 + * (first byte in the most-significant position), including continuation-bit bytes for + * multi-byte tags. This is NOT the logical BER-TLV tag number from ISO 7816. All + * hardcoded tag literals used with `find()` follow the same packed-byte convention, + * e.g. a two-byte tag 0xBF 0x81 0x00 is stored as 0xBF8100. */ struct TLV { @@ -105,7 +111,7 @@ struct TLV PCSC_CPP_CONSTEXPR_VECTOR TLV find(uint32_t find) const { TLV tlv = *this; - for (; tlv && tlv.tag != find; ++tlv) {} + for (; tlv && tlv.tag != find; ++tlv) { } // Return the found TLV or an empty one if not found return tlv; } diff --git a/src/electronic-ids/common.cpp b/src/electronic-ids/common.cpp index c8a75df5..3ca49982 100644 --- a/src/electronic-ids/common.cpp +++ b/src/electronic-ids/common.cpp @@ -37,10 +37,12 @@ const std::set& ELLIPTIC_CURVE_SIGNATURE_ALGOS() const std::set& RSA_SIGNATURE_ALGOS() { + // SHA3+RSA (RS3_*, PS3_*) omitted: no card implementation (PCSC or PKCS#11) supports it. const static std::set RS_ALGOS = { - SignatureAlgorithm::RS224, SignatureAlgorithm::RS256, SignatureAlgorithm::RS384, - SignatureAlgorithm::RS512, SignatureAlgorithm::RS3_224, SignatureAlgorithm::RS3_256, - SignatureAlgorithm::RS3_384, SignatureAlgorithm::RS3_512, + SignatureAlgorithm::RS224, + SignatureAlgorithm::RS256, + SignatureAlgorithm::RS384, + SignatureAlgorithm::RS512, }; return RS_ALGOS; } diff --git a/src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.cpp b/src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.cpp index a513a783..be0693b1 100644 --- a/src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.cpp +++ b/src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.cpp @@ -76,23 +76,27 @@ ElectronicID::Signature MsCryptoApiElectronicID::sign(const byte_vector& hash, { bool isRSA = signatureAlgo != SignatureAlgorithm::ES; BCRYPT_PKCS1_PADDING_INFO padInfo {}; - switch (hashAlgo) { - case HashAlgorithm::SHA224: - padInfo.pszAlgId = L"SHA224"; - break; - case HashAlgorithm::SHA256: - padInfo.pszAlgId = NCRYPT_SHA256_ALGORITHM; - break; - case HashAlgorithm::SHA384: - padInfo.pszAlgId = NCRYPT_SHA384_ALGORITHM; - break; - case HashAlgorithm::SHA512: - padInfo.pszAlgId = NCRYPT_SHA512_ALGORITHM; - break; - default: - // FIXME: what about HashAlgorithm::SHA3_*? - THROW(ArgumentFatalError, - "Hash algorithm " + std::to_string(hashAlgo) + " is not supported"); + if (isRSA) { + // EC signing passes raw hash bytes to NCryptSignHash without a padding descriptor, + // so any hash including SHA3 works. RSA requires a padding descriptor with a hash + // algorithm ID; SHA3+RSA is not supported by NCrypt (SHA3 is BCrypt-only). + switch (hashAlgo) { + case HashAlgorithm::SHA224: + padInfo.pszAlgId = L"SHA224"; + break; + case HashAlgorithm::SHA256: + padInfo.pszAlgId = NCRYPT_SHA256_ALGORITHM; + break; + case HashAlgorithm::SHA384: + padInfo.pszAlgId = NCRYPT_SHA384_ALGORITHM; + break; + case HashAlgorithm::SHA512: + padInfo.pszAlgId = NCRYPT_SHA512_ALGORITHM; + break; + default: + THROW(ArgumentFatalError, + "Hash algorithm " + std::to_string(hashAlgo) + " is not supported for RSA"); + } } DWORD size = 0; @@ -125,7 +129,7 @@ ElectronicID::Signature MsCryptoApiElectronicID::sign(const byte_vector& hash, THROW(MsCryptoApiError, "Signing failed with error: " + std::to_string(err)); } - return {signature, {signatureAlgo, hashAlgo}}; + return {std::move(signature), {signatureAlgo, hashAlgo}}; } } // namespace electronic_id diff --git a/src/electronic-ids/ms-cryptoapi/listMsCryptoApiElectronicIDs.cpp b/src/electronic-ids/ms-cryptoapi/listMsCryptoApiElectronicIDs.cpp index 0c643596..007e11b6 100644 --- a/src/electronic-ids/ms-cryptoapi/listMsCryptoApiElectronicIDs.cpp +++ b/src/electronic-ids/ms-cryptoapi/listMsCryptoApiElectronicIDs.cpp @@ -29,6 +29,9 @@ #include "../scope.hpp" #include "../x509.hpp" +#include +#include + using namespace std::string_literals; namespace electronic_id @@ -108,13 +111,15 @@ std::vector listMsCryptoApiElectronicIDs() continue; // TODO: log. } - std::wstring algo(5, 0); - err = NCryptGetProperty(key, NCRYPT_ALGORITHM_GROUP_PROPERTY, PBYTE(algo.data()), - DWORD(algo.size() + 1) * 2, &size, 0); + // Fixed-size buffer is intentional: only "RSA" (3) and "ECDSA" (5) are supported. + // 6 wchars = 5 chars + null terminator, fits all supported algorithm group names. + std::array algoBuf {}; + err = NCryptGetProperty(key, NCRYPT_ALGORITHM_GROUP_PROPERTY, PBYTE(algoBuf.data()), + DWORD(algoBuf.size()) * 2, &size, 0); if (FAILED(err)) { continue; // TODO: log. } - algo.resize(size / 2 - 1); + std::wstring_view algo(algoBuf.data()); if (algo != L"RSA" && !algo.starts_with(L"EC")) { // We only support RSA and ECC algorithms. continue; // TODO: log. diff --git a/src/electronic-ids/pcsc/EIDIDEMIA.cpp b/src/electronic-ids/pcsc/EIDIDEMIA.cpp index 01af5e1f..86863477 100644 --- a/src/electronic-ids/pcsc/EIDIDEMIA.cpp +++ b/src/electronic-ids/pcsc/EIDIDEMIA.cpp @@ -130,13 +130,17 @@ ElectronicID::Signature EIDIDEMIA::signWithSigningKeyImpl(const SmartCard::Sessi PIN_PADDING_CHAR); auto tmp = hash; if (isECC) { - // https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf 6.4 + // FIPS 186-5 §6.4: when signing with an n-bit key, if the hash output is longer + // than n bits, take the leftmost n bits; if shorter, the hash integer value is + // used as-is (leading zeros are implicit). We normalise to 48 bytes to match the + // P-384 key used on these cards. + // https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf constexpr size_t ECDSA384_INPUT_LENGTH = 384 / 8; if (tmp.size() < ECDSA384_INPUT_LENGTH) { - // Zero-pad hashes that are shorter than SHA-384. + // Prepend zeros — mathematically equivalent to the original integer value. tmp.insert(tmp.cbegin(), ECDSA384_INPUT_LENGTH - tmp.size(), 0x00); } else if (tmp.size() > ECDSA384_INPUT_LENGTH) { - // Truncate hashes that are longer than SHA-384. + // Keep the leftmost 48 bytes per FIPS 186-5 §6.4. tmp.resize(ECDSA384_INPUT_LENGTH); } } diff --git a/src/electronic-ids/pkcs11/PKCS11CardManager.hpp b/src/electronic-ids/pkcs11/PKCS11CardManager.hpp index abc209b9..73ed2704 100644 --- a/src/electronic-ids/pkcs11/PKCS11CardManager.hpp +++ b/src/electronic-ids/pkcs11/PKCS11CardManager.hpp @@ -80,8 +80,10 @@ class PKCS11CardManager static std::unordered_map> instances; // There is no std::hash for std::filesystem::path, use the string value. - // Note that two different path strings that refer to the same filesystem location - // will be treated as different keys (e.g. /path/to/module and /path/to/../to/module). + // Known limitation: two path strings that resolve to the same file (e.g. + // /path/to/module and /path/to/../to/module) are treated as different keys and + // would load the module twice. This is safe in practice because all paths come + // from hardcoded constants in Pkcs11ElectronicID.cpp and are never user-supplied. std::string moduleStr = module.string(); std::lock_guard lock(mutex); @@ -191,6 +193,9 @@ class PKCS11CardManager // If the module provides an external PIN dialog, login is not required. if (!providesExternalPinDialog) { try { + // The PIN buffer is cleared by the caller after C_Login returns. Whether + // the PKCS#11 module retains a copy of the PIN internally after this call + // is outside our control and is the module vendor's responsibility. C(Login, session, CKU_USER, CK_CHAR_PTR(pin), CK_ULONG(pinSize)); } catch (const VerifyPinFailed& e) { if (e.status() != VerifyPinFailed::Status::RETRY_ALLOWED) @@ -247,6 +252,11 @@ class PKCS11CardManager } private: + // Trust model: the module loaded here is a third-party vendor library installed by the + // OS administrator or card middleware package. Its integrity is gated by OS filesystem + // permissions on the hardcoded paths — not by web-eid code-signing. Every vendor + // module loaded this way runs inside the web-eid process and has access to PIN material + // and signing operations. This is an explicit trust delegation to the middleware vendor. PKCS11CardManager(const std::filesystem::path& module) { CK_C_GetFunctionList C_GetFunctionList = nullptr; @@ -364,9 +374,11 @@ class PKCS11CardManager static constexpr uint8_t pinRetryCount(CK_FLAGS flags) noexcept { - // As PKCS#11 does not provide an API for querying remaining PIN retries, we currently - // simply assume max retry count of 3, which is quite common. We might need to revisit this - // in the future once it becomes a problem. + // PKCS#11 does not expose the exact remaining retry count — only flag buckets + // (locked / final-try / count-low). We map these to 0/1/2 and assume a maximum + // of 3, which is the most common value. Cards with a higher retry budget (e.g. + // FinEID v3 allows 5) will show "3 attempts left" in the UI when the actual count + // is higher. This is a known PKCS#11 protocol limitation with no workaround. if (flags & CKF_USER_PIN_LOCKED) { return 0; } diff --git a/src/electronic-ids/pkcs11/Pkcs11ElectronicID.cpp b/src/electronic-ids/pkcs11/Pkcs11ElectronicID.cpp index b47631c7..91f4a1e8 100644 --- a/src/electronic-ids/pkcs11/Pkcs11ElectronicID.cpp +++ b/src/electronic-ids/pkcs11/Pkcs11ElectronicID.cpp @@ -224,6 +224,11 @@ Pkcs11ElectronicID::Pkcs11ElectronicID(ElectronicID::Type type) : { REQUIRE_NON_NULL(manager) + // Known limitation: if the PKCS#11 module exposes more than one authentication or + // signing token (e.g. multiple certificates on one card or multiple cards in multiple + // slots), only the last token of each type is used. Surfacing multiple tokens requires + // a larger refactor of the card-detection pipeline to return a vector of ElectronicIDs + // instead of a single one. for (const auto& token : manager->tokens()) { const auto certType = certificateType(token.cert); if (certType.isAuthentication()) {