Skip to content

Commit

Permalink
FdoSecrets: Improve client executable path handling (#6915)
Browse files Browse the repository at this point in the history 
* Fixes #6459 

Improves the overall handling of FdoSecrets showing client executable paths to the user. It does the following:

* Check executable file existence as described in [RFC] fdosecrets: add optional confirmation to secret access (#4733)
* Show application PID and dbus address in the client list
* When the executable file is inaccessible, depending on where the client name is shown:
    * when shown inline, e.g. in notification text, where space is limited, clearly say that the path is invalid
    * when shown in auth dialog, show warning and print detailed info about the client
    * when shown in the client list, draw a warning icon

Co-authored-by: Jonathan White <[email protected]>
  • Loading branch information
@Aetf @droidmonkey
Aetf and droidmonkey authored Oct 1, 2021
1 parent 860fcfd commit 60cfba8
Show file tree
Hide file tree
Showing 13 changed files with 516 additions and 152 deletions.
86 changes: 68 additions & 18 deletions share/translations/keepassxc_en.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,35 @@
<translation type="unfinished"></translation>
</message>
<message>
<source>Allow access to entries</source>
<source>Allow Selected</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Allow Selected</source>
<source>Deny All</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Deny All</source>
<source>Non-existing/inaccessible executable path. Please double-check the client is legit.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Name</source>
<translation type="unfinished">Name</translation>
</message>
<message>
<source>PID</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Executable</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Command Line</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Details</source>
<translation type="unfinished"></translation>
</message>
</context>
Expand Down Expand Up @@ -3773,28 +3793,16 @@ Error: %1</source>
<context>
<name>FdoSecrets::SettingsClientModel</name>
<message>
<source>Application</source>
<translation type="unfinished"></translation>
<source>Unknown</source>
<translation type="unfinished">Unknown</translation>
</message>
<message>
<source>Manage</source>
<source>Non-existing/inaccessible executable path. Please double-check the client is legit.</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>FdoSecrets::SettingsDatabaseModel</name>
<message>
<source>File Name</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Group</source>
<translation type="unfinished">Group</translation>
</message>
<message>
<source>Manage</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Unlock to show</source>
<translation type="unfinished"></translation>
Expand Down Expand Up @@ -7249,6 +7257,14 @@ Please consider generating a new key file.</source>
<source>Please present or touch your YubiKey to continue…</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>unknown executable (DBus address %1)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>%1 (invalid executable path)</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>QtIOCompressor</name>
Expand Down Expand Up @@ -7724,6 +7740,40 @@ Please consider generating a new key file.</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>SettingsClientModel</name>
<message>
<source>Application</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>PID</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>DBus Address</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Manage</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>SettingsDatabaseModel</name>
<message>
<source>File Name</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Group</source>
<translation type="unfinished">Group</translation>
</message>
<message>
<source>Manage</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>SettingsWidgetFdoSecrets</name>
<message>
Expand Down
47 changes: 43 additions & 4 deletions src/fdosecrets/dbus/DBusClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,53 @@
#include "fdosecrets/dbus/DBusMgr.h"
#include "fdosecrets/objects/SessionCipher.h"

#include <utility>

namespace FdoSecrets
{
DBusClient::DBusClient(DBusMgr* dbus, const QString& address, uint pid, const QString& name)
bool ProcInfo::operator==(const ProcInfo& other) const
{
return this->pid == other.pid && this->ppid == other.ppid && this->exePath == other.exePath
&& this->name == other.name && this->command == other.command;
}

bool ProcInfo::operator!=(const ProcInfo& other) const
{
return !(*this == other);
}

bool PeerInfo::operator==(const PeerInfo& other) const
{
return this->address == other.address && this->pid == other.pid && this->valid == other.valid
&& this->hierarchy == other.hierarchy;
}

bool PeerInfo::operator!=(const PeerInfo& other) const
{
return !(*this == other);
}

DBusClient::DBusClient(DBusMgr* dbus, PeerInfo process)
: m_dbus(dbus)
, m_address(address)
, m_pid(pid)
, m_name(name)
, m_process(std::move(process))
{
}

DBusMgr* DBusClient::dbus() const
{
return m_dbus;
}

QString DBusClient::name() const
{
auto exePath = m_process.exePath();
if (exePath.isEmpty()) {
return QObject::tr("unknown executable (DBus address %1)").arg(m_process.address);
}
if (!m_process.valid) {
return QObject::tr("%1 (invalid executable path)").arg(exePath);
}
return exePath;
}

bool DBusClient::itemKnown(const QUuid& uuid) const
Expand Down
82 changes: 65 additions & 17 deletions src/fdosecrets/dbus/DBusClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,56 @@ namespace FdoSecrets
class DBusMgr;
class CipherPair;

struct ProcInfo
{
uint pid;
uint ppid;
QString exePath;
QString name;
QString command;

bool operator==(const ProcInfo& other) const;
bool operator!=(const ProcInfo& other) const;
};

/**
* Contains info representing a process.
* This can be obtained by DBusMgr::serviceInfo given a dbus address.
*/
struct PeerInfo
{
/**
* @brief DBus address
*/
QString address;

uint pid;
/**
* @brief Whether current process' exePath points to a valid executable file.
*
* Note that an empty exePath is not valid.
*/
bool valid;

/**
* @brief List of parents of the process.
*
* The first element is the current process. The last element is usually PID 1.
*
* This is for showing to the user only and is intentionally simple.
* Getting detailed process info is beyond the scope of KPXC.
*/
QList<ProcInfo> hierarchy;

QString exePath() const
{
return hierarchy.front().exePath;
}

bool operator==(const PeerInfo& other) const;
bool operator!=(const PeerInfo& other) const;
};

/**
* Represent a client that has made requests to our service. A client is identified by its
* DBus address, which is guaranteed to be unique by the DBus protocol.
Expand All @@ -48,38 +98,39 @@ namespace FdoSecrets
/**
* @brief Given peer's service address, construct a client object
* @param address obtained from `QDBusMessage::service()`
* @param pid the process PID
* @param name the process name
* @param process the process info
*/
explicit DBusClient(DBusMgr* dbus, const QString& address, uint pid, const QString& name);
explicit DBusClient(DBusMgr* dbus, PeerInfo process);

DBusMgr* dbus() const
{
return m_dbus;
}
DBusMgr* dbus() const;

/**
* @return The human readable client name, usually the process name
*/
QString name() const
{
return m_name;
}
QString name() const;

/**
* @return The unique DBus address of the client
*/
QString address() const
{
return m_address;
return m_process.address;
}

/**
* @return The process id of the client
*/
uint pid() const
{
return m_pid;
return m_process.pid;
}

/**
* @return The process info
*/
const PeerInfo& processInfo() const
{
return m_process;
}

QSharedPointer<CipherPair>
Expand Down Expand Up @@ -123,10 +174,7 @@ namespace FdoSecrets

private:
QPointer<DBusMgr> m_dbus;
QString m_address;

uint m_pid{0};
QString m_name{};
PeerInfo m_process;

bool m_authorizedAll{false};

Expand Down
Loading

0 comments on commit 60cfba8

Please sign in to comment.