dbus: check whether the client wanted interactive authentication

[muelli]

Some clients do not want to prompt for passwords, e.g. when authorising keyboards when the screen is locked, prompting for a password doesn't make sense because the user cannot enter anything. This change extracts the flags and checks whether the client called with G_DBUS_CALL_FLAGS_ALLOW_INTERACTIVE_AUTHORIZATION.

[muelli]

I'm coming from https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/676 and I haven't been able to test this, yet. Sorry.

[muelli]

@hartwork may have an opinion here, too :)

[muelli]

• Flag G_DBUS_MESSAGE_FLAGS_ALLOW_INTERACTIVE_AUTHORIZATION seems to require glib >=2.46 according to https://libsoup.org/gio/GDBusMessage.html. configure.ac does not require a specific version of glib as of today and glib <2.46 may (or may not) be used on some to-be-supported environments. So we either need pre-processor if-else for glib <2.46 and >=2.46 or we need to make glib >=2.46 a known and checked-for build time dependency in configure.ac. @radosroka any thoughts on these options?

good catch. I'd bump the dependency...

[hartwork]

good catch. I'd bump the dependency...

@muelli alright, let's start with that approach then, I guess

[muelli]

• (Pull request pull request #545 probably already "fixed" the situation for gnome-settings-daemon (but this change still seems like a good idea).)

that indeed addresses part of the problem. The other part is that we try to authorise keyboards when the screen is locked. We may or may not be able to do that currently. With this change we can detect whether the user is actually allowed to authorise the device. It would still be better if the user could actually perform the authorisation, but this change probably encodes the right behaviour, because it checks whether the caller is fine with asking for authorisation.

[hartwork]

We should also should check with (say) d-feet if interactive auth still works with this patch in practice before merging, to be sure. I haven't done that, yet.

[benzea]

Note that while this patch is correct, it causes a timeout error from the client (https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/676#note_1430710). The problem is that in various places there is code like:

      if (! isAuthorizedByPolkit(invocation)) {
        return;
      }

Which is obviously wrong, as it means that no error is returned. Instead, the code needs to call

        g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR,
          G_DBUS_ERROR_ACCESS_DENIED, "Could not authenticate request"););

at some point.

Though, not sure if G_DBUS_ERROR_ACCESS_DENIED or G_DBUS_ERROR_AUTH_FAILED is right. I suppose isAuthorizedByPolkit could already return the error for the invocation, simplifying the code flow a bit and also allowing returning different values depending on what went wrong.

[muelli]

Which is obviously wrong, as it means that no error is returned. Instead, the code needs to call

uh. thanks for pointing it out. I know only little about dbus architecture and I am a bit surprised. I expected that kind of error to be produced by the broker, somehow.

But it feels like it would be easy to add.

reg. the type of error: The docs have this to say:

G_DBUS_ERROR_ACCESS_DENIED | Security restrictions don't allow doing what you're trying to do.
G_DBUS_ERROR_AUTH_FAILED | Authentication didn't work.

(from https://www.freedesktop.org/software/gstreamer-sdk/data/docs/latest/gio/gio-GDBusError.html - why does it show up under the gstreamer namespace? Rather than a more generic "dbus" namespace or something.)

[hartwork]

@benzea thanks for your input, that's definitely helpful! :+1:

@muelli how should we proceed with this pull request? Should I (a) cherry-pick your commit and make a new pull request applying the ideas of @benzea or (b) make commits for those, export using git format-patch and attach those files here for you to add here using git am or (c) … — what do you think? I should note that I have no permissions on branches or pull requests in USBGuard.

[benzea]

I know only little about dbus architecture and I am a bit surprised. I expected that kind of error to be produced by the broker, somehow.

The broker isn't doing the access control, so it can't react anymore.

[hartwork]

I know only little about dbus architecture and I am a bit surprised. I expected that kind of error to be produced by the broker, somehow.

The broker isn't doing the access control, so it can't react anymore.

Who is the broker in this picture?

[benzea]

Who is the broker in this picture?

The server that forwards messages between clients. i.e. dbus-daemon or dbus-broker in almost all cases.

[hartwork]

Regarding the missing reply aspect brought up by @benzea, there is pull request #548 ready fro review and testing now.

Regarding the allow-interactive-flag aspect brought up by @muelli: Maybe it's okay that the dbus-send CLI never sends the flag with its calls (so that making calls with e.g. sudo is required) but GNOME D-Bus GUI tool d-feet also doesn't send the flag which means that requiring the flag in USBGuard would break use from d-feet, as of today. I have opened a ticket for clarification with d-feet upstream at https://gitlab.gnome.org/GNOME/d-feet/-/issues/26 now.

I'm looking forward to your feedback :beers:

[muelli]

GUI tool d-feet also doesn't send the flag which means that requiring the flag in USBGuard would break use from d-feet, as of today.

That's a problem of the tool, i.e. d-feet. The correct behaviour is to check whether interactive authorisation was allowed by the caller. If the caller doesn't want popups to be spawned, then it'd be nice if usbguard respected that request. Whether d-feet is capable of creating such a request is not relevant for encoding the correct behaviour.

[Cropi]

Hello @muelli , may I ask you to bump the dependency for gio-2.0 >=2.26 as @hartwork suggested?

[hartwork]

@Cropi @muelli >=2.46 please, see https://libsoup.org/gio/GDBusMessage.html#GDBusMessageFlags

[muelli]

Hello @muelli , may I ask you to bump the dependency for gio-2.0 >=2.26 as @hartwork suggested?

I think I tried but couldn't add this new dependency to configure.ac since my autotools-fu is not very good. But I can try again.

[hartwork]

@muelli I believe we need something like this:

   #
   # Check for required D-Bus modules
   #
-  PKG_CHECK_MODULES([dbus], [dbus-1 gio-2.0 polkit-gobject-1],
+  PKG_CHECK_MODULES([dbus], [dbus-1 gio-2.0 >= 2.46 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, polkit-gobject-1) not found!])]

You can try with a version that your system has no way of satisfying — e.g. 100.0 — to see if things fail as expected. Script ./autogen.sh can be used to regenerated configure from configure.ac sources.

[muelli]

I've added the check now. It bails out when I put a fantasy value for the version, so I believe it's working as intended.

I haven't tested these changes, though.

[muelli]

okay, I've managed to test the change. The flag is correctly received by USBGuard. For some reason I don't yet understand, it allows me everything all the time. So there might be glitch in the code, but given that it looks so innocent, I guess it's my system's configuration.

Probably this is causing me getting unconditional access:

ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$ sudo cat /var/lib/polkit-1/localauthority/10-vendor.d/org.usbguard1.pkla
[Allow users in plugdev and sudo groups to run usbguard actions without authentication]
Identity=unix-group:plugdev;unix-group:sudo
Action=org.usbguard1.setParameter;org.usbguard.Policy1.appendRule
ResultActive=yes
ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$

[muelli]

I managed to get a proper test case, now.

ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$ busctl call --allow-interactive-authorization=yes --system org.usbguard1 /org/usbguard1/Policy   org.usbguard.Policy1 appendRule 'sub'  "allow label \"foo\"" 1 true
u 18
ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$ busctl call --allow-interactive-authorization=no --system org.usbguard1 /org/usbguard1/Policy   org.usbguard.Policy1 appendRule 'sub'  "allow label \"foo\"" 1 true
Call failed: Access denied
ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$ sudo cat /var/lib/polkit-1/localauthority/10-vendor.d/org.usbguard1.pkla
[Allow users in plugdev and sudo groups to run usbguard actions without authentication]
Identity=unix-group:pplugdev;unix-group:ssudo
Action=org.usbguard1.setParameter;org.usbguard.Policy1.appendRule
ResultActive=yes
ubuntu@ubuntu-Standard-PC-Q35-ICH9-2009:~$ 

So I conclude that the patch is working as intended.