[CVE-2019-25058] Fix unauthorized access via D-Bus (fixes #273, fixes #403)

.github/workflows/polkit.yml

@@ -0,0 +1,67 @@
+##
+## Copyright (c) 2022 Sebastian Pipping <sebastian@pipping.org>
+##
+## This program is free software; you can redistribute it and/or modify
+## it under the terms of the GNU General Public License as published by
+## the Free Software Foundation; either version 2 of the License, or
+## (at your option) any later version.
+##
+## This program is distributed in the hope that it will be useful,
+## but WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+## GNU General Public License for more details.
+##
+## You should have received a copy of the GNU General Public License
+## along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+name: Check for Polkit policy parse errors
+
+on:
+  push:
+  pull_request:
+
+jobs:
+  polkit_policies:
+
+    name: Check for Polkit policy parse errors
+    runs-on: ubuntu-20.04
+
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: Install runtime dependencies
+      run:  |
+        set -x
+        sudo apt-get update
+        sudo apt-get install --no-install-recommends --yes -V expat
+
+    - name: Check for Polkit policy parse errors
+      run:  |
+        # This will work around pkaction exiting with unjustified(?)
+        # code 1 on Ubuntu 20.04
+        check_polkit_action() { pkaction -v -a "$1" | tee /dev/stderr | fgrep -q 'implicit any' ; }
+
+        set -x
+        actions=(
+            org.usbguard.Devices1.listDevices
+            org.usbguard.Devices1.applyDevicePolicy
+            org.usbguard.Policy1.appendRule
+            org.usbguard.Policy1.listRules
+            org.usbguard.Policy1.removeRule
+            org.usbguard1.getParameter
+            org.usbguard1.setParameter
+        )
+
+        # Self-test: Assert that prior to installation, our Polkit "actions"
+        # are unknown to PolKit.
+        ! check_polkit_action "${actions[0]}"
+
+        # Install the policy so that polkin can find it
+        xmlwf      src/DBus/org.usbguard1.policy
+        sudo cp -v src/DBus/org.usbguard1.policy /usr/share/polkit-1/actions/
+
+        # Assert that after installation, all of our Polkit "actions" are known.
+        # This detects parse error regressions.
+        for action in "${actions[@]}"; do
+            check_polkit_action "${action}"
+        done

configure.ac

@@ -467,7 +467,7 @@ if test "x$with_dbus" = xyes; then
   #
   # Check for required D-Bus modules
   #
-  PKG_CHECK_MODULES([dbus], [dbus-1 gio-2.0],
+  PKG_CHECK_MODULES([dbus], [dbus-1 gio-2.0 polkit-gobject-1],
   [AC_DEFINE([HAVE_DBUS], [1], [Required GDBus API available])
   dbus_summary="system-wide; $dbus_CFLAGS $dbus_LIBS"],
   [AC_MSG_FAILURE([Required D-Bus modules (dbus-1, gio-2.0) not found!])]

src/DBus/DBusBridge.cpp

@@ -15,12 +15,15 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 //
 // Authors: Daniel Kopecek <dkopecek@redhat.com>
+// Authors: Sebastian Pipping <sebastian@pipping.org>
 //
 #ifdef HAVE_BUILD_CONFIG_H
   #include <build-config.h>
 #endif
 
 #include "DBusBridge.hpp"
+#include <polkit/polkit.h>
+
 namespace usbguard
 {
   DBusBridge::DBusBridge(GDBusConnection* const gdbus_connection,
@@ -77,6 +80,10 @@ namespace usbguard
   void DBusBridge::handleRootMethodCall(const std::string& method_name, GVariant* parameters, GDBusMethodInvocation* invocation)
   {
     if (method_name == "getParameter") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       const char* name_cstr = nullptr;
       g_variant_get(parameters, "(&s)", &name_cstr);
       std::string name(name_cstr);
@@ -86,6 +93,10 @@ namespace usbguard
     }
 
     if (method_name == "setParameter") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       const char* name_cstr = nullptr;
       const char* value_cstr = nullptr;
       g_variant_get(parameters, "(&s&s)", &name_cstr, &value_cstr);
@@ -104,6 +115,10 @@ namespace usbguard
   void DBusBridge::handlePolicyMethodCall(const std::string& method_name, GVariant* parameters, GDBusMethodInvocation* invocation)
   {
     if (method_name == "listRules") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       const char* label_cstr = nullptr;
       g_variant_get(parameters, "(&s)", &label_cstr);
       std::string label(label_cstr);
@@ -136,6 +151,10 @@ namespace usbguard
     }
 
     if (method_name == "appendRule") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       const char* rule_spec_cstr = nullptr;
       uint32_t parent_id = 0;
       gboolean temporary = false;
@@ -147,6 +166,10 @@ namespace usbguard
     }
 
     if (method_name == "removeRule") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       uint32_t rule_id = 0;
       g_variant_get(parameters, "(u)", &rule_id);
       removeRule(rule_id);
@@ -165,6 +188,10 @@ namespace usbguard
     USBGUARD_LOG(Debug) << "dbus devices method call: " << method_name;
 
     if (method_name == "listDevices") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       const char* query_cstr = nullptr;
       g_variant_get(parameters, "(&s)", &query_cstr);
       std::string query(query_cstr);
@@ -197,6 +224,10 @@ namespace usbguard
     }
 
     if (method_name == "applyDevicePolicy") {
+      if (! isAuthorizedByPolkit(invocation)) {
+        return;
+      }
+
       uint32_t device_id = 0;
       uint32_t target_integer = 0;
       gboolean permanent = false;
@@ -367,6 +398,113 @@ namespace usbguard
       device_rule.getWithConnectType().c_str());
     return builder;
   }
+
+  std::string DBusBridge::formatGError(GError* error)
+  {
+    if (error) {
+      std::stringstream formatGError;
+      formatGError << error->message << " (code " << error->code << ")";
+      return formatGError.str();
+    }
+    else {
+      return "unknown error";
+    }
+  }
+
+  bool DBusBridge::isAuthorizedByPolkit(GDBusMethodInvocation* invocation)
+  {
+    GError* error = NULL;
+    USBGUARD_LOG(Trace) << "Extracting bus name...";
+    const gchar* const /*no-free!*/ bus_name = g_dbus_method_invocation_get_sender (invocation);
+
+    if (! bus_name) {
+      USBGUARD_LOG(Trace) << "Failed to extract bus name.";
+      return false;
+    }
+
+    USBGUARD_LOG(Trace) << "Extracted bus name \"" << bus_name << "\".";
+    USBGUARD_LOG(Trace) << "Extracting interface name...";
+    const gchar* const /*no-free!*/ interfaceName = g_dbus_method_invocation_get_interface_name(invocation);
+
+    if (! interfaceName) {
+      USBGUARD_LOG(Trace) << "Failed to extract interface name.";
+      return false;
+    }
+
+    USBGUARD_LOG(Trace) << "Extracted interface name \"" << interfaceName << "\".";
+    USBGUARD_LOG(Trace) << "Extracting method name...";
+    const gchar* const /*no-free!*/ methodName = g_dbus_method_invocation_get_method_name(invocation);
+
+    if (! methodName) {
+      USBGUARD_LOG(Trace) << "Failed to extract method name.";
+      return false;
+    }
+
+    std::stringstream action_id;
+    action_id << interfaceName << "." << methodName;
+    USBGUARD_LOG(Trace) << "Extracted method name \"" << methodName << "\".";
+    USBGUARD_LOG(Trace) << "Creating a system bus Polkit subject...";
+    PolkitSubject* const subject = polkit_system_bus_name_new(bus_name);
+
+    if (! subject) {
+      USBGUARD_LOG(Trace) << "Failed to create Polkit subject.";
+      return false;
+    }
+
+    USBGUARD_LOG(Trace) << "Created.";
+    USBGUARD_LOG(Trace) << "Connecting with Polkit authority...";
+    PolkitAuthority* const authority = polkit_authority_get_sync(/*cancellable=*/ NULL, &error);
+
+    if (! authority || error) {
+      USBGUARD_LOG(Trace) << "Failed to connect to Polkit authority: " << formatGError(error) << ".";
+      g_error_free(error);
+      g_object_unref(authority);
+      g_object_unref(subject);
+      return false;
+    }
+
+    USBGUARD_LOG(Trace) << "Connected.";
+    USBGUARD_LOG(Trace) << "Customizing Polkit authentification dialog...";
+    PolkitDetails* const details = polkit_details_new();
+
+    if (! details) {
+      USBGUARD_LOG(Trace) << "Failed to customize the Polkit authentification dialog.";
+      g_object_unref(authority);
+      g_object_unref(subject);
+      return false;
+    }
+
+    polkit_details_insert (details, "polkit.message", "This USBGuard action needs authorization");
+    USBGUARD_LOG(Trace) << "Customized.";
+    USBGUARD_LOG(Trace) << "Checking authorization of action \"" << action_id.str() << "\" with Polkit ...";
+    const PolkitCheckAuthorizationFlags flags = POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION;
+    PolkitAuthorizationResult* const result = polkit_authority_check_authorization_sync
+      (authority,
+        subject,
+        action_id.str().c_str(),
+        details,
+        flags,
+        /*cancellable=*/ NULL,
+        &error);
+
+    if (! result || error) {
+      USBGUARD_LOG(Trace) << "Failed to check back with Polkit for authoriation: " << formatGError(error) << ".";
+      g_error_free(error);
+      g_object_unref(result);
+      g_object_unref(details);
+      g_object_unref(authority);
+      g_object_unref(subject);
+      return false;
+    }
+
+    gboolean isAuthorized = polkit_authorization_result_get_is_authorized(result);
+    USBGUARD_LOG(Trace) << (isAuthorized ? "Authorized" : "Not authorized") << ".";
+    g_object_unref(result);
+    g_object_unref(details);
+    g_object_unref(authority);
+    g_object_unref(subject);
+    return isAuthorized;
+  }
 } /* namespace usbguard */
 
 /* vim: set ts=2 sw=2 et */

src/DBus/DBusBridge.hpp

@@ -88,6 +88,8 @@ namespace usbguard
       bool rule_match,
       uint32_t rule_id);
 
+    static std::string formatGError(GError* error);
+    static bool isAuthorizedByPolkit(GDBusMethodInvocation* invocation);
 
     GDBusConnection* const p_gdbus_connection;
     void(*p_ipc_callback)(bool);

src/DBus/org.usbguard1.policy

@@ -1,23 +1,23 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE policyconfig PUBLIC "-//freedesktop//DTD PolicyKit Policy Configuration 1.0//EN"
         "http://www.freedesktop.org/standards/PolicyKit/1/policyconfig.dtd">
-        
+
 <policyconfig>
   <vendor>The USBGuard Project</vendor>
   <vendor_url>https://github.org/USBGuard/usbguard</vendor_url>
 
   <action id="org.usbguard.Policy1.listRules">
     <description>List the rule set (policy) used by the USBGuard daemon</description>
-    <message>Prevents from listing the USBGuard policy</message>
+    <message>Prevents listing the USBGuard policy</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
-      <allow_active>auth_self_keep_session</allow_active>
+      <allow_active>auth_self_keep</allow_active>
     </defaults>
   </action>
 
   <action id="org.usbguard.Policy1.appendRule">
     <description>Append a new rule to the policy</description>
-    <message>Prevents from appending rules to the USBGuard policy</message>
+    <message>Prevents appending rules to the USBGuard policy</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
       <allow_active>auth_admin</allow_active>
@@ -33,36 +33,36 @@
     </defaults>
   </action>
 
-  <action id="org.usbguard.Devices1.listDevices">
-    <description>List all USB devices recognized by the USBGuard daemon</description>
-    <message>Prevents from listing USB devices recognized by the USBGuard daemon</message>
+  <action id="org.usbguard.Devices1.applyDevicePolicy">
+    <description>Apply a policy to a device in USBGuard</description>
+    <message>Prevents applying a policy to a device in USBGuard</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
-      <allow_active>auth_self_keep_session</allow_active>
+      <allow_active>auth_admin</allow_active>
     </defaults>
   </action>
 
-  <action id="org.usbguard.Devices1.allowDevice">
-    <description>Authorize a USB device via the USBGuard daemon to interact with the system</description>
-    <message>Prevents from authorizing USB devices via the USBGuard daemon</message>
+  <action id="org.usbguard.Devices1.listDevices">
+    <description>List all USB devices recognized by the USBGuard daemon</description>
+    <message>Prevents listing USB devices recognized by the USBGuard daemon</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
-      <allow_active>auth_admin</allow_active>
+      <allow_active>auth_self_keep</allow_active>
     </defaults>
   </action>
 
-  <action id="org.usbguard.Devices1.blockDevice">
-    <description>Deauthorize a USB device via the USBGuard daemon</description>
-    <message>Prevents from deauthorizing USB devices via the USBGuard daemon</message>
+  <action id="org.usbguard1.getParameter">
+    <description>Get the value of a runtime parameter</description>
+    <message>Prevents getting values of runtime USBGuard parameters</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
-      <allow_active>auth_admin</allow_active>
+      <allow_active>auth_self_keep</allow_active>
     </defaults>
   </action>
 
-  <action id="org.usbguard.Devices1.rejectDevice">
-    <description>Remove a USB device via the USBGuard daemon</description>
-    <message>Prevents from removing USB devices via the USBGuard daemon</message>
+  <action id="org.usbguard1.setParameter">
+    <description>Set the value of a runtime parameter</description>
+    <message>Prevents setting values of runtime USBGuard parameters</message>
     <defaults>
       <allow_inactive>no</allow_inactive>
       <allow_active>auth_admin</allow_active>