login: handle -EALREADY from bus_verify_polkit_async_full()

Commit 536c18e5c3 ("bus-polkit: shortcut auth. after first denial")
added logic to async_polkit_query_check_action() that returns
-EALREADY when a failure or denial decision was made for a previous
action.

This has the consequence that root is able to ignore inhibitors and
shutdown etc. even when polkit explicitly denies it. This is because
when systemctl's verb_start_special() calls logind_reboot(), unless
the call succeeds or returns one of -EACCES, -EOPNOTSUPP, or
-EINPROGRESS, a fallback path is taken to attempt the action without
going through logind. Hence, since logind_reboot() started returning
-EALREADY in some cases, the fallback path was taken, and the shutdown
was performed anyways.

For example:

 root@ubuntu:/# cat /etc/polkit-1/rules.d/10-systemd-logind-no-skip-inhibitors.rules
 // Never allow strong inhibitors to be ignored.
 polkit.addRule(function(action, subject) {
     if ((action.id == "org.freedesktop.login1.power-off-ignore-inhibit" ||
          action.id == "org.freedesktop.login1.reboot-ignore-inhibit" ||
          action.id == "org.freedesktop.login1.halt-ignore-inhibit" ||
          action.id == "org.freedesktop.login1.suspend-ignore-inhibit" ||
          action.id == "org.freedesktop.login1.hibernate-ignore-inhibit")) {

         return polkit.Result.NO;
     }
 });
 root@ubuntu:/# systemctl reboot -i
 Call to Reboot failed: Operation already in progress

..but the reboot continues anyways due to the fallback.

To fix this, add logic in systemd-logind's verify_shutdown_creds() to
handle -EALREADY from bus_verify_polkit_async_full(): if we receive
-EALREADY when checking authorization for <action>-multiple-sessions,
and we are blocked on inhibitors, continue on to get the decision for
<action>-ignore-inhibit directly.

While here, add similar logic to method_inhibit(), which may need to
verify multiple polkit actions in a single call.

Fixes 536c18e5c33fd682fcd38d228b46a339adbe150b

diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
index 7bec9e644d732b3e6fee2584efe66407cdff2051..b0b7cf6cc013abaced49c006fb27044f1f033389 100644 (file)
--- a/src/login/logind-dbus.c
+++ b/src/login/logind-dbus.c
@@ -2172,7 +2172,7 @@ static int verify_shutdown_creds(
                 sd_bus_error *error) {
 
         _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL;
-        bool multiple_sessions, blocked, interactive;
+        bool multiple_sessions, blocked, interactive, error_or_denial = false;
         Inhibitor *offending = NULL;
         uid_t uid;
         int r;
@@ -2206,8 +2206,15 @@ static int verify_shutdown_creds(
                                 interactive ? POLKIT_ALLOW_INTERACTIVE : 0,
                                 &m->polkit_registry,
                                 error);
-                if (r < 0)
-                        return r;
+                if (r < 0) {
+                        /* If we get -EALREADY, it means a polkit decision was made, but not for
+                         * this action in particular. Assuming we are blocked on inhibitors,
+                         * ignore that error and allow the decision to be revealed below. */
+                        if (blocked && r == -EALREADY)
+                                error_or_denial = true;
+                        else
+                                return r;
+                }
                 if (r == 0)
                         return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
         }
@@ -2261,6 +2268,13 @@ static int verify_shutdown_creds(
                         return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
         }
 
+        /* If error_or_denial was set above, it means that a polkit denial or
+         * error was deferred for a future call to bus_verify_polkit_async_full()
+         * to catch. In any case, it also means that the payload guarded by
+         * these polkit calls should never be executed, and hence we should
+         * never reach this point. */
+        assert(!error_or_denial);
+
         return 0;
 }
 
@@ -3754,6 +3768,7 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error
         InhibitMode mm;
         InhibitWhat w;
         uid_t uid;
+        bool error_or_denial = false;
         int r;
 
         assert(message);
@@ -3801,12 +3816,26 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error
                                 /* details= */ NULL,
                                 &m->polkit_registry,
                                 error);
-                if (r < 0)
-                        return r;
+                if (r < 0) {
+                        /* If we get -EALREADY, it means a polkit decision was made, but not for
+                         * this action in particular. Assuming there are more actions requested,
+                         * ignore that error and allow the decision to be revealed later. */
+                        if ((~v & w) && r == -EALREADY)
+                                error_or_denial = true;
+                        else
+                                return r;
+                }
                 if (r == 0)
                         return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
         }
 
+        /* If error_or_denial was set above, it means that a polkit denial or
+         * error was deferred for a future call to bus_verify_polkit_async()
+         * to catch. In any case, it also means that the payload guarded by
+         * these polkit calls should never be executed, and hence we should
+         * never reach this point. */
+        assert(!error_or_denial);
+
         r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID|SD_BUS_CREDS_PIDFD, &creds);
         if (r < 0)
                 return r;