From 2a9d92faeb48ec284700fd0ee5307cd0e36160f7 Mon Sep 17 00:00:00 2001 From: Aetf Date: Sun, 24 Oct 2021 10:22:50 -0400 Subject: [PATCH] FdoSecrets: reject setting refs via the API (#7043) * FdoSecrets: add TOTP as a readonly attribute * FdoSecrets: reject setting fields containing refs, fixes #6802 It is still possible to set refs using KPXC UI. --- src/fdosecrets/README.md | 13 +++-- src/fdosecrets/objects/Item.cpp | 96 ++++++++++++++++++++++----------- src/fdosecrets/objects/Item.h | 1 + tests/gui/TestGuiFdoSecrets.cpp | 84 +++++++++++++++++++++++------ tests/gui/TestGuiFdoSecrets.h | 3 ++ 5 files changed, 144 insertions(+), 53 deletions(-) diff --git a/src/fdosecrets/README.md b/src/fdosecrets/README.md index bd28754a1..059bdae22 100644 --- a/src/fdosecrets/README.md +++ b/src/fdosecrets/README.md @@ -9,8 +9,9 @@ can connect and access the exposed database in KeePassXC. ## Configurable settings * The user can specify if a database is exposed on DBus, and which group is exposed. -* Whether to show desktop notification is shown when an entry is retrieved. -* Whether to skip confirmation for entries deleted from DBus +* Whether to show desktop notification is shown when an entry's secret is retrieved. +* Whether to confirm for entries deleted from DBus +* Whether to confirm each entry's access ## Implemented Attributes on Item Object @@ -22,10 +23,11 @@ The following attributes are exposed: |UserName|The entry user name| |URL|The entry URL| |Notes|The entry notes| +|TOTP|The TOTP code if the entry has one| In addition, all non-protected custom attributes are also exposed. -## Architecture +## Implementation * `FdoSecrets::Service` is the top level DBus service * There is one and only one `FdoSecrets::Collection` per opened database tab @@ -33,8 +35,11 @@ In addition, all non-protected custom attributes are also exposed. ### Signal connections +Collection here means the `Collection` object in code. Not the logical concept "collection" +that the user interacts with. + - Collections are created when a corresponding database tab opened -- If the database is locked, a collection is created +- If the database is locked, a collection is still created - When the database is unlocked, collection populates its children - If the unlocked database's exposed group is none, collection deletes itself - If the database's exposed group changes, collection repopulates diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index 9a7d94739..2c6d13435 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/src/fdosecrets/objects/Item.cpp @@ -32,10 +32,25 @@ namespace FdoSecrets { + struct EntryUpdater + { + Entry* entry; + explicit EntryUpdater(Entry* entry) + : entry(entry) + { + entry->beginUpdate(); + } - const QSet Item::ReadOnlyAttributes(QSet() << ItemAttributes::UuidKey << ItemAttributes::PathKey); + ~EntryUpdater() + { + entry->endUpdate(); + } + }; - static void setEntrySecret(Entry* entry, const QByteArray& data, const QString& contentType); + const QSet Item::ReadOnlyAttributes(QSet() << ItemAttributes::UuidKey << ItemAttributes::PathKey + << ItemAttributes::TotpKey); + + static DBusResult setEntrySecret(Entry* entry, const QByteArray& data, const QString& contentType); static Secret getEntrySecret(Entry* entry); namespace @@ -110,6 +125,9 @@ namespace FdoSecrets // add some informative and readonly attributes attrs[ItemAttributes::UuidKey] = m_backend->uuidToHex(); attrs[ItemAttributes::PathKey] = path(); + if (m_backend->hasTotp()) { + attrs[ItemAttributes::TotpKey] = m_backend->totp(); + } return {}; } @@ -124,23 +142,28 @@ namespace FdoSecrets return ret; } - m_backend->beginUpdate(); - - auto entryAttrs = m_backend->attributes(); + // set on a temp variable first and check for errors to avoid partial updates + EntryAttributes tempAttrs; + tempAttrs.copyDataFrom(m_backend->attributes()); for (auto it = attrs.constBegin(); it != attrs.constEnd(); ++it) { - if (entryAttrs->isProtected(it.key()) || it.key() == EntryAttributes::PasswordKey) { - continue; + if (tempAttrs.isProtected(it.key()) || it.key() == EntryAttributes::PasswordKey) { + return QDBusError::InvalidArgs; } if (ReadOnlyAttributes.contains(it.key())) { - continue; + return QDBusError::InvalidArgs; } - entryAttrs->set(it.key(), it.value()); + if (EntryAttributes::matchReference(it.value()).hasMatch()) { + return QDBusError::InvalidArgs; + } + + tempAttrs.set(it.key(), it.value()); } - m_backend->endUpdate(); - + // actually update the backend + EntryUpdater eu(m_backend); + m_backend->attributes()->copyDataFrom(&tempAttrs); return {}; } @@ -170,10 +193,12 @@ namespace FdoSecrets return ret; } - m_backend->beginUpdate(); - m_backend->setTitle(label); - m_backend->endUpdate(); + if (EntryAttributes::matchReference(label).hasMatch()) { + return QDBusError::InvalidArgs; + } + EntryUpdater eu(m_backend); + m_backend->setTitle(label); return {}; } @@ -278,12 +303,14 @@ namespace FdoSecrets // decode using session auto decoded = secret.session->decode(secret); - // set in backend - m_backend->beginUpdate(); - setEntrySecret(m_backend, decoded.value, decoded.contentType); - m_backend->endUpdate(); + // block references + if (EntryAttributes::matchReference(decoded.value).hasMatch()) { + return QDBusError::InvalidArgs; + } - return {}; + EntryUpdater eu(m_backend); + // set in backend + return setEntrySecret(m_backend, decoded.value, decoded.contentType); } DBusResult Item::setProperties(const QVariantMap& properties) @@ -378,7 +405,7 @@ namespace FdoSecrets return pathComponents.join('/'); } - void setEntrySecret(Entry* entry, const QByteArray& data, const QString& contentType) + DBusResult setEntrySecret(Entry* entry, const QByteArray& data, const QString& contentType) { auto mimeName = contentType.split(';').takeFirst().trimmed(); @@ -397,23 +424,28 @@ namespace FdoSecrets } if (!mimeType.isValid() || !mimeType.inherits(QStringLiteral("text/plain")) || !codec) { + if (EntryAttributes::matchReference(contentType).hasMatch()) { + return QDBusError::InvalidArgs; + } // we can't handle this content type, save the data as attachment, and clear the password field entry->setPassword(""); entry->attachments()->set(FDO_SECRETS_DATA, data); entry->attributes()->set(FDO_SECRETS_CONTENT_TYPE, contentType); - return; + } else { + auto password = codec->toUnicode(data); + if (EntryAttributes::matchReference(password).hasMatch()) { + return QDBusError::InvalidArgs; + } + // save the data to password field + entry->setPassword(password); + if (entry->attachments()->hasKey(FDO_SECRETS_DATA)) { + entry->attachments()->remove(FDO_SECRETS_DATA); + } + if (entry->attributes()->hasKey(FDO_SECRETS_CONTENT_TYPE)) { + entry->attributes()->remove(FDO_SECRETS_CONTENT_TYPE); + } } - - // save the data to password field - if (entry->attachments()->hasKey(FDO_SECRETS_DATA)) { - entry->attachments()->remove(FDO_SECRETS_DATA); - } - if (entry->attributes()->hasKey(FDO_SECRETS_CONTENT_TYPE)) { - entry->attributes()->remove(FDO_SECRETS_CONTENT_TYPE); - } - - Q_ASSERT(codec); - entry->setPassword(codec->toUnicode(data)); + return {}; } Secret getEntrySecret(Entry* entry) diff --git a/src/fdosecrets/objects/Item.h b/src/fdosecrets/objects/Item.h index 8be4e7d27..a8d4f4ba3 100644 --- a/src/fdosecrets/objects/Item.h +++ b/src/fdosecrets/objects/Item.h @@ -30,6 +30,7 @@ namespace FdoSecrets { constexpr const auto UuidKey = "Uuid"; constexpr const auto PathKey = "Path"; + constexpr const auto TotpKey = "TOTP"; } // namespace ItemAttributes class Session; diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index ffdcd338f..5e2a52e2e 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -41,6 +41,7 @@ #include #include #include +#include int main(int argc, char* argv[]) { @@ -1248,30 +1249,24 @@ void TestGuiFdoSecrets::testItemSecret() // first create Secret in wire format, // then convert to internal format and encrypt // finally convert encrypted internal format back to wire format to pass to SetSecret - wire::Secret ss; - ss.contentType = TEXT_PLAIN; - ss.value = "NewPassword"; - ss.session = QDBusObjectPath(sess->path()); - auto encrypted = m_clientCipher->encrypt(ss.unmarshal(m_plugin->dbus())); - DBUS_VERIFY(item->SetSecret(encrypted.marshal())); - - COMPARE(entry->password().toUtf8(), ss.value); + const QByteArray expected = QByteArrayLiteral("NewPassword"); + auto encrypted = encryptPassword(expected, TEXT_PLAIN, sess); + DBUS_VERIFY(item->SetSecret(encrypted)); + COMPARE(entry->password().toUtf8(), expected); } // set secret with something else is saved as attachment + const QByteArray expected = QByteArrayLiteral("NewPasswordBinary"); { - wire::Secret expected; - expected.contentType = APPLICATION_OCTET_STREAM; - expected.value = QByteArrayLiteral("NewPasswordBinary"); - expected.session = QDBusObjectPath(sess->path()); - DBUS_VERIFY(item->SetSecret(m_clientCipher->encrypt(expected.unmarshal(m_plugin->dbus())).marshal())); - + auto encrypted = encryptPassword(expected, APPLICATION_OCTET_STREAM, sess); + DBUS_VERIFY(item->SetSecret(encrypted)); COMPARE(entry->password(), QStringLiteral("")); - + } + { DBUS_GET(encrypted, item->GetSecret(QDBusObjectPath(sess->path()))); auto ss = m_clientCipher->decrypt(encrypted.unmarshal(m_plugin->dbus())); - COMPARE(ss.contentType, expected.contentType); - COMPARE(ss.value, expected.value); + COMPARE(ss.contentType, APPLICATION_OCTET_STREAM); + COMPARE(ss.value, expected); } } @@ -1374,6 +1369,51 @@ void TestGuiFdoSecrets::testItemLockState() DBUS_VERIFY(item->SetSecret(encrypted)); } +void TestGuiFdoSecrets::testItemRejectSetReferenceFields() +{ + // expose a subgroup, entries in it should not be able to retrieve data from entries outside it + auto rootEntry = m_db->rootGroup()->entries().first(); + VERIFY(rootEntry); + auto subgroup = m_db->rootGroup()->findGroupByPath("/Homebanking/Subgroup"); + VERIFY(subgroup); + FdoSecrets::settings()->setExposedGroup(m_db, subgroup->uuid()); + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + auto item = getFirstItem(coll); + VERIFY(item); + auto sess = openSession(service, DhIetf1024Sha256Aes128CbcPkcs7::Algorithm); + VERIFY(sess); + + const auto refText = QStringLiteral("{REF:P@T:%1}").arg(rootEntry->title()); + + // reject ref in label + { + auto reply = item->setLabel(refText); + VERIFY(reply.isFinished() && reply.isError()); + COMPARE(reply.error().type(), QDBusError::InvalidArgs); + } + // reject ref in custom attributes + { + auto reply = item->setAttributes({{"steal", refText}}); + VERIFY(reply.isFinished() && reply.isError()); + COMPARE(reply.error().type(), QDBusError::InvalidArgs); + } + // reject ref in password + { + auto reply = item->SetSecret(encryptPassword(refText.toUtf8(), "text/plain", sess)); + VERIFY(reply.isFinished() && reply.isError()); + COMPARE(reply.error().type(), QDBusError::InvalidArgs); + } + // reject ref in content type + { + auto reply = item->SetSecret(encryptPassword("dummy", refText, sess)); + VERIFY(reply.isFinished() && reply.isError()); + COMPARE(reply.error().type(), QDBusError::InvalidArgs); + } +} + void TestGuiFdoSecrets::testAlias() { auto service = enableService(); @@ -1585,6 +1625,16 @@ QSharedPointer TestGuiFdoSecrets::createItem(const QSharedPointer(itemPath); } +FdoSecrets::wire::Secret +TestGuiFdoSecrets::encryptPassword(QByteArray value, QString contentType, const QSharedPointer& sess) +{ + wire::Secret ss; + ss.contentType = std::move(contentType); + ss.value = std::move(value); + ss.session = QDBusObjectPath(sess->path()); + return m_clientCipher->encrypt(ss.unmarshal(m_plugin->dbus())).marshal(); +} + bool TestGuiFdoSecrets::driveAccessControlDialog(bool remember) { processEvents(); diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h index 1ed6d7f66..285619f86 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -89,6 +89,7 @@ private slots: void testItemSecret(); void testItemDelete(); void testItemLockState(); + void testItemRejectSetReferenceFields(); void testAlias(); void testDefaultAliasAlwaysPresent(); @@ -120,6 +121,8 @@ private: const FdoSecrets::wire::StringStringMap& attr, bool replace, bool expectPrompt = false); + FdoSecrets::wire::Secret + encryptPassword(QByteArray value, QString contentType, const QSharedPointer& sess); template QSharedPointer getProxy(const QDBusObjectPath& path) const { auto ret = QSharedPointer{