Keepassxc: Secret service integration, confirm authorization request (CVE-2018-19358)

[intika]

Summary

This is for the secret service implementation this would be similar to when a browser extension request access to DB (keepassxc then prompt for confirmation) or similar to the functioning of kwallet (where it ask for confirmation when new app request the service)...

Current Behavior

When the secret service DB is unlocked any application can request password without confirmation... (we can enable the notification tho to be notified of an access.)

Desired Behavior

Keepassxc should ask for confirmation when a new application use secret service for the first time

Possible Solution

Add a feature to ask for confirmation (like the notification feature but with confirmation)
save the authorized app somewhere in the secret service db...

Note

This has always been an issue in gnome keyring (details here gnome-issue and CVE-2018-19358) it has never been solved, this can be considered as a serious security bug... and this is impacting keepassxc as well...

An advanced implementation could even limit an application to its passwords to avoid allowing an authorized application to get other passwords

I don't really have much time to implement this... but i will be probably adding a patch to trigger a request authorization window (for my needs anyway... is it worth a PR ?)...

[intika]

Here is a quick ugly patch that request confirmation on secret service access... i don't really have the time to develop a proper solution right now... may be later on...

This will basically display the same infos as on the notification dialog, when secret service password is accessed, but this time with a dialog requesting allow/deny

This fix is bound to the feature "Notify on access"

diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp
index f3b8bceb..48db3433 100644
--- a/src/fdosecrets/objects/Item.cpp
+++ b/src/fdosecrets/objects/Item.cpp
@@ -33,6 +33,10 @@
 #include <QSet>
 #include <QTextCodec>

+#include "fdosecrets/FdoSecretsSettings.h"
+#include "gui/MessageBox.h"
+#include <QMessageBox>
+
 namespace FdoSecrets
 {

@@ -235,6 +239,26 @@ namespace FdoSecrets
                 tr(R"(Entry "%1" from database "%2" was used by %3)")
                     .arg(m_backend->title(), collection()->name(), callingPeerName()));
         }
+        //return secret;
+
+        if (calledFromDBus() && FdoSecrets::settings()->showNotification()) {
+            QMessageBox msgBox;
+            msgBox.setIcon(QMessageBox::Question);
+            QPushButton *allowButton = msgBox.addButton(tr("Allow access"), QMessageBox::YesRole);
+            QPushButton *denyButton = msgBox.addButton(tr("Deny"), QMessageBox::NoRole);
+            msgBox.setDefaultButton(allowButton);
+            msgBox.setEscapeButton(denyButton);
+            msgBox.setWindowFlags(Qt::WindowStaysOnTopHint);
+            msgBox.setWindowTitle(tr("KeePassXC Request"));
+            msgBox.setText(
+                    tr("\nKeepassXC Secret Service: new app request\n\nApplication: \n\n%3\n\nRequest entry: \n%1 \n\nFrom database: \n%2\n\n")
+                        .arg(m_backend->title(), collection()->name(), callingPeerName()));    
+            msgBox.exec();
+            msgBox.raise();
+
+            if (msgBox.clickedButton() == allowButton) {return secret;}
+            return {}; 
+        }
         return secret;
     }

diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp
index 6bca1f12..3eb997e5 100644
--- a/src/fdosecrets/objects/Service.cpp
+++ b/src/fdosecrets/objects/Service.cpp
@@ -31,6 +31,8 @@
 #include <QDBusServiceWatcher>
 #include <QDebug>

+#include <QMessageBox>
+
 namespace
 {
     constexpr auto DEFAULT_ALIAS = "default";
@@ -367,6 +369,26 @@ namespace FdoSecrets
                 tr(R"(%n Entry(s) was used by %1)", "%1 is the name of an application", res.size())
                     .arg(callingPeerName()));
         }
+        //return res;
+       
+        if (calledFromDBus() && FdoSecrets::settings()->showNotification()) {
+            QMessageBox msgBox;
+            msgBox.setIcon(QMessageBox::Question);
+            QPushButton *allowButton = msgBox.addButton(tr("Allow access"), QMessageBox::YesRole);
+            QPushButton *denyButton = msgBox.addButton(tr("Deny"), QMessageBox::NoRole);
+            msgBox.setDefaultButton(allowButton);
+            msgBox.setEscapeButton(denyButton);
+            msgBox.setWindowFlags(Qt::WindowStaysOnTopHint);
+            msgBox.setWindowTitle(tr("KeePassXC Request"));
+            msgBox.setText(
+                tr("\nKeepassXC Secret Service: new app request\n\nApplication: \n\n%1\n\nRequesting %n entry\n\n","", res.size())
+                  .arg(callingPeerName()));
+            msgBox.exec();
+            msgBox.raise();
+
+            if (msgBox.clickedButton() == allowButton) {return res;}
+            return {};
+        }
         return res;
     }

[droidmonkey]

Should this be closed??

[phoerious]

I don't think so.

[intika]

i updated my ugly patch after some tests... wont require a lot of time to add it properly with an additional settings entry... i can work on it in 1 week or so... right now this patch is bind to the feature "notify on access"

Note that this is a quick implementation to fix the issue (where it always ask for confirmation if the feature is enabled), an other more elegant fix would be to add a system similar to the browser extension where the authorization choice would be saved to the data base... and may be add the elegant solution on top of this fix where if a user want to be asked anyway... (because app are not authenticated but just known by name)

Sorry i did close the issue because i did not though that it would interest you ;)

Let me know if you want a PR with the current patch.

Edit: i made a pr with the patch... just in case it can help... sorry i don't have a bunch of time for it otherwise i would of made a better implementation

[Aetf]

I'm aware of the issue from the beginning when implementing the feature, and planned to implement something about session authorization per application.

However, the fundamental problem is: DBus is inherently insecure, and there is no way to securely identify an application. The only thing you can get is the caller's process ID and probably a binary name, which can be easily changed. A malicious app can very well appear as a harmless process name. So this gives the user the false illusion that the access is secure but it is not. Ideally, this should be handled in the application level: the spec should force the client to include some public key/signature when creating the session, so the server can easily and reliably identify the client.

Even worse, most clients default to plain/clear text when communicating with the server. The secret can be easily sniffed by any process connected to the same session bus.

The idea of asking confirmation before any secret is retrieved is good. But AFAIK there is no way to implement a reliable "remember my decision for this app" functionality. So my guess is that the user will soon get frustrated so they disable the feature or blindly click through the confirmation.

If we can come up with a reliable solution, I can take the time to properly implement it, including settings etc.

[Aetf]

From some reading, it may be possible to authenticate on the DBus level using configuration files. So the access to Secret Service API on DBus is limited. It's not clear to me how this will solve the impersonate problem, though.

[angelsl]

So this gives the user the false illusion that the access is secure but it is not.

I think having an option to have a prompt is still better than nothing.

Because at least if I get a prompt when I don't expect it, then I know something is up (and I can stop whatever is requesting the credentials from getting it). With the notification now, I 'll notice if something weird is happening, but I can't stop my credentials from being taken.

But AFAIK there is no way to implement a reliable "remember my decision for this app" functionality. So my guess is that the user will soon get frustrated so they disable the feature or blindly click through the confirmation.

Having the option would be nice. It can be disabled by default, so we're no worse than the current situation. You could display a warning about the insecurity of DBus when a user enables the option.

[Aetf]

I'm not against having a prompt. :) But the remembering option would be dangerous unless there is a reliable way to identify applications.

See my comments in the closed PR for more context.

[yshui]

I want to chime in and say it would really nice to have the prompt. right now i am falling back to have the db locked after 10s and type in master password everytime, which is way more frustrating.

As for remembering decision, I agree it is not possible _with how the secret service API is currently designed_. But certainly there is still room for improvement, i.e. asking the client to provide a key for identification.

But ultimately if we assume malicious application exists on the user's computer, then nothing is really that secure. as the identification key could be stolen. is the browser integration not vulnerable to this?

[droidmonkey]

Our browser integration implements all the security features that aetf mentioned a few posts above.

[yshui]

Our browser integration implements all the security features that aetf mentioned a few posts above.

I assume the browser extension would have to store some kind of session key somewhere?

[droidmonkey]

Yes it is stored in the extension settings.

[yshui]

So a malicious program could steal that key. That's what I meant by:

But ultimately if we assume malicious application exists on the user's computer, then nothing is really that secure.

[droidmonkey]

Since the browser extension uses native messaging and local pipes it is far more secure to interception attacks than dbus. So even if the key is "stolen" actually using that key is non trivial to gather data. However, if your system is compromised there is nothing much you can do to protect yourself.

[yshui]

The SecretService does use encryption for secrets. It doesn't have a
mechanism to authenticate the application at the moment, but it certainly
can gain one. If you want to make the API better, you should join the
conversation.

However, if your system is compromised there is nothing much you can do to protect yourself.

Exactly my point.

[Gigadoc2]

Since the browser extension uses native messaging and local pipes it is far more secure to interception attacks than dbus. So even if the key is "stolen" actually using that key is non trivial to gather data. However, if your system is compromised there is nothing much you can do to protect yourself.

May I ask how using native messaging + local pipes is more secure than dbus? On most desktop systems all the applications run as the same user, so couldn't a malicious application just connect to the pipe/socket and pretend to be the browser (after it has read the key from the browsers storage, which also belongs to the same user)? I am under the impression that the main problem (on Linux at least, don't know about the others) is that there is no reliable way to identify an application and that this applies to both DBus and regular pipes/sockets (whcih DBus builds on).

[droidmonkey]

You are correct, they are pretty much the same thing. DBus is certainly a more shared interface, however, than our custom pipe.

[Gigadoc2]

That is true. Thanks!

[yshui]

DBus is certainly a more shared interface, however, than our custom pipe.

Sounds like an argument for security through obscurity

[droidmonkey]

No, it's not. All communication on the browser pipe is encrypted. Only the initial key exchange, when you first register the browser to the db, is unencrypted (necessarily).

[Aetf]

In fdosecrtes sensitive data are also encrypted after the initial key exchange. And (if I understand correctly) the exchanging algorithm ensures that even there is another process monitoring the communication, it can not get the shared secret used to encrypt later messages.