Make PIN erasing more secure

WE2-479

Signed-off-by: Mart Somermaa <[email protected]>

src/controller/CMakeLists.txt

@@ -31,6 +31,7 @@ add_library(controller STATIC
     threads/controllerchildthread.hpp
     threads/waitforcardthread.hpp
     ui.hpp
+    utils/erasedata.hpp
     utils/observer_ptr.hpp
     utils/qdisablecopymove.hpp
     utils/utils.hpp

src/controller/command-handlers/authenticate.cpp

@@ -124,7 +124,7 @@ QVariantMap Authenticate::onConfirm(WebEidUI* window,
         QString::fromStdString(cardCertAndPin.cardInfo->eid().authSignatureAlgorithm());
 
     pcsc_cpp::byte_vector pin;
-    getPin(pin, cardCertAndPin.cardInfo->eid().smartcard(), window);
+    getPin(pin, cardCertAndPin.cardInfo->eid(), window);
     auto pin_cleanup = qScopeGuard([&pin] {
         // Erase PIN memory.
         std::fill(pin.begin(), pin.end(), '\0');

src/controller/command-handlers/sign.cpp

@@ -98,7 +98,7 @@ 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().smartcard(), window);
+    getPin(pin, cardCertAndPin.cardInfo->eid(), window);
     auto pin_cleanup = qScopeGuard([&pin] {
         // Erase PIN memory.
         std::fill(pin.begin(), pin.end(), '\0');

src/controller/command-handlers/signauthutils.cpp

@@ -20,6 +20,7 @@
  * SOFTWARE.
  */
 
+#include "utils/erasedata.hpp"
 #include "signauthutils.hpp"
 
 #include "ui.hpp"
@@ -66,39 +67,28 @@ template QString validateAndGetArgument<QString>(const QString& argName, const Q
 template QByteArray validateAndGetArgument<QByteArray>(const QString& argName,
                                                        const QVariantMap& args, bool allowNull);
 
-template <typename T, typename C>
-inline void eraseData(T& data)
+void getPin(pcsc_cpp::byte_vector& pin, const ElectronicID& eid, WebEidUI* window)
 {
-    // According to docs, constData() never causes a deep copy to occur, so we can abuse it
-    // to overwrite the underlying buffer since the underlying data is not really const.
-    C* chars = const_cast<C*>(data.constData());
-    for (int i = 0; i < data.length(); ++i) {
-        chars[i] = '\0';
-    }
-}
-
-int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, WebEidUI* window)
-{
-    // Doesn't apply to PIN pads.
-    if (card.readerHasPinPad()) {
-        return 0;
+    // If the reader has a PIN pad or when enternal PIN dialog is used, do nothing.
+    if (eid.smartcard().readerHasPinPad() || eid.providesExternalPinDialog()) {
+        return;
     }
 
     REQUIRE_NON_NULL(window)
 
-    QString pinqs = window->getPin();
-    if (pinqs.isEmpty()) {
+    QString pinQStr = window->getPin();
+    if (pinQStr.isEmpty()) {
         THROW(ProgrammingError, "Empty PIN");
     }
-    int len = (int)pinqs.length();
 
+    uint len = pinQStr.length();
     pin.resize(len);
-    for (int i = 0; i < len; i++) {
-        pin[i] = pinqs[i].cell();
+
+    for (uint i = 0; i < len; i++) {
+        pin[i] = pinQStr[i].unicode() & 0xff;
     }
-    eraseData<QString, QChar>(pinqs);
 
-    return len;
+    eraseData(pinQStr);
 }
 
 QVariantMap signatureAlgoToVariantMap(const SignatureAlgorithm signatureAlgo)

src/controller/command-handlers/signauthutils.hpp

@@ -45,6 +45,6 @@ extern template QString validateAndGetArgument<QString>(const QString& argName,
 extern template QByteArray
 validateAndGetArgument<QByteArray>(const QString& argName, const QVariantMap& args, bool allowNull);
 
-int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, WebEidUI* window);
+void getPin(pcsc_cpp::byte_vector& pin, const electronic_id::ElectronicID& eid, WebEidUI* window);
 
 QVariantMap signatureAlgoToVariantMap(const electronic_id::SignatureAlgorithm signatureAlgo);

src/controller/utils/erasedata.hpp

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2020-2024 Estonian Information System Authority
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#pragma once
+
+#include <QString>
+
+inline void eraseData(QString& data)
+{
+    // According to docs, constData() never causes a deep copy to occur, so we can abuse it
+    // to overwrite the underlying buffer since the underlying data is not really const.
+    QChar* chars = const_cast<QChar*>(data.constData());
+    for (int i = 0; i < data.length(); ++i) {
+        chars[i] = '\0';
+    }
+}

src/ui/webeiddialog.cpp

@@ -20,6 +20,8 @@
  * SOFTWARE.
  */
 
+#include "utils/erasedata.hpp"
+
 #include "webeiddialog.hpp"
 #include "application.hpp"
 #include "punycode.hpp"
@@ -137,7 +139,8 @@ WebEidDialog::WebEidDialog(QWidget* parent) : WebEidUI(parent), ui(new Private)
             ++i;
         }
         menu->show();
-        menu->move(ui->langButton->geometry().bottomRight() - menu->geometry().topRight() + QPoint(0, 2));
+        menu->move(ui->langButton->geometry().bottomRight() - menu->geometry().topRight()
+                   + QPoint(0, 2));
         connect(langGroup, qOverload<QAbstractButton*>(&QButtonGroup::buttonClicked), menu,
                 [this, menu](QAbstractButton* action) {
                     QSettings().setValue(QStringLiteral("lang"), action->property("lang"));
@@ -289,6 +292,7 @@ QString WebEidDialog::getPin()
     // QString uses QAtomicPointer internally and is thread-safe.
     // There should be only single reference and this is transferred to the caller for safety
     QString ret = pin;
+    eraseData(pin);
     pin.clear();
     return ret;
 }