flatpak: Add USB block / allowlist #5620

GeorgesStavracas · 2023-12-07T16:26:28Z

Add '--usb' and '--no-usb' to the FlatpakContext option group.

Map these parameters to either the list, or the blocklist, of a new "USB Devices" group in the metadata key file. It looks like this:

[USB Devices]
blocked-devices=cls:01:*;
allowed-devices=vnd:0fd9+dev:0080;vnd:0fd9+dev:008c;

Flatpak itself does not use these values, they're meant to be used by e.g. XDG Desktop Portal to filter which devices the app can see through the USB portal (work in progress).

Blocked devices must always take precedence over allowed devices.

This is heavily inspired by #4083

This is an alternative to exporting portal files (see #5612)

Erick555 · 2023-12-07T16:58:21Z

Are we sure app maintainers or end users will understand what those permissions do and how to manage them? It's not quite human readable format.

GeorgesStavracas · 2023-12-07T17:26:21Z

App maintainers who write apps that use USB devices surely know what vendor id and product id are.

Users are expected to interact with portal requests, which have human-readable device names.

GeorgesStavracas · 2023-12-07T18:22:56Z

This work can be verified by building this branch of Boatswain: https://gitlab.gnome.org/World/boatswain/-/tree/gbsneto/usb-manifest

After checking out this branch, you can run the following command to build it: $ flatpak-builder --user --install --ccache --force-clean --verbose _build build-aux/com.feaneron.Boatswain

Then you can verify that this PR properly sets the USB Devices group by cat'ing the contents of .flatpak-info:

$ flatpak --user run --command=bash com.feaneron.Boatswain
$ cat /.flatpak-info

At the bottom of the file there should be the following:

[USB Devices]
allowed-devices=0fd9:008f;0fd9:0063;0fd9:0090;0fd9:0080;0fd9:0086;0fd9:006d;0fd9:0060;0fd9:006c;

If you run Boatswain with --no-usb, the argument is properly merged in the info file as well:

$ flatpak --user run --command=bash --no-usb=0fd9:008f com.feaneron.Boatswain
$ cat /.flatpak-info

[...]

[USB Devices]
blocked-devices=0fd9:008f;
allowed-devices=0fd9:008f;0fd9:0063;0fd9:0090;0fd9:0080;0fd9:0086;0fd9:006d;0fd9:0060;0fd9:006c;

doc/flatpak-build-finish.xml

common/flatpak-context.c

GeorgesStavracas · 2023-12-07T18:48:33Z

Damn, I pushed by mistake, sorry for disrupting the review

TingPing

Overall this looks good to me (pending single discussion). I think this is a better approach than #5612.

I'll leave this open for a bit for others to chime in.

TingPing · 2023-12-07T19:02:20Z

Are we sure app maintainers or end users will understand what those permissions do and how to manage them? It's not quite human readable format.

It was mentioned that usb.ids exists to map these IDs to a somewhat readable value. Frontend tools can use this in the future. However for the permission itself that isn't usable.

GeorgesStavracas · 2023-12-07T19:12:37Z

The testing steps described at #5620 (comment) still work with the last push 👍

GeorgesStavracas · 2023-12-08T18:49:32Z

The corresponding USB portal which actually uses this information can be found here: flatpak/xdg-desktop-portal#1238

Erick555 · 2023-12-08T19:42:50Z

Does the fact flatpak exposes various paths under /sys (block bus class dev devices) affects privacy/security model of usb access? i.e. could app still list available devices even without --usb permission?

GeorgesStavracas · 2023-12-08T19:47:01Z

Does the fact flatpak exposes various paths under /sys (block bus class dev devices) affects privacy/security model of usb access? i.e. could app still list available devices even without --usb permission?

That's looks a separate issue. It should be addressed separately.

hadess

I did not review the code itself, and my analysis of requirements might not be complete.

I would advise focusing first on the portal and its implementation, and its ability to authorise and revoke access to devices as a priority, as the flatpak changes only really modify device enumeration.

hadess · 2023-12-09T10:44:25Z

doc/flatpak-run.xml

+
+ <listitem><para>
+ Adds a USB device to the list of allowed devices.
+ <arg choice="plain">VENDOR_ID</arg> must be a valid vendor


Every time you mention the vendor or product IDs, you should mention in which format they will be parsed. Here it seems like the only format permitted is hex.

I tried to document the exact supported syntax for vendor and product ids, and device classes and subclasses. Does it look better now?

hadess · 2023-12-09T10:46:33Z

doc/flatpak-build-finish.xml

+ <term><option>--usb=VENDOR_ID:PRODUCT_ID</option></term>
+
+ <listitem><para>
+ Adds a USB device to the list of allowed devices.


This makes it seem like "allowed devices" will be available to the application without going through a portal. The fact that a portal is necessary needs to be mentioned. This also probably needs to be reworded because they're not going to be blocked by the portal, but not listed in the portal's enumeration instead.

I tried to mention that this is used by the USB portal in the documentation. How does it read to you now?

hadess · 2023-12-09T10:50:51Z

doc/flatpak-build-finish.xml

@@ -284,6 +284,31 @@ key=v1;v2;
 </para></listitem>
 </varlistentry>

+ <varlistentry>
+ <term><option>--usb=VENDOR_ID:PRODUCT_ID</option></term>


Having this be only available as a finish-args is close to unmaintainable for app developers with more than a few supported devices. For example, this is the list of devices supported by libfprint (and its Flatpak demo application that talks USB directly): https://fprint.freedesktop.org/supported-devices.html
It would be necessary to have the ability to import the list from an outside source, such as a plain text file, or a manifest snippet, for any serious use.

This could be done in the flatpak-builder layer?

It also strikes me as somewhat inefficient to list such large lists as part of the metadata. Do we expect a scanner app to display this entire list on the commandline when you install it?

It's probably not efficient, yeah; but I don't have a clear picture of viable options here. I mean, Flatpak could have an extra /.flatpak-usb-devices file, but it's probably going to be equally as large and, probably, equally as annoying. Another alternative is a new binary format for this file, which sounds like an overkill to me.

Is there any particular approach you'd like to see explored in this branch?

hadess · 2023-12-09T10:52:12Z

doc/flatpak-metadata.xml

+ <term><option>allowed-devices</option> (string list)</term>
+ <listitem><para>
+ List of allowed USB devices. Each element of can be composed
+ of "VENDOR_ID:PRODUCT_ID", or "VENDOR_ID:*". Available since


This doesn't seem to "allowlist" all USB devices, which would be necessary for WebUSB support in Chromium-based browsers. If devices aren't listed, they wouldn't be enumerated, and couldn't be accessed.

A common requirement for enumerating USB devices is based on the class, or interface identifiers. For example, if you wanted direct access to all devices with a "network" USB interface, this doesn't seem to allow it.

I left that out somewhat on purpose, because I wanted to think better about how to handle these different queries through the command line. So far there is:

Specific device: --usb=AAAA:BBBB

All devices from specific vendor: --usb=AAAA:*

For class and subclass queries, I can imagine the following options:

--usb=class=AA:[BB|*]

--usb-class=AA:[BB|*]

--usb=AA:[BB|*]

I personally prefer (1) and (2). However, does that mean devices and vendor queries need to be adjusted into --usb=device=AAAA:BBBB, and --usb=vendor=AAAA? It looks a bit convoluted to me.

An option for allowlisting all devices is --usb=*:*. Or a contracted form, --usb=*.

Thoughts?

I think maybe the usb selector could be made more generic.
Say, a per element prefix of dev/cls/vnd, and maybe allow combinations.
So cls:AA:*+vnd=BB. We could have a generic --usb argument taking these, or helper --usb-device=... that constructs the generic key.

Thanks for the suggestion @alexlarsson, I'll try and implement that

With this last push, I've applied this cls/vnd/dev syntax to --usb and --no-usb. Some rules apply here:

--usb=all must not have any further rules (e.g. --usb=all+vnd:0fd9 is in valid). There are tests for that.

dev rules must have a vnd rule together, but vnd is okay to not have a matching dev. e.g. --usb=vnd:0fd9 is valid, --usb=vnd:0fd9+dev:0060 is valid, but --usb=dev:0060` is invalid. There are tests for that.

cls rules can be freely applied on top of dev and vnd. There are tests for that.

Notice that I haven't yet added the helper --usb-device. If people seem satisfied with this approach, I'll be happy to add the helper 🙂

Please let me know what you think!

As I mentionnd in the other thread #5620 (comment)

I can see issues about using * in any of the command line arguments.

common/flatpak-context.c

alexlarsson · 2023-12-11T15:13:22Z

common/flatpak-context.c

+ if (!g_hash_table_contains (new->blocked_usb_devices, value))
+ return TRUE;
+ }
+


This could potentially be smarter wrt globs. I.e. don't warn when aa:bb was removed, but aa:* was added. Not sure how important this is.

It's probably good to have. I'll look into adding it.

alexlarsson · 2023-12-12T08:44:20Z

One thing i worry about here is the messaging. It looks like allowed-device means that the usb device access is allowed, but really it just means it is discoverable. We need both the cli options, the metadata key names and the part displayed during install to make this more obvious.

GeorgesStavracas · 2023-12-19T20:48:33Z

This is how the Flatpak manifest of my little Stream Deck app looks with this branch: https://gitlab.gnome.org/World/boatswain/-/merge_requests/25

I feel like it's certainly missing flatpak-builder integration in the JSON file. Happy to add support for that once we're settled with something on Flatpak itself.

Still to-do in this branch, in no particular order:

Rename the metadata keys to something more sensible - so that doesn't imply these devices will be automatically allowed or blocked, and instead that the portal will read it (flatpak: Add USB block / allowlist #5620 (comment))
Add a more robust check for whether permissions are increasing or not (flatpak: Add USB block / allowlist #5620 (comment))
How should it support large lists of devices? (flatpak: Add USB block / allowlist #5620 (comment))
How to show these devices at install time in the CLI and/or app stores? (flatpak: Add USB block / allowlist #5620 (comment))

hfiguiere · 2024-03-10T03:55:36Z

common/flatpak-context.c

@@ -1531,6 +1970,8 @@ static GOptionEntry context_options[] = {
 { "system-no-talk-name", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_system_no_talk_name_cb, N_("Don't allow app to talk to name on the system bus"), N_("DBUS_NAME") },
 { "add-policy", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_add_generic_policy_cb, N_("Add generic policy option"), N_("SUBSYSTEM.KEY=VALUE") },
 { "remove-policy", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_remove_generic_policy_cb, N_("Remove generic policy option"), N_("SUBSYSTEM.KEY=VALUE") },
+ { "usb", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_usb_cb, N_("Add USB device to allowlist"), N_("VENDOR_ID:PRODUCT_ID") },
+ { "no-usb", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_no_usb_cb, N_("Add USB device to blocklist"), N_("VENDOR_ID:PRODUCT_ID") },


we have nodevice, nosocket and nofilesystem so I think this should be nousb, for consistency.

hfiguiere · 2024-03-10T04:20:32Z

doc/flatpak-run.xml

@@ -752,6 +820,9 @@ key=v1;v2;
 <para>
 <command>$ flatpak run --command=bash org.gnome.Sdk</command>
 </para>
+ <para>
+ <command>$ flatpak run org.gnome.Boxes --no-usb=0fd9:*</command>


There is an issue using * here is that on most shell the expression is expanded.

Here on zsh:

% flatpak run org.gnome.Boxes --nousb=0fd9:* zsh: no matches found: --nousb=0fd9:*

This works though:

Suggested change

<command>$ flatpak run org.gnome.Boxes --no-usb=0fd9:*</command>

<command>$ flatpak run org.gnome.Boxes --no-usb=0fd9:\*</command>

Maybe instead the syntax could be VEND: for "all devices from vendor VEND"

hfiguiere · 2024-03-10T15:18:00Z

doc/flatpak-run.xml

+ A device class and subclass query. <arg choice="plain">DATA</arg>
+ must be in the form of <arg choice="plain">CLASS:SUBCLASS</arg> where
+ both <arg choice="plain">CLASS</arg> and <arg choice="plain">SUBCLASS</arg>
+ must valid 2-digit hexadecimal class id numbers. Alternatively,


Suggested change

must valid 2-digit hexadecimal class id numbers. Alternatively,

must be a valid 2-digit hexadecimal class id number. Alternatively,

hfiguiere · 2024-03-10T15:18:39Z

doc/flatpak-run.xml

+
+ <listitem><para>
+ A device product id query. <arg choice="plain">DATA</arg>
+ must be a valid 4-character hexadecimal product id number


Suggested change

must be a valid 4-character hexadecimal product id number

must be a valid 4-digit hexadecimal product id number

hfiguiere · 2024-03-10T15:18:53Z

doc/flatpak-run.xml

+
+ <listitem><para>
+ A device vendor id query. <arg choice="plain">DATA</arg>
+ must be a valid 4-character hexadecimal vendor id number


Suggested change

must be a valid 4-character hexadecimal vendor id number

must be a valid 4-digit hexadecimal vendor id number

hfiguiere · 2024-03-18T01:13:44Z

Also I have a concern about the syntax: I have 332 rules here for Epson Scan 2: I parsed the .rules with python. It's tedious to add to the manifest.

hfiguiere · 2024-03-18T03:05:22Z

A libgphoto2 device list is 2595 long.

hfiguiere · 2024-03-18T23:20:07Z

This original branch is at gbsneto/usb-original (for archive)

hfiguiere · 2024-03-19T03:45:24Z

rebased (a second time and added some fixes)

common/flatpak-context.c

swick · 2024-03-25T12:55:21Z

Giving access to all devices of one vendor is a mistake. If the vendor starts making a smartcard, fido device, etc, the app suddenly has access to this and there is no way for a human reviewing the manifest to figure out if all devices of a vendor makes sense for this specific app.

Add '--usb' and '--no-usb' to the FlatpakContext option group. Map these parameters to either the list, or the blocklist, of a new "USB Devices" group in the metadata key file. It looks like this: ``` [USB Devices] blocked-devices=cls:01:*; allowed-devices=vnd:0fd9+dev:0080;vnd:0fd9+dev:0080; ``` Flatpak itself does not use these values, they're meant to be used by e.g. XDG Desktop Portal to filter which devices the app can see through the USB portal (work in progress). Blocked devices must always take precedence over allowed devices. This is heavily inspired by flatpak#4083

Signed-off-by: Hubert Figuière <[email protected]>

Also added test for the empty string query Signed-off-by: Hubert Figuière <[email protected]>

Signed-off-by: Hubert Figuière <[email protected]>

GeorgesStavracas requested review from smcv, refi64 and TingPing December 7, 2023 16:26

GeorgesStavracas force-pushed the gbsneto/usb branch from e151230 to 43cff99 Compare December 7, 2023 17:23

GeorgesStavracas force-pushed the gbsneto/usb branch 3 times, most recently from 90c5dff to 284b4a4 Compare December 7, 2023 18:13

TingPing reviewed Dec 7, 2023

View reviewed changes

doc/flatpak-build-finish.xml Outdated Show resolved Hide resolved

TingPing reviewed Dec 7, 2023

View reviewed changes

common/flatpak-context.c Outdated Show resolved Hide resolved

TingPing reviewed Dec 7, 2023

View reviewed changes

common/flatpak-context.c Outdated Show resolved Hide resolved

GeorgesStavracas force-pushed the gbsneto/usb branch from 284b4a4 to b1ee7fe Compare December 7, 2023 18:47

TingPing approved these changes Dec 7, 2023

View reviewed changes

GeorgesStavracas force-pushed the gbsneto/usb branch from b1ee7fe to 45c3f26 Compare December 7, 2023 19:10

hadess requested changes Dec 9, 2023

View reviewed changes

alexlarsson reviewed Dec 11, 2023

View reviewed changes

common/flatpak-context.c Outdated Show resolved Hide resolved

alexlarsson reviewed Dec 11, 2023

View reviewed changes

GeorgesStavracas force-pushed the gbsneto/usb branch from 45c3f26 to 45afd8d Compare December 19, 2023 20:32

GeorgesStavracas mentioned this pull request Dec 19, 2023

USB portal flatpak/xdg-desktop-portal#1238

Draft

hfiguiere mentioned this pull request Feb 11, 2024

Device permission compatibility #5681

Open

2 tasks

hfiguiere reviewed Mar 10, 2024

View reviewed changes

hfiguiere force-pushed the gbsneto/usb branch 2 times, most recently from 4b791dd to e765beb Compare March 19, 2024 03:43

hfiguiere reviewed Mar 22, 2024

View reviewed changes

common/flatpak-context.c Outdated Show resolved Hide resolved

GeorgesStavracas and others added 6 commits March 26, 2024 23:10

usb: the commnd line option is 'nousb'

8294894

Signed-off-by: Hubert Figuière <[email protected]>

usb: Fix some doc grammar

c6cb90e

Signed-off-by: Hubert Figuière <[email protected]>

context: Refactor adding usb queries

29c95ed

Signed-off-by: Hubert Figuière <[email protected]>

context: parse_usb properly handle empty query

90a5a9b

Also added test for the empty string query Signed-off-by: Hubert Figuière <[email protected]>

context: refactor saving USB device lists

65bf0e5

Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the gbsneto/usb branch from 94433bb to e62d5dc Compare March 27, 2024 03:16

context: Implement device lists for usb

6773b9e

Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the gbsneto/usb branch from e62d5dc to 6773b9e Compare March 27, 2024 03:49

	<command>$ flatpak run org.gnome.Boxes --no-usb=0fd9:*</command>
	<command>$ flatpak run org.gnome.Boxes --no-usb=0fd9:\*</command>

	must valid 2-digit hexadecimal class id numbers. Alternatively,
	must be a valid 2-digit hexadecimal class id number. Alternatively,

	must be a valid 4-character hexadecimal product id number
	must be a valid 4-digit hexadecimal product id number

	must be a valid 4-character hexadecimal vendor id number
	must be a valid 4-digit hexadecimal vendor id number

flatpak: Add USB block / allowlist #5620

Are you sure you want to change the base?

flatpak: Add USB block / allowlist #5620

Conversation

GeorgesStavracas commented Dec 7, 2023 • edited

Erick555 commented Dec 7, 2023

GeorgesStavracas commented Dec 7, 2023

GeorgesStavracas commented Dec 7, 2023

GeorgesStavracas commented Dec 7, 2023

TingPing left a comment

Choose a reason for hiding this comment

TingPing commented Dec 7, 2023

GeorgesStavracas commented Dec 7, 2023

GeorgesStavracas commented Dec 8, 2023

Erick555 commented Dec 8, 2023 • edited

GeorgesStavracas commented Dec 8, 2023

hadess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlarsson commented Dec 12, 2023

GeorgesStavracas commented Dec 19, 2023

hfiguiere Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hfiguiere commented Mar 18, 2024

hfiguiere commented Mar 18, 2024

hfiguiere commented Mar 18, 2024

hfiguiere commented Mar 19, 2024

swick commented Mar 25, 2024

GeorgesStavracas commented Dec 7, 2023 •

edited

Erick555 commented Dec 8, 2023 •

edited

hfiguiere Mar 10, 2024 •

edited