Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linux: Re-introduce D-Bus API Authorization #7110

Merged
merged 13 commits into from Jul 26, 2023
1 change: 1 addition & 0 deletions linux/debian/control
Expand Up @@ -13,6 +13,7 @@ Build-Depends: debhelper (>= 11.2),
python3-yaml,
python3-jinja2,
python3-click,
libcap-dev,
libgl-dev,
libopengl-dev (>= 1.3.0~),
libqt6core5compat6-dev (>=6.2.0~),
Expand Down
1 change: 1 addition & 0 deletions linux/mozillavpn.spec
Expand Up @@ -17,6 +17,7 @@ Requires: wireguard-tools

BuildRequires: cargo
BuildRequires: golang >= 1.18
BuildRequires: libcap-devel
BuildRequires: libsecret-devel
BuildRequires: openssl-devel
BuildRequires: python3-yaml
Expand Down
3 changes: 2 additions & 1 deletion src/apps/vpn/cmake/linux.cmake
Expand Up @@ -7,7 +7,8 @@ target_link_libraries(mozillavpn PRIVATE Qt6::DBus)

find_package(PkgConfig REQUIRED)
pkg_check_modules(libsecret REQUIRED IMPORTED_TARGET libsecret-1)
target_link_libraries(mozillavpn PRIVATE PkgConfig::libsecret)
pkg_check_modules(libcap REQUIRED IMPORTED_TARGET libcap)
target_link_libraries(mozillavpn PRIVATE PkgConfig::libsecret PkgConfig::libcap)

# Linux platform source files
target_sources(mozillavpn PRIVATE
Expand Down
151 changes: 151 additions & 0 deletions src/apps/vpn/platforms/linux/daemon/dbusservice.cpp
Expand Up @@ -4,11 +4,15 @@

#include "dbusservice.h"

#include <sys/capability.h>
#include <unistd.h>

#include <QCoreApplication>
#include <QDBusConnection>
#include <QDBusInterface>
#include <QJsonDocument>
#include <QJsonObject>
#include <QScopeGuard>
#include <QtDBus/QtDBus>

#include "dbus_adaptor.h"
Expand Down Expand Up @@ -57,6 +61,9 @@ DBusService::DBusService(QObject* parent) : Daemon(parent) {
QDBusPendingCallWatcher* watcher = new QDBusPendingCallWatcher(reply, this);
QObject::connect(watcher, SIGNAL(finished(QDBusPendingCallWatcher*)), this,
SLOT(userListCompleted(QDBusPendingCallWatcher*)));

// Drop as many root permissions as we are able.
dropRootPermissions();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own understanding, why are we dropping all possible permissions here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is generally good security practice to drop permissions that aren't required. For example, if we had a security bug that allowed an attacker to hijack the process, they would be limited only to the remaining capabilities and would be denied full root permissions.

}

DBusService::~DBusService() { MZ_COUNT_DTOR(DBusService); }
Expand Down Expand Up @@ -99,6 +106,11 @@ QString DBusService::version() {
bool DBusService::activate(const QString& jsonConfig) {
logger.debug() << "Activate";

if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return false;
}

QJsonDocument json = QJsonDocument::fromJson(jsonConfig.toLocal8Bit());
if (!json.isObject()) {
logger.error() << "Invalid input";
Expand Down Expand Up @@ -131,6 +143,12 @@ bool DBusService::activate(const QString& jsonConfig) {

bool DBusService::deactivate(bool emitSignals) {
logger.debug() << "Deactivate";
if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return false;
}

m_sessionUid = 0;
firewallClear();
return Daemon::deactivate(emitSignals);
}
Expand All @@ -141,6 +159,11 @@ QString DBusService::status() {

QString DBusService::getLogs() {
logger.debug() << "Log request";
if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return QString();
}

return Daemon::logs();
}

Expand Down Expand Up @@ -211,6 +234,11 @@ QString DBusService::runningApps() {

/* Update the firewall for running applications matching the application ID. */
bool DBusService::firewallApp(const QString& appName, const QString& state) {
if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return false;
}

logger.debug() << "Setting" << appName << "to firewall state" << state;

// Update the split tunnelling state for any running apps.
Expand All @@ -237,6 +265,11 @@ bool DBusService::firewallApp(const QString& appName, const QString& state) {

/* Update the firewall for the application matching the desired PID. */
bool DBusService::firewallPid(int rootpid, const QString& state) {
if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return false;
}

#if 0
ProcessGroup* group = m_pidtracker->group(rootpid);
if (!group) {
Expand All @@ -258,8 +291,126 @@ bool DBusService::firewallPid(int rootpid, const QString& state) {

/* Clear the firewall and return all applications to the active state */
bool DBusService::firewallClear() {
if (!checkCallerAuthz()) {
logger.error() << "Insufficient caller permissions";
return false;
}

logger.debug() << "Clearing excluded app list";
m_wgutils->resetAllCgroups();
m_excludedApps.clear();
return true;
}

/* Drop root permissions from the daemon. */
void DBusService::dropRootPermissions() {
logger.debug() << "Dropping root permissions";

cap_t caps = cap_get_proc();
if (caps == nullptr) {
logger.warning() << "Failed to retrieve process capabilities";
return;
}
auto guard = qScopeGuard([&] { cap_free(caps); });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being used somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just used for garbage collection. The scopeguard ensures that cap_free() is invoked regardless of how this function exits.


// Clear the capability set, which effectively makes us an unpriveleged user.
cap_clear(caps);

// Acquire CAP_NET_ADMIN, we need it to perform bringup and management
// of the network interfaces and Wireguard tunnel.
//
// Acquire CAP_SETUID, we need it to masquerade as other users on their
// session busses for application tracking.
//
// NOTE: ptrace is a dangerous permission to hold. If it may be safer to
// relent on the executable check and grant CAP_NET_ADMIN to the client
// process during installation.
//
// Clear all other capabilities, effectively discarding our root permissions.
cap_value_t newcaps[] = {CAP_NET_ADMIN, CAP_SETUID};
int numcaps = sizeof(newcaps) / sizeof(cap_value_t);
oskirby marked this conversation as resolved.
Show resolved Hide resolved
if (cap_set_flag(caps, CAP_EFFECTIVE, numcaps, newcaps, CAP_SET) ||
cap_set_flag(caps, CAP_PERMITTED, numcaps, newcaps, CAP_SET)) {
logger.warning() << "Failed to set process capability flags";
return;
}
if (cap_set_proc(caps) != 0) {
logger.warning() << "Failed to update process capabilities";
return;
}
}

/* Checks to see if the caller has sufficient authorization */
bool DBusService::checkCallerAuthz() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the way this function is being used/called above (if (!checkCallerAuthz) {...}, would a name such as DBusService::callerAuthorized() make the code more intuitive?

if (!calledFromDBus()) {
// If this is not a D-Bus call, it came from the daemon itself.
return true;
}
QDBusConnectionInterface* iface = QDBusConnection::systemBus().interface();
oskirby marked this conversation as resolved.
Show resolved Hide resolved

// If the VPN is active, and we know the UID that turned it on, as a special
// case we permit that user full access to the D-Bus API in order to manage
// the connection.
if (m_sessionUid != 0) {
QDBusReply<uint> reply = iface->serviceUid(message().service());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QDBusReply<uint> reply = iface->serviceUid(message().service());
const QDBusReply<uint> reply = iface->serviceUid(message().service());

if (reply.isValid() && m_sessionUid == reply.value()) {
return true;
}
}
// Otherwise, if this is the activate method, we permit any non-root user to
// activate the VPN, but we will remember their UID for later authentication.
else if ((message().type() == QDBusMessage::MethodCallMessage) &&
(message().member() == "activate")) {
QDBusReply<uint> reply = iface->serviceUid(message().service());
oskirby marked this conversation as resolved.
Show resolved Hide resolved
uint senderuid = reply.value();
if (reply.isValid() && senderuid != 0) {
m_sessionUid = senderuid;
return true;
}
}
// In all other cases, the use of this D-Bus API requires the CAP_NET_ADMIN
// permission, which we can check by examining the PID of the sender. Note
// that a zero UID (root) is used as a guard value to fall back to this case.

// Get the PID of the D-Bus message sender.
QDBusReply<uint> reply = iface->servicePid(message().service());
oskirby marked this conversation as resolved.
Show resolved Hide resolved
uint senderpid = reply.value();
oskirby marked this conversation as resolved.
Show resolved Hide resolved
if (!reply.isValid() || (senderpid == 0)) {
// Could not lookup the sender's PID. Rejected!
logger.warning() << "Failed to resolve sender PID";
return false;
}

// Get the capabilties of the sender process.
cap_t caps = cap_get_pid(senderpid);
if (caps == nullptr) {
logger.warning() << "Failed to retrieve process capabilities";
return false;
}
auto guard = qScopeGuard([&] { cap_free(caps); });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the lambda expression isn't being called further down, can we remove the guard assignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope guard is used to ensure that cap_free() is called at function exit, regardless of which path we take to exit the function.


// Check if the calling process has CAP_NET_ADMIN.
cap_flag_value_t flag;
if (cap_get_flag(caps, CAP_NET_ADMIN, CAP_EFFECTIVE, &flag) != 0) {
logger.warning() << "Failed to retrieve process cap_net_admin flags";
return false;
}
return (flag == CAP_SET);
}

uint DBusService::getCallerUid() {
if (!calledFromDBus()) {
// If this is not a D-Bus call, it came from the daemon itself.
return getuid();
}

// Get the UID of the D-Bus message sender.
QDBusConnectionInterface* iface = QDBusConnection::systemBus().interface();
oskirby marked this conversation as resolved.
Show resolved Hide resolved
QDBusReply<uint> reply = iface->serviceUid(message().service());
if (!reply.isValid()) {
// Could not lookup the sender's PID. Rejected!
logger.warning() << "Failed to resolve sender UID";
return 0;
}
return reply.value();
}
9 changes: 8 additions & 1 deletion src/apps/vpn/platforms/linux/daemon/dbusservice.h
Expand Up @@ -5,6 +5,8 @@
#ifndef DBUSSERVICE_H
#define DBUSSERVICE_H

#include <QDBusContext>

#include "apptracker.h"
#include "daemon/daemon.h"
#include "dnsutilslinux.h"
Expand All @@ -14,7 +16,7 @@

class DbusAdaptor;

class DBusService final : public Daemon {
class DBusService final : public Daemon, protected QDBusContext {
Q_OBJECT
Q_DISABLE_COPY_MOVE(DBusService)
Q_CLASSINFO("D-Bus Interface", "org.mozilla.vpn.dbus")
Expand Down Expand Up @@ -51,6 +53,9 @@ class DBusService final : public Daemon {

private:
bool removeInterfaceIfExists();
bool checkCallerAuthz();
uint getCallerUid();
void dropRootPermissions();

private slots:
void appLaunched(const QString& cgroup, const QString& appId, int rootpid);
Expand All @@ -68,6 +73,8 @@ class DBusService final : public Daemon {

AppTracker* m_appTracker = nullptr;
QList<QString> m_excludedApps;

uint m_sessionUid = 0;
};

#endif // DBUSSERVICE_H