dbus: check whether the client wanted interactive authentication #546

muelli · 2022-04-15T08:06:25Z

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 · 2022-04-15T08:07:37Z

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 · 2022-04-15T08:13:26Z

@hartwork may have an opinion here, too :)

hartwork

Hi @muelli,

that's a nice idea, thanks for bringing this to my attention and for taking it upstream! 👍
Some notes:

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?
CI does not like the current code formatting, see:
- log at https://github.com/USBGuard/usbguard/runs/6035863630?check_suite_focus=true#step:7:139
- helper ./scripts/reformat-sources.sh
(My picky vote for GDBusMessage* const message adding const.)
(Pull request pull request polkit: Always allow getParameter/listDevices/listRules in active sessions (fixes #544) #545 probably already "fixed" the situation for gnome-settings-daemon (but this change still seems like a good idea).)

What do you think?

Best, Sebastian

muelli · 2022-04-15T09:47:20Z

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 · 2022-04-15T09:51:38Z

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

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

muelli · 2022-04-15T09:52:39Z

(Pull request pull request polkit: Always allow getParameter/listDevices/listRules in active sessions (fixes #544) #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 · 2022-04-15T09:54:43Z

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.

hartwork

@muelli I believe this is the place to insert >=2.46 (for gio):

usbguard/configure.ac

Line 470 in 4982b01

PKG_CHECK_MODULES([dbus], [dbus-1 gio-2.0 polkit-gobject-1],

benzea · 2022-04-15T16:31:10Z

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 · 2022-04-16T07:33:23Z

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 · 2022-04-16T11:04:08Z

@benzea thanks for your input, that's definitely helpful! 👍

@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 · 2022-04-16T11:06:04Z

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 · 2022-04-16T11:10:33Z

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 · 2022-05-06T14:50:17Z

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 · 2022-05-26T17:10:32Z

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 🍻

muelli · 2022-09-27T10:19:09Z

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 · 2022-10-24T08:26:41Z

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

hartwork · 2022-10-24T12:14:38Z

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

muelli · 2022-10-24T20:36:16Z

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 · 2022-10-24T22:31:09Z

@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.

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. The flag is available since 2.46: https://libsoup.org/gio/GDBusMessage.html#GDBusMessageFlags Hence, we update the dependency in configure.ac.

muelli · 2022-11-05T21:01:03Z

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 · 2022-11-07T15:36:45Z

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 · 2022-11-07T16:32:01Z

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.

hartwork suggested changes Apr 15, 2022

View reviewed changes

muelli force-pushed the polkit-interactive-authorisation branch from 0a86714 to 6d890cd Compare April 15, 2022 09:49

hartwork suggested changes Apr 15, 2022

View reviewed changes

hartwork mentioned this pull request May 26, 2022

D-Bus: Send reply on auth failure #548

Merged

muelli force-pushed the polkit-interactive-authorisation branch from 6d890cd to e70d445 Compare November 4, 2022 12:09

Cropi approved these changes Nov 16, 2022

View reviewed changes

radosroka merged commit d003aa1 into USBGuard:master Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dbus: check whether the client wanted interactive authentication #546

dbus: check whether the client wanted interactive authentication #546

muelli commented Apr 15, 2022

muelli commented Apr 15, 2022

muelli commented Apr 15, 2022

hartwork left a comment

muelli commented Apr 15, 2022

hartwork commented Apr 15, 2022

muelli commented Apr 15, 2022

hartwork commented Apr 15, 2022 •

edited

Loading

hartwork left a comment

benzea commented Apr 15, 2022 •

edited

Loading

muelli commented Apr 16, 2022

hartwork commented Apr 16, 2022 •

edited

Loading

benzea commented Apr 16, 2022

hartwork commented Apr 16, 2022

benzea commented May 6, 2022

hartwork commented May 26, 2022

muelli commented Sep 27, 2022

Cropi commented Oct 24, 2022

hartwork commented Oct 24, 2022

muelli commented Oct 24, 2022

hartwork commented Oct 24, 2022

muelli commented Nov 5, 2022

muelli commented Nov 7, 2022 •

edited

Loading

muelli commented Nov 7, 2022

dbus: check whether the client wanted interactive authentication #546

dbus: check whether the client wanted interactive authentication #546

Conversation

muelli commented Apr 15, 2022

muelli commented Apr 15, 2022

muelli commented Apr 15, 2022

hartwork left a comment

Choose a reason for hiding this comment

muelli commented Apr 15, 2022

hartwork commented Apr 15, 2022

muelli commented Apr 15, 2022

hartwork commented Apr 15, 2022 • edited Loading

hartwork left a comment

Choose a reason for hiding this comment

benzea commented Apr 15, 2022 • edited Loading

muelli commented Apr 16, 2022

hartwork commented Apr 16, 2022 • edited Loading

benzea commented Apr 16, 2022

hartwork commented Apr 16, 2022

benzea commented May 6, 2022

hartwork commented May 26, 2022

muelli commented Sep 27, 2022

Cropi commented Oct 24, 2022

hartwork commented Oct 24, 2022

muelli commented Oct 24, 2022

hartwork commented Oct 24, 2022

muelli commented Nov 5, 2022

muelli commented Nov 7, 2022 • edited Loading

muelli commented Nov 7, 2022

hartwork commented Apr 15, 2022 •

edited

Loading

benzea commented Apr 15, 2022 •

edited

Loading

hartwork commented Apr 16, 2022 •

edited

Loading

muelli commented Nov 7, 2022 •

edited

Loading