From 2a62ccdd47514c260928e64fa3c44b1331158482 Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Tue, 30 Jul 2024 23:00:40 +0300 Subject: [PATCH] Resolved/removed fixme-s in code Signed-off-by: Lauris Kaplinski --- .../command-handlers/authenticate.cpp | 13 +++++----- src/controller/command-handlers/sign.cpp | 14 +++++----- .../command-handlers/signauthutils.cpp | 26 +++++++++---------- .../command-handlers/signauthutils.hpp | 2 +- src/controller/controller.cpp | 9 +++++++ src/controller/inputoutputmode.cpp | 3 --- src/ui/webeiddialog.cpp | 26 ++++++++++++++++--- src/ui/webeiddialog.hpp | 1 + 8 files changed, 61 insertions(+), 33 deletions(-) diff --git a/src/controller/command-handlers/authenticate.cpp b/src/controller/command-handlers/authenticate.cpp index a5697386..65276283 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().smartcard(), 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..8630f54b 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().smartcard(), 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..6b525e16 100644 --- a/src/controller/command-handlers/signauthutils.cpp +++ b/src/controller/command-handlers/signauthutils.cpp @@ -24,7 +24,6 @@ #include "ui.hpp" #include "commandhandler.hpp" -#include "utils/utils.hpp" using namespace electronic_id; @@ -78,29 +77,28 @@ inline void eraseData(T& data) } } -pcsc_cpp::byte_vector getPin(const ElectronicID& card, WebEidUI* window) +int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, WebEidUI* window) { // Doesn't apply to PIN pads. - if (card.smartcard().readerHasPinPad() || card.providesExternalPinDialog()) { - return {}; + if (card.readerHasPinPad()) { + return 0; } REQUIRE_NON_NULL(window) - auto pin = window->getPin(); - if (pin.isEmpty()) { + QString pinqs = window->getPin(); + if (pinqs.isEmpty()) { THROW(ProgrammingError, "Empty PIN"); } + int len = (int)pinqs.length(); - // TODO: Avoid making copies of the PIN in memory. - auto pinQByteArray = pin.toUtf8(); - pcsc_cpp::byte_vector pinBytes {pinQByteArray.begin(), pinQByteArray.end()}; - - // TODO: Verify that the buffers are actually zeroed and no copies remain. - eraseData(pin); - eraseData(pinQByteArray); + pin.resize(len); + for (int i = 0; i < len; i++) { + pin[i] = pinqs[i].cell(); + } + eraseData(pinqs); - return pinBytes; + return len; } QVariantMap signatureAlgoToVariantMap(const SignatureAlgorithm signatureAlgo) diff --git a/src/controller/command-handlers/signauthutils.hpp b/src/controller/command-handlers/signauthutils.hpp index b26cccbd..cb56f541 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); +int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, 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/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 35950e15..d5b8ff17 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -284,7 +284,10 @@ 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; + pin.clear(); + return ret; } void WebEidDialog::onSmartCardStatusUpdate(const RetriableError status) @@ -332,7 +335,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] { @@ -443,7 +447,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 { @@ -465,8 +468,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; } @@ -678,6 +684,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 726558e8..06b20ccb 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);