Relax strictness of TOTP Base32 validation

* Fix #3754 - Accept valid TOTP keys that require padding when converted to Base32.
* Allow use of spaces and lower case letters in the TOTP secret key.
This commit is contained in:
Jonathan White 2019-11-08 18:06:13 -05:00 committed by Janek Bevendorff
parent 4edb623745
commit f9d2696046
2 changed files with 11 additions and 7 deletions

View File

@ -1,6 +1,5 @@
/* /*
* Copyright (C) 2017 Weslly Honorato <weslly@protonmail.com> * Copyright (C) 2019 KeePassXC Team <team@keepassxc.org>
* Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
* *
* This program is free software: you can redistribute it and/or modify * This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
@ -46,9 +45,11 @@ TotpSetupDialog::~TotpSetupDialog()
void TotpSetupDialog::saveSettings() void TotpSetupDialog::saveSettings()
{ {
// Secret key sanity check // Secret key sanity check
auto key = m_ui->seedEdit->text().toLatin1(); // Convert user input to all uppercase and remove '='
auto key = m_ui->seedEdit->text().toUpper().remove(" ").remove("=").toLatin1();
auto sanitizedKey = Base32::sanitizeInput(key); auto sanitizedKey = Base32::sanitizeInput(key);
if (sanitizedKey != key) { // Use startsWith to ignore added '=' for padding at the end
if (!sanitizedKey.startsWith(key)) {
MessageBox::information(this, MessageBox::information(this,
tr("Invalid TOTP Secret"), tr("Invalid TOTP Secret"),
tr("You have entered an invalid secret key. The key must be in Base32 format.\n" tr("You have entered an invalid secret key. The key must be in Base32 format.\n"
@ -112,7 +113,9 @@ void TotpSetupDialog::init()
// Read entry totp settings // Read entry totp settings
auto settings = m_entry->totpSettings(); auto settings = m_entry->totpSettings();
if (settings) { if (settings) {
m_ui->seedEdit->setText(settings->key); auto key = settings->key;
m_ui->seedEdit->setText(key.remove("="));
m_ui->seedEdit->setCursorPosition(0);
m_ui->stepSpinBox->setValue(settings->step); m_ui->stepSpinBox->setValue(settings->step);
if (settings->encoder.shortName == Totp::STEAM_SHORTNAME) { if (settings->encoder.shortName == Totp::STEAM_SHORTNAME) {

View File

@ -756,7 +756,8 @@ void TestGui::testTotp()
QApplication::processEvents(); QApplication::processEvents();
QString exampleSeed = "gezdgnbvgy3tqojqgezdgnbvgy3tqojq"; QString exampleSeed = "gezd gnbvgY 3tqojqGEZdgnb vgy3tqoJq===";
QString expectedFinalSeed = exampleSeed.toUpper().remove(" ").remove("=");
auto* seedEdit = setupTotpDialog->findChild<QLineEdit*>("seedEdit"); auto* seedEdit = setupTotpDialog->findChild<QLineEdit*>("seedEdit");
seedEdit->setText(""); seedEdit->setText("");
QTest::keyClicks(seedEdit, exampleSeed); QTest::keyClicks(seedEdit, exampleSeed);
@ -781,7 +782,7 @@ void TestGui::testTotp()
editEntryWidget->setCurrentPage(1); editEntryWidget->setCurrentPage(1);
auto* attrTextEdit = editEntryWidget->findChild<QPlainTextEdit*>("attributesEdit"); auto* attrTextEdit = editEntryWidget->findChild<QPlainTextEdit*>("attributesEdit");
QTest::mouseClick(editEntryWidget->findChild<QAbstractButton*>("revealAttributeButton"), Qt::LeftButton); QTest::mouseClick(editEntryWidget->findChild<QAbstractButton*>("revealAttributeButton"), Qt::LeftButton);
QCOMPARE(attrTextEdit->toPlainText(), exampleSeed); QCOMPARE(attrTextEdit->toPlainText(), expectedFinalSeed);
auto* editEntryWidgetButtonBox = editEntryWidget->findChild<QDialogButtonBox*>("buttonBox"); auto* editEntryWidgetButtonBox = editEntryWidget->findChild<QDialogButtonBox*>("buttonBox");
QTest::mouseClick(editEntryWidgetButtonBox->button(QDialogButtonBox::Ok), Qt::LeftButton); QTest::mouseClick(editEntryWidgetButtonBox->button(QDialogButtonBox::Ok), Qt::LeftButton);