Take PIN ownership to minimze memory copy-s

followup WE2-479 Signed-off-by: Raul Metsma <[email protected]>
web-eid · Oct 16, 2024 · 97d14a1 · 97d14a1
1 parent b8d6e7f
commit 97d14a1
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 27 deletions.
diff --git a/lib/libelectronic-id b/lib/libelectronic-id
diff --git a/src/controller/command-handlers/authenticate.cpp b/src/controller/command-handlers/authenticate.cpp
@@ -57,7 +57,7 @@ QVariantMap createAuthenticationToken(const QString& signatureAlgorithm,
 }
 
 QByteArray createSignature(const QString& origin, const QString& challengeNonce,
-                           const ElectronicID& eid, const pcsc_cpp::byte_vector& pin)
+                           const ElectronicID& eid, pcsc_cpp::byte_vector&& pin)
 {
     static const std::map<JsonWebSignatureAlgorithm, QCryptographicHash::Algorithm>
         SIGNATURE_ALGO_TO_HASH {
@@ -86,7 +86,7 @@ QByteArray createSignature(const QString& origin, const QString& challengeNonce,
     const pcsc_cpp::byte_vector hashToBeSigned {hashToBeSignedQBytearray.cbegin(),
                                                 hashToBeSignedQBytearray.cend()};
 
-    const auto signature = eid.signWithAuthKey(pin, hashToBeSigned);
+    const auto signature = eid.signWithAuthKey(std::move(pin), hashToBeSigned);
 
     return QByteArray::fromRawData(reinterpret_cast<const char*>(signature.data()),
                                    int(signature.size()))
@@ -120,20 +120,14 @@ Authenticate::Authenticate(const CommandWithArguments& cmd) : CertificateReader(
 QVariantMap Authenticate::onConfirm(WebEidUI* window,
                                     const CardCertificateAndPinInfo& cardCertAndPin)
 {
-    const auto signatureAlgorithm =
-        QString::fromStdString(cardCertAndPin.cardInfo->eid().authSignatureAlgorithm());
-
-    pcsc_cpp::byte_vector pin;
-    getPin(pin, cardCertAndPin.cardInfo->eid(), window);
-    auto pin_cleanup = qScopeGuard([&pin] {
-        // Erase PIN memory.
-        std::fill(pin.begin(), pin.end(), '\0');
-    });
-
     try {
+        const auto signatureAlgorithm =
+            QString::fromStdString(cardCertAndPin.cardInfo->eid().authSignatureAlgorithm());
+        pcsc_cpp::byte_vector pin;
+        pin.reserve(5 + 16); // Avoid realloc: apdu + pin padding
+        getPin(pin, cardCertAndPin.cardInfo->eid(), window);
         const auto signature =
-            createSignature(origin.url(), challengeNonce, cardCertAndPin.cardInfo->eid(), pin);
-
+            createSignature(origin.url(), challengeNonce, cardCertAndPin.cardInfo->eid(), std::move(pin));
         return createAuthenticationToken(signatureAlgorithm, cardCertAndPin.certificateBytesInDer,
                                          signature);
 

diff --git a/src/controller/command-handlers/sign.cpp b/src/controller/command-handlers/sign.cpp
@@ -32,11 +32,11 @@ using namespace electronic_id;
 namespace
 {
 
-QPair<QString, QVariantMap> signHash(const ElectronicID& eid, const pcsc_cpp::byte_vector& pin,
+QPair<QString, QVariantMap> signHash(const ElectronicID& eid, pcsc_cpp::byte_vector&& pin,
                                      const QByteArray& docHash, const HashAlgorithm hashAlgo)
 {
     const auto hashBytes = pcsc_cpp::byte_vector {docHash.begin(), docHash.end()};
-    const auto signature = eid.signWithSigningKey(pin, hashBytes, hashAlgo);
+    const auto signature = eid.signWithSigningKey(std::move(pin), hashBytes, hashAlgo);
 
     const auto signatureBase64 =
         QByteArray::fromRawData(reinterpret_cast<const char*>(signature.first.data()),
@@ -97,16 +97,11 @@ void Sign::emitCertificatesReady(const std::vector<CardCertificateAndPinInfo>& c
 
 QVariantMap Sign::onConfirm(WebEidUI* window, const CardCertificateAndPinInfo& cardCertAndPin)
 {
-    pcsc_cpp::byte_vector pin;
-    getPin(pin, cardCertAndPin.cardInfo->eid(), window);
-    auto pin_cleanup = qScopeGuard([&pin] {
-        // Erase PIN memory.
-        std::fill(pin.begin(), pin.end(), '\0');
-    });
-
     try {
-        const auto signature = signHash(cardCertAndPin.cardInfo->eid(), pin, docHash, hashAlgo);
-
+        pcsc_cpp::byte_vector pin;
+        pin.reserve(5 + 16); // Avoid realloc: apdu + pin padding
+        getPin(pin, cardCertAndPin.cardInfo->eid(), window);
+        const auto signature = signHash(cardCertAndPin.cardInfo->eid(), std::move(pin), docHash, hashAlgo);
         return {{QStringLiteral("signature"), signature.first},
                 {QStringLiteral("signatureAlgorithm"), signature.second}};
 
@@ -144,7 +139,7 @@ void Sign::validateAndStoreDocHashAndHashAlgo(const QVariantMap& args)
     docHash =
         QByteArray::fromBase64(validateAndGetArgument<QByteArray>(QStringLiteral("hash"), args));
 
-    QString hashAlgoInput = validateAndGetArgument<QString>(QStringLiteral("hashFunction"), args);
+    auto hashAlgoInput = validateAndGetArgument<QString>(QStringLiteral("hashFunction"), args);
     if (hashAlgoInput.size() > 8) {
         THROW(CommandHandlerInputDataError, "hashFunction value is invalid");
     }
+2 −2		include/electronic-id/electronic-id.hpp
+2 −2		src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.cpp
+2 −2		src/electronic-ids/ms-cryptoapi/MsCryptoApiElectronicID.hpp
+6 −6		src/electronic-ids/pcsc/EIDIDEMIA.cpp
+2 −2		src/electronic-ids/pcsc/EIDIDEMIA.hpp
+12 −15		src/electronic-ids/pcsc/FinEID.cpp
+5 −5		src/electronic-ids/pcsc/FinEID.hpp
+8 −12		src/electronic-ids/pcsc/PcscElectronicID.hpp
+6 −7		src/electronic-ids/pcsc/pcsc-common.hpp
+2 −2		src/electronic-ids/pkcs11/Pkcs11ElectronicID.cpp
+2 −2		src/electronic-ids/pkcs11/Pkcs11ElectronicID.hpp
+3 −3		tests/integration/test-authenticate.cpp
+1 −1		tests/integration/test-signing.cpp
+25 −20		tests/mock/test-get-certificate.cpp