From 785a64cc3b3123541657831e615b0e97d3341ca5 Mon Sep 17 00:00:00 2001 From: Janek Bevendorff Date: Fri, 23 Nov 2018 13:49:55 +0100 Subject: [PATCH] Fix bugs introduced by database refactor #2491 (#2503) * Fix SSHAgent identity removal on database lock * Refactor storage and manipulation of SSHAgent keys to streamline process with multiple db's * Clear password field when widget is hidden, resolves #2502 --- src/gui/DatabaseOpenWidget.cpp | 11 +- src/gui/DatabaseWidget.cpp | 9 +- src/gui/PasswordEdit.cpp | 2 +- src/gui/PasswordEdit.h | 3 +- src/gui/entry/EditEntryWidget.cpp | 13 +- src/gui/entry/EditEntryWidget.h | 1 + src/sshagent/SSHAgent.cpp | 238 ++++++++++++++++-------------- src/sshagent/SSHAgent.h | 9 +- 8 files changed, 155 insertions(+), 131 deletions(-) diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 73c41ae0c..b3427dc49 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -124,6 +124,12 @@ void DatabaseOpenWidget::hideEvent(QHideEvent* event) disconnect(YubiKey::instance(), nullptr, this, nullptr); m_yubiKeyBeingPolled = false; #endif + + if (isVisible()) { + return; + } + + clearForms(); } void DatabaseOpenWidget::load(const QString& filename) @@ -148,8 +154,9 @@ void DatabaseOpenWidget::load(const QString& filename) void DatabaseOpenWidget::clearForms() { - m_ui->editPassword->clear(); + m_ui->editPassword->setText(""); m_ui->comboKeyFile->clear(); + m_ui->comboKeyFile->setEditText(""); m_ui->checkPassword->setChecked(true); m_ui->checkKeyFile->setChecked(false); m_ui->checkChallengeResponse->setChecked(false); @@ -225,7 +232,7 @@ void DatabaseOpenWidget::openDatabase() } else { m_ui->messageWidget->showMessage(tr("Unable to open the database:\n%1").arg(error), MessageWidget::Error); - m_ui->editPassword->clear(); + m_ui->editPassword->setText(""); #ifdef WITH_XC_TOUCHID // unable to unlock database, reset TouchID for the current database diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 71db6258e..60c529fe6 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -212,11 +212,8 @@ DatabaseWidget::DatabaseWidget(QSharedPointer db, QWidget* parent) #ifdef WITH_XC_SSHAGENT if (config()->get("SSHAgent", false).toBool()) { - connect(this, - SIGNAL(currentModeChanged(DatabaseWidget::Mode)), - SSHAgent::instance(), - SLOT(databaseModeChanged(DatabaseWidget::Mode))); - connect(this, SIGNAL(closeRequest()), SSHAgent::instance(), SLOT(databaseModeChanged())); + connect(this, SIGNAL(databaseLocked()), SSHAgent::instance(), SLOT(databaseModeChanged())); + connect(this, SIGNAL(databaseUnlocked()), SSHAgent::instance(), SLOT(databaseModeChanged())); } #endif @@ -1147,7 +1144,7 @@ bool DatabaseWidget::lock() if (!m_db->save(nullptr, false, false)) { return false; } - } else if (isLocked()) { + } else if (!isLocked()) { QString msg; if (!m_db->metadata()->name().toHtmlEscaped().isEmpty()) { msg = tr("\"%1\" was modified.\nSave changes?").arg(m_db->metadata()->name().toHtmlEscaped()); diff --git a/src/gui/PasswordEdit.cpp b/src/gui/PasswordEdit.cpp index 02067ccc4..6ec044f9e 100644 --- a/src/gui/PasswordEdit.cpp +++ b/src/gui/PasswordEdit.cpp @@ -61,7 +61,7 @@ void PasswordEdit::setShowPassword(bool show) setText(m_basePasswordEdit->text()); } else { // This fix a bug when the QLineEdit is disabled while switching config - if (isEnabled() == false) { + if (!isEnabled()) { setEnabled(true); setReadOnly(false); } diff --git a/src/gui/PasswordEdit.h b/src/gui/PasswordEdit.h index 222376d09..29b33f0bd 100644 --- a/src/gui/PasswordEdit.h +++ b/src/gui/PasswordEdit.h @@ -20,6 +20,7 @@ #define KEEPASSX_PASSWORDEDIT_H #include +#include class PasswordEdit : public QLineEdit { @@ -46,7 +47,7 @@ private slots: private: bool passwordsEqual() const; - PasswordEdit* m_basePasswordEdit; + QPointer m_basePasswordEdit; }; #endif // KEEPASSX_PASSWORDEDIT_H diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp index 657c352b8..a19a89332 100644 --- a/src/gui/entry/EditEntryWidget.cpp +++ b/src/gui/entry/EditEntryWidget.cpp @@ -1,5 +1,3 @@ -#include - /* * Copyright (C) 2010 Felix Geyer * Copyright (C) 2017 KeePassXC Team @@ -443,6 +441,8 @@ void EditEntryWidget::updateSSHAgentKeyInfo() if (SSHAgent::instance()->isAgentRunning()) { m_sshAgentUi->addToAgentButton->setEnabled(true); m_sshAgentUi->removeFromAgentButton->setEnabled(true); + + SSHAgent::instance()->setAutoRemoveOnLock(key, m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked()); } } @@ -558,21 +558,18 @@ void EditEntryWidget::addKeyToAgent() m_sshAgentUi->commentTextLabel->setText(key.comment()); m_sshAgentUi->publicKeyEdit->document()->setPlainText(key.publicKey()); - quint32 lifetime = 0; + int lifetime = 0; bool confirm = m_sshAgentUi->requireUserConfirmationCheckBox->isChecked(); if (m_sshAgentUi->lifetimeCheckBox->isChecked()) { lifetime = m_sshAgentUi->lifetimeSpinBox->value(); } - if (!SSHAgent::instance()->addIdentity(key, lifetime, confirm)) { + if (!SSHAgent::instance()->addIdentity(key, m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked(), + static_cast(lifetime), confirm)) { showMessage(SSHAgent::instance()->errorString(), MessageWidget::Error); return; } - - if (m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked()) { - SSHAgent::instance()->removeIdentityAtLock(key, m_entry->uuid()); - } } void EditEntryWidget::removeKeyFromAgent() diff --git a/src/gui/entry/EditEntryWidget.h b/src/gui/entry/EditEntryWidget.h index a594d2072..d45b29726 100644 --- a/src/gui/entry/EditEntryWidget.h +++ b/src/gui/entry/EditEntryWidget.h @@ -22,6 +22,7 @@ #include #include #include +#include #include "config-keepassx.h" #include "gui/EditWidget.h" diff --git a/src/sshagent/SSHAgent.cpp b/src/sshagent/SSHAgent.cpp index 05299091a..6244e8d45 100644 --- a/src/sshagent/SSHAgent.cpp +++ b/src/sshagent/SSHAgent.cpp @@ -38,16 +38,17 @@ SSHAgent::SSHAgent(QObject* parent) SSHAgent::~SSHAgent() { - for (const QSet& keys : m_keys.values()) { - for (OpenSSHKey key : keys) { - removeIdentity(key); - } + auto it = m_addedKeys.begin(); + while (it != m_addedKeys.end()) { + OpenSSHKey key = it.key(); + removeIdentity(key); + it = m_addedKeys.erase(it); } } SSHAgent* SSHAgent::instance() { - if (m_instance == nullptr) { + if (!m_instance) { qFatal("Race condition: instance wanted before it was initialized, this is a bug."); } @@ -159,7 +160,16 @@ bool SSHAgent::sendMessage(const QByteArray& in, QByteArray& out) #endif } -bool SSHAgent::addIdentity(OpenSSHKey& key, quint32 lifetime, bool confirm) +/** + * Add the identity to the SSH agent. + * + * @param key identity / key to add + * @param lifetime time after which the key should expire + * @param confirm ask for confirmation before adding the key + * @param removeOnLock autoremove from agent when the Database is locked + * @return true on success + */ +bool SSHAgent::addIdentity(OpenSSHKey& key, bool removeOnLock, quint32 lifetime, bool confirm) { if (!isAgentRunning()) { m_error = tr("No agent running, cannot add identity."); @@ -201,9 +211,18 @@ bool SSHAgent::addIdentity(OpenSSHKey& key, quint32 lifetime, bool confirm) return false; } + OpenSSHKey keyCopy = key; + keyCopy.clearPrivate(); + m_addedKeys[keyCopy] = removeOnLock; return true; } +/** + * Remove an identity from the SSH agent. + * + * @param key identity to remove + * @return true on success + */ bool SSHAgent::removeIdentity(OpenSSHKey& key) { if (!isAgentRunning()) { @@ -222,120 +241,121 @@ bool SSHAgent::removeIdentity(OpenSSHKey& key) request.writeString(keyData); QByteArray responseData; - if (!sendMessage(requestData, responseData)) { - return false; - } - - if (responseData.length() < 1 || static_cast(responseData[0]) != SSH_AGENT_SUCCESS) { - m_error = tr("Agent does not have this identity."); - return false; - } - - return true; + return sendMessage(requestData, responseData); } -void SSHAgent::removeIdentityAtLock(const OpenSSHKey& key, const QUuid& uuid) +/** + * Change "remove identity on lock" setting for a key already added to the agent. + * Will to nothing if the key has not been added to the agent. + * + * @param key key to change setting for + * @param autoRemove whether to remove the key from the agent when database is locked + */ +void SSHAgent::setAutoRemoveOnLock(const OpenSSHKey& key, bool autoRemove) { - OpenSSHKey copy = key; - copy.clearPrivate(); - m_keys[uuid].insert(copy); + if (m_addedKeys.contains(key)) { + m_addedKeys[key] = autoRemove; + } } -void SSHAgent::databaseModeChanged(DatabaseWidget::Mode mode) +void SSHAgent::databaseModeChanged() { - DatabaseWidget* widget = qobject_cast(sender()); - - if (widget == nullptr) { + auto* widget = qobject_cast(sender()); + if (!widget) { return; } - const QUuid& uuid = widget->database()->uuid(); - - if (mode == DatabaseWidget::Mode::LockedMode && m_keys.contains(uuid)) { - - QSet keys = m_keys.take(uuid); - for (OpenSSHKey key : keys) { - if (!removeIdentity(key)) { - emit error(m_error); - } - } - } else if (mode == DatabaseWidget::Mode::ViewMode && !m_keys.contains(uuid)) { - for (Entry* e : widget->database()->rootGroup()->entriesRecursive()) { - - if (widget->database()->metadata()->recycleBinEnabled() - && e->group() == widget->database()->metadata()->recycleBin()) { - continue; - } - - if (!e->attachments()->hasKey("KeeAgent.settings")) { - continue; - } - - KeeAgentSettings settings; - settings.fromXml(e->attachments()->value("KeeAgent.settings")); - - if (!settings.allowUseOfSshKey()) { - continue; - } - - QByteArray keyData; - QString fileName; - if (settings.selectedType() == "attachment") { - fileName = settings.attachmentName(); - keyData = e->attachments()->value(fileName); - } else if (!settings.fileName().isEmpty()) { - QFile file(settings.fileName()); - QFileInfo fileInfo(file); - - fileName = fileInfo.fileName(); - - if (file.size() > 1024 * 1024) { - continue; - } - - if (!file.open(QIODevice::ReadOnly)) { - continue; - } - - keyData = file.readAll(); - } - - if (keyData.isEmpty()) { - continue; - } - - OpenSSHKey key; - - if (!key.parse(keyData)) { - continue; - } - - if (!key.openPrivateKey(e->password())) { - continue; - } - - if (key.comment().isEmpty()) { - key.setComment(e->username()); - } - - if (key.comment().isEmpty()) { - key.setComment(fileName); - } - - if (settings.removeAtDatabaseClose()) { - removeIdentityAtLock(key, uuid); - } - - if (settings.addAtDatabaseOpen()) { - int lifetime = 0; - - if (settings.useLifetimeConstraintWhenAdding()) { - lifetime = settings.lifetimeConstraintDuration(); - } - - if (!addIdentity(key, lifetime, settings.useConfirmConstraintWhenAdding())) { + if (widget->isLocked()) { + auto it = m_addedKeys.begin(); + while (it != m_addedKeys.end()) { + OpenSSHKey key = it.key(); + if (it.value()) { + if (!removeIdentity(key)) { emit error(m_error); } + it = m_addedKeys.erase(it); + } else { + // don't remove it yet + m_addedKeys[key] = false; + ++it; + } + } + + return; + } + + for (Entry* e : widget->database()->rootGroup()->entriesRecursive()) { + + if (widget->database()->metadata()->recycleBinEnabled() + && e->group() == widget->database()->metadata()->recycleBin()) { + continue; + } + + if (!e->attachments()->hasKey("KeeAgent.settings")) { + continue; + } + + KeeAgentSettings settings; + settings.fromXml(e->attachments()->value("KeeAgent.settings")); + + if (!settings.allowUseOfSshKey()) { + continue; + } + + QByteArray keyData; + QString fileName; + if (settings.selectedType() == "attachment") { + fileName = settings.attachmentName(); + keyData = e->attachments()->value(fileName); + } else if (!settings.fileName().isEmpty()) { + QFile file(settings.fileName()); + QFileInfo fileInfo(file); + + fileName = fileInfo.fileName(); + + if (file.size() > 1024 * 1024) { + continue; + } + + if (!file.open(QIODevice::ReadOnly)) { + continue; + } + + keyData = file.readAll(); + } + + if (keyData.isEmpty()) { + continue; + } + + OpenSSHKey key; + + if (!key.parse(keyData)) { + continue; + } + + if (!key.openPrivateKey(e->password())) { + continue; + } + + if (key.comment().isEmpty()) { + key.setComment(e->username()); + } + + if (key.comment().isEmpty()) { + key.setComment(fileName); + } + + if (!m_addedKeys.contains(key) && settings.addAtDatabaseOpen()) { + quint32 lifetime = 0; + + if (settings.useLifetimeConstraintWhenAdding()) { + lifetime = static_cast(settings.lifetimeConstraintDuration()); + } + + if (!addIdentity(key, settings.removeAtDatabaseClose(), + lifetime, settings.useConfirmConstraintWhenAdding())) { + emit error(m_error); } } } diff --git a/src/sshagent/SSHAgent.h b/src/sshagent/SSHAgent.h index 7cd8a1f1f..4311a911a 100644 --- a/src/sshagent/SSHAgent.h +++ b/src/sshagent/SSHAgent.h @@ -21,6 +21,7 @@ #include "OpenSSHKey.h" #include +#include #include #include "gui/DatabaseWidget.h" @@ -35,15 +36,15 @@ public: const QString errorString() const; bool isAgentRunning() const; - bool addIdentity(OpenSSHKey& key, quint32 lifetime = 0, bool confirm = false); + bool addIdentity(OpenSSHKey& key, bool removeOnLock, quint32 lifetime, bool confirm); bool removeIdentity(OpenSSHKey& key); - void removeIdentityAtLock(const OpenSSHKey& key, const QUuid& uuid); + void setAutoRemoveOnLock(const OpenSSHKey& key, bool autoRemove); signals: void error(const QString& message); public slots: - void databaseModeChanged(DatabaseWidget::Mode mode = DatabaseWidget::Mode::LockedMode); + void databaseModeChanged(); private: const quint8 SSH_AGENT_FAILURE = 5; @@ -71,7 +72,7 @@ private: const quint32 AGENT_COPYDATA_ID = 0x804e50ba; #endif - QMap> m_keys; + QHash m_addedKeys; QString m_error; };