From e42c67e7799cf09a09650105000eeaebf3415abc Mon Sep 17 00:00:00 2001 From: lauris71 Date: Tue, 6 Aug 2024 15:33:36 +0300 Subject: [PATCH] Resolve fixme-s in code (#303) * Resolved/removed fixme-s in code Signed-off-by: Lauris Kaplinski * Make PIN erasing more secure WE2-479 Signed-off-by: Mart Somermaa --------- Signed-off-by: Lauris Kaplinski Signed-off-by: Mart Somermaa Co-authored-by: Lauris Kaplinski Co-authored-by: Mart Somermaa --- src/controller/CMakeLists.txt | 1 + .../command-handlers/authenticate.cpp | 13 ++++--- src/controller/command-handlers/sign.cpp | 14 ++++--- .../command-handlers/signauthutils.cpp | 38 +++++++------------ .../command-handlers/signauthutils.hpp | 2 +- src/controller/controller.cpp | 9 +++++ src/controller/inputoutputmode.cpp | 3 -- src/controller/utils/erasedata.hpp | 35 +++++++++++++++++ src/mac/js | 2 +- src/ui/webeiddialog.cpp | 32 ++++++++++++++-- src/ui/webeiddialog.hpp | 1 + 11 files changed, 104 insertions(+), 46 deletions(-) create mode 100644 src/controller/utils/erasedata.hpp diff --git a/src/controller/CMakeLists.txt b/src/controller/CMakeLists.txt index 0748a9d4..58b2bca5 100644 --- a/src/controller/CMakeLists.txt +++ b/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 diff --git a/src/controller/command-handlers/authenticate.cpp b/src/controller/command-handlers/authenticate.cpp index a5697386..3af44db0 100644 --- a/src/controller/command-handlers/authenticate.cpp +++ b/src/controller/command-handlers/authenticate.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -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); diff --git a/src/controller/command-handlers/sign.cpp b/src/controller/command-handlers/sign.cpp index d2b7bc45..5b383aba 100644 --- a/src/controller/command-handlers/sign.cpp +++ b/src/controller/command-handlers/sign.cpp @@ -25,6 +25,8 @@ #include "signauthutils.hpp" #include "utils/utils.hpp" +#include + using namespace electronic_id; namespace @@ -95,16 +97,16 @@ void Sign::emitCertificatesReady(const std::vector& 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}}; diff --git a/src/controller/command-handlers/signauthutils.cpp b/src/controller/command-handlers/signauthutils.cpp index 52615529..eda56432 100644 --- a/src/controller/command-handlers/signauthutils.cpp +++ b/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(const QString& argName, const Q template QByteArray validateAndGetArgument(const QString& argName, const QVariantMap& args, bool allowNull); -template -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(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(pin); - eraseData(pinQByteArray); + for (uint i = 0; i < len; i++) { + pin[i] = pinQStr[i].unicode() & 0xff; + } - return pinBytes; + eraseData(pinQStr); } QVariantMap signatureAlgoToVariantMap(const SignatureAlgorithm signatureAlgo) diff --git a/src/controller/command-handlers/signauthutils.hpp b/src/controller/command-handlers/signauthutils.hpp index b26cccbd..2aaa2b38 100644 --- a/src/controller/command-handlers/signauthutils.hpp +++ b/src/controller/command-handlers/signauthutils.hpp @@ -45,6 +45,6 @@ extern template QString validateAndGetArgument(const QString& argName, extern template QByteArray validateAndGetArgument(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); diff --git a/src/controller/controller.cpp b/src/controller/controller.cpp index 5894e103..4bbbd9f7 100644 --- a/src/controller/controller.cpp +++ b/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()); } diff --git a/src/controller/inputoutputmode.cpp b/src/controller/inputoutputmode.cpp index 2d5480b5..f9d57e59 100644 --- a/src/controller/inputoutputmode.cpp +++ b/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"); } diff --git a/src/controller/utils/erasedata.hpp b/src/controller/utils/erasedata.hpp new file mode 100644 index 00000000..bc6e2b86 --- /dev/null +++ b/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 + +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(data.constData()); + for (int i = 0; i < data.length(); ++i) { + chars[i] = '\0'; + } +} diff --git a/src/mac/js b/src/mac/js index aa573f48..8a8de4f8 160000 --- a/src/mac/js +++ b/src/mac/js @@ -1 +1 @@ -Subproject commit aa573f48e03a5740483d78097800ae8e47377019 +Subproject commit 8a8de4f85b90800099790edca9e869fa965695d9 diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index f13ffe41..261319b5 100644 --- a/src/ui/webeiddialog.cpp +++ b/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(&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 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 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); diff --git a/src/ui/webeiddialog.hpp b/src/ui/webeiddialog.hpp index cdbe2c0b..bf60a98a 100644 --- a/src/ui/webeiddialog.hpp +++ b/src/ui/webeiddialog.hpp @@ -104,6 +104,7 @@ class WebEidDialog final : public WebEidUI template void setupOK(Func func, const char* text = {}, bool enabled = false); void displayPinBlockedError(); + void displayFatalError(std::function message); void showPinInputWarning(bool show); void resizeHeight();