Linux: Re-introduce D-Bus API Authorization #7110
Conversation
oskirby
changed the title
oskirby
changed the title
oskirby
force-pushed
the
linux-dbus-api-authz
branch
from
07a1708
to
95febdd
Compare
oskirby
force-pushed
the
linux-dbus-api-authz
branch
from
6805d9e
to
31d3656
Compare
| } | ||
|
|
||
| /* Checks to see if the caller has sufficient authorization */ | ||
| bool DBusService::checkCallerAuthz() { |
There was a problem hiding this comment.
Given the way this function is being used/called above (if (!checkCallerAuthz) {...}, would a name such as DBusService::callerAuthorized() make the code more intuitive?
| @@ -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(); | |||
There was a problem hiding this comment.
For my own understanding, why are we dropping all possible permissions here?
There was a problem hiding this comment.
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.
| logger.warning() << "Failed to retrieve process capabilities"; | ||
| return; | ||
| } | ||
| auto guard = qScopeGuard([&] { cap_free(caps); }); |
There was a problem hiding this comment.
This is just used for garbage collection. The scopeguard ensures that cap_free() is invoked regardless of how this function exits.
| logger.warning() << "Failed to retrieve process capabilities"; | ||
| return false; | ||
| } | ||
| auto guard = qScopeGuard([&] { cap_free(caps); }); |
There was a problem hiding this comment.
If the lambda expression isn't being called further down, can we remove the guard assignment?
There was a problem hiding this comment.
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 for CAP_NET_ADMIN to auth D-Bus calls. * Add libcap as a build dependency * Require D-Bus authorization for controller APIs * Permit API calls originating from the daemon itself. * Drop daemon permissions after launch. * Apply auth check to split tunneling APIs too * Restrict API access by UID
* Linux: Re-introduce D-Bus API Authorization (#7110) * Check for CAP_NET_ADMIN to auth D-Bus calls. * Add libcap as a build dependency * Require D-Bus authorization for controller APIs * Permit API calls originating from the daemon itself. * Drop daemon permissions after launch. * Apply auth check to split tunneling APIs too * Restrict API access by UID --------- Co-authored-by: Naomi Kirby <oskirby@gmail.com>
* Check for CAP_NET_ADMIN to auth D-Bus calls. * Add libcap as a build dependency * Require D-Bus authorization for controller APIs * Permit API calls originating from the daemon itself. * Drop daemon permissions after launch. * Apply auth check to split tunneling APIs too * Restrict API access by UID
Description
We recently found that the use of polkit in our code was incorrect in that it wasn't actually enforcing anything so we decided to simply remove the code around polkit. This PR attempts to restore proper authorization of the D-Bus API used to manipulate the controller and network interface.
The authorization check goes as follows:
CAP_NET_ADMINcapability, then access to the D-Bus API is granted.activatemethod and the VPN was previously disconnected, access is granted and the UID that activated the connection is recorded for later.In other words: Any user can activate the VPN, and once activated only that user can manipulate the VPN connection. However, processes with
CAP_NET_ADMINbypass this check and are always authorized.Checklist