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

@@ -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 show desktop notification is shown when an entry's secret is retrieved.
-* Whether to skip confirmation for entries deleted from DBus
+* 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

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<QString> Item::ReadOnlyAttributes(QSet<QString>() << ItemAttributes::UuidKey << ItemAttributes::PathKey);
+        ~EntryUpdater()
+        {
+            entry->endUpdate();
+        }
+    };
 
-    static void setEntrySecret(Entry* entry, const QByteArray& data, const QString& contentType);
+    const QSet<QString> Item::ReadOnlyAttributes(QSet<QString>() << 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();
+        // set on a temp variable first and check for errors to avoid partial updates
-
+        EntryAttributes tempAttrs;
-        auto entryAttrs = m_backend->attributes();
+        tempAttrs.copyDataFrom(m_backend->attributes());
         for (auto it = attrs.constBegin(); it != attrs.constEnd(); ++it) {
-            if (entryAttrs->isProtected(it.key()) || it.key() == EntryAttributes::PasswordKey) {
+            if (tempAttrs.isProtected(it.key()) || it.key() == EntryAttributes::PasswordKey) {
-                continue;
+                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;
             }
 
-        m_backend->endUpdate();
+            tempAttrs.set(it.key(), it.value());
+        }
 
+        // 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();
+        if (EntryAttributes::matchReference(label).hasMatch()) {
-        m_backend->setTitle(label);
+            return QDBusError::InvalidArgs;
-        m_backend->endUpdate();
+        }
 
+        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
+        // block references
-        m_backend->beginUpdate();
+        if (EntryAttributes::matchReference(decoded.value).hasMatch()) {
-        setEntrySecret(m_backend, decoded.value, decoded.contentType);
+            return QDBusError::InvalidArgs;
-        m_backend->endUpdate();
+        }
 
-        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);
             }
-
+        }
-        Q_ASSERT(codec);
+        return {};
-        entry->setPassword(codec->toUnicode(data));
     }
 
     Secret getEntrySecret(Entry* entry)

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;

tests/gui/TestGuiFdoSecrets.cpp

@@ -41,6 +41,7 @@
 #include <QLineEdit>
 #include <QSignalSpy>
 #include <QTest>
+#include <utility>
 
 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;
+        const QByteArray expected = QByteArrayLiteral("NewPassword");
-        ss.contentType = TEXT_PLAIN;
+        auto encrypted = encryptPassword(expected, TEXT_PLAIN, sess);
-        ss.value = "NewPassword";
+        DBUS_VERIFY(item->SetSecret(encrypted));
-        ss.session = QDBusObjectPath(sess->path());
+        COMPARE(entry->password().toUtf8(), expected);
-        auto encrypted = m_clientCipher->encrypt(ss.unmarshal(m_plugin->dbus()));
-        DBUS_VERIFY(item->SetSecret(encrypted.marshal()));
-
-        COMPARE(entry->password().toUtf8(), ss.value);
     }
 
     // set secret with something else is saved as attachment
+    const QByteArray expected = QByteArrayLiteral("NewPasswordBinary");
     {
-        wire::Secret expected;
+        auto encrypted = encryptPassword(expected, APPLICATION_OCTET_STREAM, sess);
-        expected.contentType = APPLICATION_OCTET_STREAM;
+        DBUS_VERIFY(item->SetSecret(encrypted));
-        expected.value = QByteArrayLiteral("NewPasswordBinary");
-        expected.session = QDBusObjectPath(sess->path());
-        DBUS_VERIFY(item->SetSecret(m_clientCipher->encrypt(expected.unmarshal(m_plugin->dbus())).marshal()));
-
         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.contentType, APPLICATION_OCTET_STREAM);
-        COMPARE(ss.value, expected.value);
+        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<ItemProxy> TestGuiFdoSecrets::createItem(const QSharedPointer<Ses
     return getProxy<ItemProxy>(itemPath);
 }
 
+FdoSecrets::wire::Secret
+TestGuiFdoSecrets::encryptPassword(QByteArray value, QString contentType, const QSharedPointer<SessionProxy>& 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();

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<SessionProxy>& sess);
     template <typename Proxy> QSharedPointer<Proxy> getProxy(const QDBusObjectPath& path) const
     {
         auto ret = QSharedPointer<Proxy>{