Resolve fixme-s in code (#303)

* Resolved/removed fixme-s in code

Signed-off-by: Lauris Kaplinski <[email protected]>

* Make PIN erasing more secure

WE2-479

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

---------

Signed-off-by: Lauris Kaplinski <[email protected]>
Signed-off-by: Mart Somermaa <[email protected]>
Co-authored-by: Lauris Kaplinski <[email protected]>
Co-authored-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

@@ -30,6 +30,7 @@
 #include <QJsonDocument>
 #include <QCryptographicHash>
 #include <QDir>
+#include <QScopeGuard>
 
 #include <map>
 
@@ -122,17 +123,17 @@ QVariantMap Authenticate::onConfirm(WebEidUI* window,
     const auto signatureAlgorithm =
         QString::fromStdString(cardCertAndPin.cardInfo->eid().authSignatureAlgorithm());
 
-    auto pin = getPin(cardCertAndPin.cardInfo->eid(), window);
+    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 =
             createSignature(origin.url(), challengeNonce, cardCertAndPin.cardInfo->eid(), pin);
 
-        // Erase the PIN memory.
-        // TODO: Use a scope guard. Verify that the buffers are actually zeroed and no copies
-        // remain.
-        std::fill(pin.begin(), pin.end(), '\0');
-
         return createAuthenticationToken(signatureAlgorithm, cardCertAndPin.certificateBytesInDer,
                                          signature);
 

src/controller/command-handlers/sign.cpp

@@ -25,6 +25,8 @@
 #include "signauthutils.hpp"
 #include "utils/utils.hpp"
 
+#include <QScopeGuard>
+
 using namespace electronic_id;
 
 namespace
@@ -95,16 +97,16 @@ void Sign::emitCertificatesReady(const std::vector<CardCertificateAndPinInfo>& c
 
 QVariantMap Sign::onConfirm(WebEidUI* window, const CardCertificateAndPinInfo& cardCertAndPin)
 {
-    auto pin = getPin(cardCertAndPin.cardInfo->eid(), window);
+    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);
 
-        // Erase PIN memory.
-        // TODO: Use a scope guard. Verify that the buffers are actually zeroed
-        // and no copies remain.
-        std::fill(pin.begin(), pin.end(), '\0');
-
         return {{QStringLiteral("signature"), signature.first},
                 {QStringLiteral("signatureAlgorithm"), signature.second}};
 

src/controller/command-handlers/signauthutils.cpp

@@ -20,11 +20,11 @@
  * SOFTWARE.
  */
 
+#include "utils/erasedata.hpp"
 #include "signauthutils.hpp"
 
 #include "ui.hpp"
 #include "commandhandler.hpp"
-#include "utils/utils.hpp"
 
 using namespace electronic_id;
 
@@ -67,40 +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';
-    }
-}
-
-pcsc_cpp::byte_vector getPin(const ElectronicID& card, WebEidUI* window)
-{
-    // Doesn't apply to PIN pads.
-    if (card.smartcard().readerHasPinPad() || card.providesExternalPinDialog()) {
-        return {};
+    // 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)
 
-    auto pin = window->getPin();
-    if (pin.isEmpty()) {
+    QString pinQStr = window->getPin();
+    if (pinQStr.isEmpty()) {
         THROW(ProgrammingError, "Empty PIN");
     }
 
-    // TODO: Avoid making copies of the PIN in memory.
-    auto pinQByteArray = pin.toUtf8();
-    pcsc_cpp::byte_vector pinBytes {pinQByteArray.begin(), pinQByteArray.end()};
+    uint len = pinQStr.length();
+    pin.resize(len);
 
-    // TODO: Verify that the buffers are actually zeroed and no copies remain.
-    eraseData<QString, QChar>(pin);
-    eraseData<QByteArray, char>(pinQByteArray);
+    for (uint i = 0; i < len; i++) {
+        pin[i] = pinQStr[i].unicode() & 0xff;
+    }
 
-    return pinBytes;
+    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);
 
-pcsc_cpp::byte_vector getPin(const electronic_id::ElectronicID& 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/controller.cpp

@@ -101,6 +101,15 @@ void Controller::run()
 
         startCommandExecution();
 
+    } catch (const std::invalid_argument& exc) {
+        if (isInStdinMode) {
+            // Pass invalid argument message to the caller just in case it may be interested
+            // The result will be {"invalid-argument" : message}
+            // Command parameter is only used if exception will be raised during json creation
+            writeResponseToStdOut(isInStdinMode, {{QStringLiteral("invalid-argument"), exc.what()}},
+                                  "invalid-argument");
+        }
+        onCriticalFailure(exc.what());
     } catch (const std::exception& error) {
         onCriticalFailure(error.what());
     }

src/controller/inputoutputmode.cpp

@@ -71,7 +71,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto messageLength = readMessageLength(std::cin);
 
     if (messageLength < 5) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Message length is "
                                     + std::to_string(messageLength) + ", at least 5 required");
     }
@@ -88,7 +87,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto json = QJsonDocument::fromJson(message);
 
     if (!json.isObject()) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Invalid JSON, not an object");
     }
 
@@ -97,7 +95,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto arguments = jsonObject["arguments"];
 
     if (!command.isString() || !arguments.isObject()) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Invalid JSON, the main object does not "
                                     "contain a 'command' string and 'arguments' object");
     }

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"));
@@ -287,7 +290,11 @@ QString WebEidDialog::getPin()
 {
     // getPin() is called from background threads and must be thread-safe.
     // QString uses QAtomicPointer internally and is thread-safe.
-    return pin;
+    // There should be only single reference and this is transferred to the caller for safety
+    QString ret = pin;
+    eraseData(pin);
+    pin.clear();
+    return ret;
 }
 
 void WebEidDialog::onSmartCardStatusUpdate(const RetriableError status)
@@ -335,7 +342,8 @@ void WebEidDialog::onMultipleCertificatesReady(
         ui->selectAnotherCertificate->setVisible(certificateAndPinInfos.size() > 1);
         connect(ui->selectAnotherCertificate, &QPushButton::clicked, this,
                 [this, origin, certificateAndPinInfos] {
-                    ui->pinInput->clear();
+                    // We set pinInput to empty text instead of clear() to also reset undo buffer
+                    ui->pinInput->setText({});
                     onMultipleCertificatesReady(origin, certificateAndPinInfos);
                 });
         setupOK([this, origin] {
@@ -446,7 +454,6 @@ void WebEidDialog::onVerifyPinFailed(const VerifyPinFailed::Status status, const
 
     std::function<QString()> message;
 
-    // FIXME: don't allow retry in case of UNKNOWN_ERROR
     switch (status) {
     case Status::RETRY_ALLOWED:
         message = [retriesLeft]() -> QString {
@@ -469,8 +476,11 @@ void WebEidDialog::onVerifyPinFailed(const VerifyPinFailed::Status status, const
         message = [] { return tr("PIN entry cancelled."); };
         break;
     case Status::PIN_ENTRY_DISABLED:
+        message = [] { return tr("PIN entry disabled"); };
+        break;
     case Status::UNKNOWN_ERROR:
         message = [] { return tr("Technical error"); };
+        displayFatalError(message);
         break;
     }
 
@@ -684,6 +694,20 @@ void WebEidDialog::displayPinBlockedError()
     ui->helpButton->show();
 }
 
+void WebEidDialog::displayFatalError(std::function<QString()> message)
+{
+    ui->pinTitleLabel->hide();
+    ui->pinInput->hide();
+    ui->pinTimeoutTimer->stop();
+    ui->pinTimeRemaining->hide();
+    ui->pinEntryTimeoutProgressBar->hide();
+    setTrText(ui->pinErrorLabel, message);
+    ui->pinErrorLabel->show();
+    ui->okButton->hide();
+    ui->cancelButton->setEnabled(true);
+    ui->cancelButton->show();
+}
+
 void WebEidDialog::showPinInputWarning(bool show)
 {
     style()->unpolish(ui->pinInput);

src/ui/webeiddialog.hpp

@@ -104,6 +104,7 @@ class WebEidDialog final : public WebEidUI
     template <typename Func>
     void setupOK(Func func, const char* text = {}, bool enabled = false);
     void displayPinBlockedError();
+    void displayFatalError(std::function<QString()> message);
 
     void showPinInputWarning(bool show);
     void resizeHeight();