From d0bb03ccf60044a3d82853180c1b3699a3518749 Mon Sep 17 00:00:00 2001 From: Mart Somermaa Date: Fri, 2 Aug 2024 22:01:03 +0300 Subject: [PATCH] Make PIN erasing more secure WE2-479 Signed-off-by: Mart Somermaa --- src/controller/CMakeLists.txt | 1 + .../command-handlers/authenticate.cpp | 2 +- src/controller/command-handlers/sign.cpp | 2 +- .../command-handlers/signauthutils.cpp | 34 +++++++----------- .../command-handlers/signauthutils.hpp | 2 +- src/controller/utils/erasedata.hpp | 35 +++++++++++++++++++ src/mac/js | 2 +- src/ui/webeiddialog.cpp | 6 +++- 8 files changed, 57 insertions(+), 27 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 65276283..3af44db0 100644 --- a/src/controller/command-handlers/authenticate.cpp +++ b/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'); diff --git a/src/controller/command-handlers/sign.cpp b/src/controller/command-handlers/sign.cpp index 8630f54b..5b383aba 100644 --- a/src/controller/command-handlers/sign.cpp +++ b/src/controller/command-handlers/sign.cpp @@ -98,7 +98,7 @@ void Sign::emitCertificatesReady(const std::vector& 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'); diff --git a/src/controller/command-handlers/signauthutils.cpp b/src/controller/command-handlers/signauthutils.cpp index 6b525e16..eda56432 100644 --- a/src/controller/command-handlers/signauthutils.cpp +++ b/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(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'; - } -} - -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(pinqs); - return len; + eraseData(pinQStr); } QVariantMap signatureAlgoToVariantMap(const SignatureAlgorithm signatureAlgo) diff --git a/src/controller/command-handlers/signauthutils.hpp b/src/controller/command-handlers/signauthutils.hpp index cb56f541..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); -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); 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 9f2336ec..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")); @@ -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; }