From 495b5273be41f9e54d0c5369e59c3402db4e3202 Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Wed, 15 Nov 2023 12:55:38 +0200 Subject: [PATCH 1/7] Reverted 02662c11e880d5... (+8 squashed commits) Squashed commits: [d850e68] Revert "Reorganized pin handling to be extra sure that copy remains in memory" This reverts commit fdc8b43b8ef1afe8ab5926297837499b4c7ae304. [99bc2b5] Revert "Clear pin string from dialog after query" This reverts commit 2ea88b0beb5b9eb5a341e5ff55ea4fda89f36d04. [b07c28d] Revert "Clear PIN ui with setText("") if certificate is changed" This reverts commit a6763ea55340c34366285e4e87349dbc2c293ead. [a6763ea] Clear PIN ui with setText("") if certificate is changed [d743b24] Separated invalid input error from other technical erros [2ea88b0] Clear pin string from dialog after query [fdc8b43] Reorganized pin handling to be extra sure that copy remains in memory [02662c1] Do not allow PIN retry on unknown error, instead show "technical error" and cancel button Output {"invalid-argument":"message"} in stdin/out mode if json parsing fails --- src/controller/controller.cpp | 17 ++++++++++++++--- src/controller/controller.hpp | 2 ++ src/ui/webeiddialog.cpp | 14 +++++++------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/controller/controller.cpp b/src/controller/controller.cpp index e82dfcac..ca6a27c3 100644 --- a/src/controller/controller.cpp +++ b/src/controller/controller.cpp @@ -39,9 +39,8 @@ using namespace electronic_id; namespace { -// TODO: Should we use more detailed error codes? E.g. report input data error back to the website -// etc. const QString RESP_TECH_ERROR = QStringLiteral("ERR_WEBEID_NATIVE_FATAL"); +const QString RESP_INVALID_INVOCATION = QStringLiteral("ERR_WEBEID_INVALID_INVOCATION"); const QString RESP_USER_CANCEL = QStringLiteral("ERR_WEBEID_USER_CANCELLED"); QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessage) @@ -102,7 +101,8 @@ void Controller::run() commandHandler = getCommandHandler(*command); startCommandExecution(); - + } catch (const std::invalid_argument& error) { + onInvalidInvocation(error.what()); } catch (const std::exception& error) { onCriticalFailure(error.what()); } @@ -361,6 +361,17 @@ void Controller::onCriticalFailure(const QString& error) exit(); } +void Controller::onInvalidInvocation(const QString& error) +{ + qCritical() << "Invalid arguments to command" << std::string(commandType()) + << "reason:" << error; + _result = makeErrorObject(RESP_INVALID_INVOCATION, error); + writeResponseToStdOut(isInStdinMode, _result, commandType()); + disposeUI(); + WebEidUI::showFatalError(); + exit(); +} + void Controller::exit() { if (window) { diff --git a/src/controller/controller.hpp b/src/controller/controller.hpp index ae973f7c..12ec9371 100644 --- a/src/controller/controller.hpp +++ b/src/controller/controller.hpp @@ -70,6 +70,8 @@ class Controller : public QObject // Failure handler, reports the error and quits the application. void onCriticalFailure(const QString& error); + // Invalid arguments, we keep it spearate to distinguish it from technical errors + void onInvalidInvocation(const QString& error); private: void startCommandExecution(); diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index f41f010f..55c5c6dd 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -546,10 +546,12 @@ void WebEidDialog::connectOkToCachePinAndEmitSelectedCertificate( // QString uses QAtomicPointer internally and is thread-safe. pin = ui->pinInput->text(); - // TODO: We need to erase the PIN in the widget buffer, this needs further work. - // Investigate if it is possible to keep the PIN in secure memory, e.g. with a - // custom Qt widget. - ui->pinInput->clear(); + // We use setText instead of clear, so undo/redo will be cleared as well + ui->pinInput->setText(""); + // TODO: To be sure that no copy of PIN text remains in memory, a custom + // widget should be implemented, that: + // - Stores PIN in locked byte vector + // - DOes not leak content through accessibility interface emit accepted(certAndPin); }); @@ -728,13 +730,11 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) tr("Operation not supported"), pixmap("no-id-card"_L1)}; case RetriableError::SMART_CARD_COMMAND_ERROR: - return {tr("Error communicating with the card."), tr("Operation failed"), + return {tr("Error communicating with the card. Please try again."), tr("Operation failed"), pixmap("no-id-card"_L1)}; - // TODO: what action should the user take? Should this be fatal? case RetriableError::PKCS11_ERROR: return {tr("Card driver error. Please try again."), tr("Card driver error"), pixmap("no-id-card"_L1)}; - // TODO: what action should the user take? Should this be fatal? case RetriableError::SCARD_ERROR: return {tr("An error occurred in the Smart Card service required to use the ID-card. Make " "sure that the ID-card and the card reader are connected correctly or relaunch " From 60993567ac399955188a773aff66a9ffade2d5b2 Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Thu, 30 Nov 2023 14:01:19 +0200 Subject: [PATCH 2/7] Use {} instead of empty string to clear pinInput --- src/ui/webeiddialog.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 55c5c6dd..11893bbd 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -547,7 +547,7 @@ void WebEidDialog::connectOkToCachePinAndEmitSelectedCertificate( pin = ui->pinInput->text(); // We use setText instead of clear, so undo/redo will be cleared as well - ui->pinInput->setText(""); + ui->pinInput->setText({}); // TODO: To be sure that no copy of PIN text remains in memory, a custom // widget should be implemented, that: // - Stores PIN in locked byte vector From 10390eebc35e6326bbecdad816a8d1047685945b Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Tue, 28 Nov 2023 12:47:21 +0200 Subject: [PATCH 3/7] Changed vcpkg git commit id --- .github/workflows/cmake-windows.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cmake-windows.yml b/.github/workflows/cmake-windows.yml index 18e6251a..12b720e8 100644 --- a/.github/workflows/cmake-windows.yml +++ b/.github/workflows/cmake-windows.yml @@ -21,7 +21,7 @@ jobs: with: vcpkgArguments: gtest openssl vcpkgTriplet: x64-windows - vcpkgGitCommitId: 9b9c2758ece1d8ac0de90589730bb5ccf45c0874 + vcpkgGitCommitId: 98a562a04cd03728f399e79e1b37bcccb5a69b37 - name: Install Qt uses: jurplel/install-qt-action@v3 From e4a335df2759d2dc885d80e2f846b560895d3645 Mon Sep 17 00:00:00 2001 From: lauris71 Date: Mon, 27 May 2024 10:12:39 +0300 Subject: [PATCH 4/7] Update src/ui/webeiddialog.cpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mart Sõmermaa --- src/ui/webeiddialog.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 01952e1c..887759a6 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -554,7 +554,7 @@ void WebEidDialog::connectOkToCachePinAndEmitSelectedCertificate( // QString uses QAtomicPointer internally and is thread-safe. pin = ui->pinInput->text(); - // We use setText instead of clear, so undo/redo will be cleared as well + // Use setText() instead of clear() to clear undo/redo history as well. ui->pinInput->setText({}); // TODO: To be sure that no copy of PIN text remains in memory, a custom // widget should be implemented, that: From eadbc5ab9a031aacac443a3b9b974344d4e339eb Mon Sep 17 00:00:00 2001 From: lauris71 Date: Mon, 27 May 2024 10:17:27 +0300 Subject: [PATCH 5/7] Update src/ui/webeiddialog.cpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mart Sõmermaa --- src/ui/webeiddialog.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 887759a6..d9e3eeb3 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -556,10 +556,10 @@ void WebEidDialog::connectOkToCachePinAndEmitSelectedCertificate( // Use setText() instead of clear() to clear undo/redo history as well. ui->pinInput->setText({}); - // TODO: To be sure that no copy of PIN text remains in memory, a custom - // widget should be implemented, that: - // - Stores PIN in locked byte vector - // - DOes not leak content through accessibility interface + // TODO: Implement a custom widget to ensure no copy of the PIN text remains in memory. + // The widget should: + // - store the PIN in a locked byte vector, + // - prevent content leaks via the accessibility interface. emit accepted(certAndPin); }); From 83d06cdc5b5f31a565e4206e293dc86bf15bdf74 Mon Sep 17 00:00:00 2001 From: Lauris Kaplinski Date: Mon, 10 Jun 2024 10:27:16 +0300 Subject: [PATCH 6/7] Removed pixmap --- src/ui/webeiddialog.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 0ede7cc6..b8e204db 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -749,11 +749,17 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc QT_TR_NOOP("Operation not supported"), "no-id-card"_L1}; case RetriableError::SMART_CARD_COMMAND_ERROR: - return {QT_TR_NOOP("Error communicating with the card."), QT_TR_NOOP("Operation failed"), - pixmap("no-id-card"_L1)}; + return { + QT_TR_NOOP( + "Error communicating with the card."), + QT_TR_NOOP("Operation failed"), "no-id-card"_L1}; + case RetriableError::PKCS11_ERROR: - return {QT_TR_NOOP("Card driver error. Please try again."), QT_TR_NOOP("Card driver error"), - pixmap("no-id-card"_L1)}; + return { + QT_TR_NOOP( + "Card driver error. Please try again."), + QT_TR_NOOP("Card driver error"), "no-id-card"_L1}; + case RetriableError::SCARD_ERROR: return {QT_TR_NOOP( "An error occurred in the Smart Card service required to use the ID-card. Make " From 2270c90986560c81c0defcd55e6fbd0fccdb40d9 Mon Sep 17 00:00:00 2001 From: Mart Somermaa Date: Fri, 21 Jun 2024 19:12:24 +0300 Subject: [PATCH 7/7] Remove onInvalidInvocation(), clang-format formatting fixes WE2-145 Signed-off-by: Mart Somermaa --- src/controller/controller.cpp | 15 +-------------- src/controller/controller.hpp | 2 -- src/ui/webeiddialog.cpp | 20 ++++++++++---------- tests/tests/changecertificatevaliduntil.hpp | 5 +++-- 4 files changed, 14 insertions(+), 28 deletions(-) diff --git a/src/controller/controller.cpp b/src/controller/controller.cpp index 8be95970..5894e103 100644 --- a/src/controller/controller.cpp +++ b/src/controller/controller.cpp @@ -40,7 +40,6 @@ namespace { const QString RESP_TECH_ERROR = QStringLiteral("ERR_WEBEID_NATIVE_FATAL"); -const QString RESP_INVALID_INVOCATION = QStringLiteral("ERR_WEBEID_INVALID_INVOCATION"); const QString RESP_USER_CANCEL = QStringLiteral("ERR_WEBEID_USER_CANCELLED"); QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessage) @@ -101,8 +100,7 @@ void Controller::run() commandHandler = getCommandHandler(*command); startCommandExecution(); - } catch (const std::invalid_argument& error) { - onInvalidInvocation(error.what()); + } catch (const std::exception& error) { onCriticalFailure(error.what()); } @@ -362,17 +360,6 @@ void Controller::onCriticalFailure(const QString& error) exit(); } -void Controller::onInvalidInvocation(const QString& error) -{ - qCritical() << "Invalid arguments to command" << std::string(commandType()) - << "reason:" << error; - _result = makeErrorObject(RESP_INVALID_INVOCATION, error); - writeResponseToStdOut(isInStdinMode, _result, commandType()); - disposeUI(); - WebEidUI::showFatalError(); - exit(); -} - void Controller::exit() { if (window) { diff --git a/src/controller/controller.hpp b/src/controller/controller.hpp index d0001ea9..e4af4828 100644 --- a/src/controller/controller.hpp +++ b/src/controller/controller.hpp @@ -70,8 +70,6 @@ class Controller : public QObject // Failure handler, reports the error and quits the application. void onCriticalFailure(const QString& error); - // Invalid arguments, we keep it spearate to distinguish it from technical errors - void onInvalidInvocation(const QString& error); private: void startCommandExecution(); diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp index 7c09fe72..93bcfa14 100644 --- a/src/ui/webeiddialog.cpp +++ b/src/ui/webeiddialog.cpp @@ -159,7 +159,7 @@ WebEidDialog::WebEidDialog(QWidget* parent) : WebEidUI(parent), ui(new Private) ui->fatalHelp->hide(); ui->selectAnotherCertificate->hide(); - connect(ui->pageStack, &QStackedWidget::currentChanged, this, [this]{ + connect(ui->pageStack, &QStackedWidget::currentChanged, this, [this] { ui->pageStack->setFixedHeight(ui->pageStack->currentWidget()->sizeHint().height()); }); connect(ui->selectionGroup, qOverload(&QButtonGroup::buttonClicked), this, @@ -700,6 +700,7 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc QT_TR_NOOP("The smart card service required to use the ID-card is not running. Please " "start the smart card service and try again."), QT_TR_NOOP("Launch the Smart Card service"), "cardreader"_L1}; + case RetriableError::NO_SMART_CARD_READERS_FOUND: return {QT_TR_NOOP("Card reader not connected. Please connect the card reader to " "the computer."), @@ -709,6 +710,7 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc case RetriableError::PKCS11_TOKEN_NOT_PRESENT: return {QT_TR_NOOP("ID-card not found. Please insert the ID-card into the reader."), QT_TR_NOOP("Insert the ID-card"), "no-id-card"_L1}; + case RetriableError::SMART_CARD_WAS_REMOVED: case RetriableError::PKCS11_TOKEN_REMOVED: return {QT_TR_NOOP( @@ -722,6 +724,7 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc "Operation failed. Make sure that the ID-card and the card reader are connected " "correctly."), QT_TR_NOOP("Check the ID-card and the reader connection"), "no-id-card"_L1}; + case RetriableError::FAILED_TO_COMMUNICATE_WITH_CARD_OR_READER: return { QT_TR_NOOP( @@ -737,17 +740,13 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc QT_TR_NOOP("Operation not supported"), "no-id-card"_L1}; case RetriableError::SMART_CARD_COMMAND_ERROR: - return { - QT_TR_NOOP( - "Error communicating with the card."), - QT_TR_NOOP("Operation failed"), "no-id-card"_L1}; + return {QT_TR_NOOP("Error communicating with the card."), QT_TR_NOOP("Operation failed"), + "no-id-card"_L1}; case RetriableError::PKCS11_ERROR: - return { - QT_TR_NOOP( - "Card driver error. Please try again."), - QT_TR_NOOP("Card driver error"), "no-id-card"_L1}; - + return {QT_TR_NOOP("Card driver error. Please try again."), QT_TR_NOOP("Card driver error"), + "no-id-card"_L1}; + case RetriableError::SCARD_ERROR: return {QT_TR_NOOP( "An error occurred in the Smart Card service required to use the ID-card. Make " @@ -780,5 +779,6 @@ WebEidDialog::retriableErrorToTextTitleAndIcon(const RetriableError error) noexc case RetriableError::UNKNOWN_ERROR: return {QT_TR_NOOP("Unknown error"), QT_TR_NOOP("Unknown error"), "no-id-card"_L1}; } + return {QT_TR_NOOP("Unknown error"), QT_TR_NOOP("Unknown error"), "no-id-card"_L1}; } diff --git a/tests/tests/changecertificatevaliduntil.hpp b/tests/tests/changecertificatevaliduntil.hpp index 3b8e76cc..7c4dfcf8 100644 --- a/tests/tests/changecertificatevaliduntil.hpp +++ b/tests/tests/changecertificatevaliduntil.hpp @@ -28,7 +28,7 @@ #include inline PcscMock::byte_vector::iterator findUTCDateTime(PcscMock::byte_vector::iterator first, - PcscMock::byte_vector::iterator last) + PcscMock::byte_vector::iterator last) { constexpr unsigned char UTC_DATETIME_TAG = 0x17; constexpr unsigned char LENGTH_TAG = 0x0d; @@ -93,5 +93,6 @@ inline PcscMock::ApduScript replaceCertValidUntilTo2010(const PcscMock::ApduScri inline PcscMock::ApduScript replaceCertValidUntilToNextYear(const PcscMock::ApduScript& script) { // UTCDateTime needs 2-digit year since 2000, add +1 for next year - return replaceCertValidUntilYear(script, 4, std::to_string(QDate::currentDate().year() - 2000 + 1)); + return replaceCertValidUntilYear(script, 4, + std::to_string(QDate::currentDate().year() - 2000 + 1)); }