[MIR] usbguard

Assigned to me for MIR review

Understanding that usbguard is a target for main, I've been running it on my laptop for a little while and can say that there is a real issue with the daemon stopping which causes all USB inserts to fail closed until the daemon is restarted. I've also suspected some external keyboard weirdness after plug/unplug/plug to be because of usbguard. I've not debugged any of this but feel like it has something to do with an udevadm trigger events, perhaps as done by snapd, perhaps by suspend/resume. I highly recommend someone really test this software's usability before we put ourselves on the hook for maintenance (I tried it a year or more ago and it was quite unusable, but seems better now, so I may be biased).

On Tue, Apr 09, 2019 at 11:33:00PM -0000, Jamie Strandboge wrote:
> Understanding that usbguard is a target for main, I've been running it
> on my laptop for a little while and can say that there is a real issue
> with the daemon stopping which causes all USB inserts to fail closed
> until the daemon is restarted. I've also suspected some external

Did you get any logs or apport crash reports when it died?

I've been running usbguard and usbguard-applet-qt since 2018-06-25 and
have really been happy with it. I've always intended to compare it against
usbauth but because usbguard has just worked flawlessly I never got around
to trying the alternatives.

Thanks

[Duplication]
Function-wise there seems to be usbauth and usbguard (thanks Seth and Jamie for adding your early experiences).
None of them is in main yet so it would be no duplication.
Picking among the two projects can be hard as they promise almost the same.
But in terms of upstream activity, commits, forks and all that USBGuard seems to win over USBAuth for now.
Therefore the choice between those to seems to be made by the community.

[Embedded sources and static linking]
- no embedded code of other projects
- it builds a static library, but that is only used for tests (make check) and not in the binary files
- no golang

[Security]
- history of CVEs = no CVEs yet
- does not use webkit1,2
- does not use lib*v8 directly
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrates arbitrary javascript into the desktop
- does not deals with system authentication (eg, pam), etc)

Not so good
- runs a daemon as root
- parses data formats (it has a API on dbus)
- opens a port (well a Dbus listener to be specific)
- the program deals with (device) authentication

A few open bugs indicate known security issues which should be
fixed before promotion:
https://github.com/USBGuard/usbguard/issues/277
https://github.com/USBGuard/usbguard/issues/231

Therefore this will need an in depth security review for sure.

[Common blockers]
- builds fine at the moment
- test suite runs on build
- no python code
- translations are in src/GUI.Qt/...

Not so good:
- No autopkgtest (e.g. if kernel USB handling breaks it)
  OTOH hard to implement as you need to fake USB devices (maybe qemu can help).
  There is in 7.3 a "umockdev based device manager capable of simulating
  devices based on umockdev-record files"
- The Team (according to the report) has a bug subscriber (security),
  Security might be subscribed, but I don't think they own/maintain the package.
  They might just be subscribed just for watching, I think Desktop will need to
  own this one if we promote it (up for discussion between security and desktop).

[Packaging red flags]
- no Ubuntu delta
- watch file present
- upstream as well as Debian seems regular and active in regard to updates
- Lintian is ok with the package
- d/rules seems to be short an easy
- not using Built-Using
- no golang package

Not so good:
- lacks symbols tracking for libusbguard.so.0
- the current release (since July last year) 7.4 is not yet packaged

[Upstream red flags]
- bugs are 5 Debian, 5 Ubuntu and 60 open / 142 closed upstream
- no usage of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no Dependency on webkit, qtwebkit, seed or libgoa-*
- no Embedded source copies

Not so good - is the status on open bug in general atm:
- there are a few crashes in the Ubuntu bug tracker and as we
  learned crashing the daemon means disconnected devices.
- The upstream bug tracker has two concerning bugs that might need to be
  addressed before promoting it being
  https://github.com/USBGuard/usbguard/issues/229 (issue with duplicate IDs)
  https://github.com/USBGuard/usbguard/issues/279 (linux 5.0 incompatibility)
- build has a concerning warning
  dpkg-shlibdeps: warning: debian/libusbguard0/u...

As an addendum to the TODOs, after purging it it should no more block new devices (as it does right now). Which is not fun since the tools to make them allowed are no more installed.

Thanks for the review Christian, It seems we have some work to do there, we will plan some of that for next cycle.

Note that Desktop has not been a driver for the feature request, but we got first pinged about it by security. GNOME might be integrating it next cycle though which would create a requirement from the desktop side (probably optional). In any case security/desktop needs to discuss ownership indeed.

> > Understanding that usbguard is a target for main, I've been running it
> > on my laptop for a little while and can say that there is a real issue
> > with the daemon stopping which causes all USB inserts to fail closed
> > until the daemon is restarted. I've also suspected some external
>
> Did you get any logs or apport crash reports when it died?

Ok, it just happened to me when plugging in a usb drive. It didn't crash:

$ systemctl status usbguard
● usbguard.service - USBGuard daemon
   Loaded: loaded (/lib/systemd/system/usbguard.service; enabled; vendor preset: enab
   Active: active (running) since Mon 2019-04-08 06:47:50 CDT; 4 days ago
     Docs: man:usbguard-daemon(8)
 Main PID: 2404 (usbguard-daemon)
    Tasks: 2 (limit: 4915)
   Memory: 8.9M
   CGroup: /system.slice/usbguard.service
           └─2404 /usr/sbin/usbguard-daemon -f -s -c /etc/usbguard/usbguard-daemon.co

Apr 12 06:59:48 foo usbguard-daemon[2404]: ueventProcessRead: failed to read pen
Apr 12 06:59:48 foo usbguard-daemon[2404]: UEventDeviceManager thread: UEvent de
Apr 12 07:45:47 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 07:45:50 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 10:48:36 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 10:48:37 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 10:49:24 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 10:49:25 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 13:33:10 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=
Apr 12 13:33:11 foo systemd[1]: /lib/systemd/system/usbguard.service:7: PIDFile=

but did seem to stop processing events:
Apr 12 06:59:48 foo usbguard-daemon[2404]: ueventProcessRead: failed to read pending uevent: rc=-1 errno=105
Apr 12 06:59:48 foo usbguard-daemon[2404]: UEventDeviceManager thread: UEvent device manager: recvmsg: No buffer space available

The error happened again after a snapd upgrade. I suspect it isn't handling the udev trigger events that snapd does particularly well (even though that is supposed to be safe).

Oh, cool, a starting point to work with! Any chance an apt-get install --reinstall would trip it?

Thanks

I reviewed usbguard 0.7.4+ds-1 as checked into eoan. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

usbguard consists of a daemon which manages the authorization of new USB
devices via udev events. It provides an IPC interface (which by default is
only accessible by the root user but the Debian packaging adds the wheel
and usbguard groups) which allows it to be controlled - this uses protobuf
for serialization. A separate daemon, usbguard-dbus provides an interface
over the DBus system bus which uses PolicyKit to ensure only administrator
users can access privileged methods (such as authorizing new devices or
changing the overall policy etc). Also provides an audit backend so that it
can provide audit events. Includes support for restricting itself with a
built-in seccomp policy which is enabled in the Debian package.

A separate binary usbguard provides a CLI to interact with the daemon via
the IPC interface.

Finally, a separate binary package, usbguard-applet-qt provides a GUI to
control the daemon using it's own IPC interface. Users who use the applet
are assumed to be in the wheel or usbguard groups to enable them to control
the daemon.

- CVE History:
  - None
- Build-Depends
  - Huge list including Qt, GLib, DBus, PolicyKit, libseccomp, libaudit and
    docbook,xsltproc,pandoc etc for documentation generation
- pre/post inst/rm scripts
  - postinst creates empty copies of and fixes permissions on
    /etc/usbguard/rules.conf and /etc/usbguard/usbguard-daemon.conf so it
    is only accessible by the owner
    - Is this normal? I note this is flagged as a WARNING in csp-source.txt
      by uaudit.
  - postrm removes the above two configuration files and the configuration
    dir /etc/usbguard if empty.
- init scripts
  - None
- systemd units
  - usbguard daemon system service - runs usbguard-daemon with the default
    configuration and restarts on failure
  - usbguard-dbus system service - runs usbguard-dbus on DBus system bus
    with restart on failure
- dbus services
  - org.usbguard.system service
    - provides an interface to interact with
- setuid binaries
  - None
- binaries in PATH
  - /usr/bin/usbguard
  - /usr/sbin/usbguard-daemon
  - /usr/sbin/usbguard-dbus
- sudo fragments
  - None
- udev rules
  - None
- unit tests
  - Includes some unit tests which run during the build
  - No autopkgtests
- cron jobs
  - None
- Build logs:
  - No significant warnings / errors in the build log

- Processes spawned
  - usbguard watch command supports spawning a process as specified on the
    command-line - so no direct chance for shell command-injection here
- Memory management
  - There is not a lot of dynamic memory management - what there is appears
    to be quite clean and careful
- File IO
  - The configuration file for the daemon is read carefully and the path to
    this is hardcoded if NOT overridden by a command-line parameter
  - Files in sysfs are written to to authorize devices
  - umask is defensively set even though in general not many files are
    created by usbguard.
- Logging
  - Logging is careful
- Environment variable usage
  - Uses USBGUARD_DEBUG to enable more logging output...

Thanks for the review, I'm going to try to help moving that a bit forward.

> Not so good:
> - lacks symbols tracking for libusbguard.so.0

I've forwarded that to Debian with a patch now
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931380

> - the current release (since July last year) 7.4 is not yet packaged

The update is in Debian and Ubuntu since Disco.

GNOME is reviewing the feature integration to their desktop but side they would welcome extra testing. I will try to set up a ppa for that once they are done with the current round of code reviews.

The usbguard upstream bugs mentioned before didn't get much traction so far though...

> dpkg-shlibdeps: warning: debian/libusbguard0/usr/lib/x86_64-linux-gnu/usbguard
> /libusbguard.so.0.0.0 contains an unresolvable reference to symbol pthread_create: it's
> probably a plugin
> That is concerning as in this case it is not meant to be a plugin.
> Missing dependency?

That issue seems to have been fixed, no such warning in the current version build logs/in a local build

sorry ignore that about the warning, it's still there, reported to debian

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931381

> - lacks symbols tracking for libusbguard.so.0

The Debian maintainer responded to that saying

'Usbguard upstream does not give any guarantees for backwards
compatibility for the usbguard library as long as its not version 1.0.
Thats also why the library is installed to
/usr/share/<triplet>/usbguard, to make clear that it is, at least for
now, an internal library.
Do you think there are benefits in shipping the symbols file regardless
of the instability?'

I agree that if upstream doesn't consider it stable and ship in a private directory there is probably little point having the .symbols file for it

Hi Seb,
thanks for putting effort into this in general - I know it can be rather time consuming to clean up those review findings. You asked me to look again into the aspect of the library lacking a symbols file due to the new info presented above.

The file is in the normal directory as all other libs (on the current version in Eoan):
  /usr/lib/x86_64-linux-gnu/usbguard/libusbguard.so.0.0.0
So the placing or naming (0.0.0) of the lib is no "excuse" IMHO.

If the lib is moving fast or is even considered unstable it would be almost even more important to track symbols to know what changed for consumers. But if it really is
a) only internal
b) not meant to be used by other programs (so far)
c) symbols tracking is just a burden
Why not making it a static link and not ship the lib at all?
I haven't checked the build system usbguard uses if that is easy or hard in this case.
But shipping a lib that is not meant to be used as a lib sounds like calling for problems.

> The file is in the normal directory as all other libs (on the current version in Eoan):
> /usr/lib/x86_64-linux-gnu/usbguard/libusbguard.so.0.0.0

Well, the fact that it's in a subdir there and not directly in /usr/lib/x86_64-linux-gnu means it's not in the standard LD_LIBRARY_PATH and wouldn't be available to other programs unless they do tweaks to go look in that location no?

I'm unsure why upstream is building a shared library but I will try to figure out, I guess it might be because they want to share some functions between their different binaries while keeping those smaller

> > The file is in the normal directory as all other libs (on the current version in Eoan):
> > /usr/lib/x86_64-linux-gnu/usbguard/libusbguard.so.0.0.0
>
> Well, the fact that it's in a subdir there and not directly in
> /usr/lib/x86_64-linux-gnu means it's not in the standard LD_LIBRARY_PATH
> and wouldn't be available to other programs unless they do tweaks to go
> look in that location no?

Yes you are right on that.
Please do not invest too much time on the symbols, compared to the
other findings this is a minor nit-pick and while not perfect it
really isn't critical.
The bigger issues with replugged devices not being recognized or
issues with the service dying blocking everything are much much more
important.

I found the cause of usbguard becoming unresponsive and filed https://bugs.launchpad.net/usbguard/+bug/1855189 (with https://github.com/USBGuard/usbguard/issues/349).

0.7.5 does not fix bug #1855189 (and code inspection suggests 0.7.6 is also affected). IMO, bug #1855189 needs to be fixed as part of main inclusion.

From https://bugs.launchpad.net/ubuntu/+source/usbguard/+bug/1855189/comments/2: "FYI, upstream committed https://github.com/USBGuard/usbguard/pull/378 to address this issue. It is essentially what I mentioned in the bug description."

If we apply that patch to our usbguard packages, I have no concerns. Alex gave security team ACK so I think, at least from the security team's POV, this can be promoted once the patch is applied.

Thanks Jamie!

From the MIR Team we had a list of todos in comment #4:
Some are resolved already around the lib/symbols as well as the formerly open security issues being resolved in the new release.

What is left seems to be:
- Change the current on-install default to block all but host controllers
  to "allow all that is attached on install".
  That can either be done in a postinst or by changing the base
  PresentDevicePolicy config.
- to avoid a install hickup maybe even set PresentDevicePolicy=allow, then
  set all to be allowed and only then reset to "apply-policy"
- set IPCAllowedGroups=root to also allow sudo group
- Fix plug/unplug/plug issues
- implementing an autopkgtest might be required (see current issues with linux 5.0 upstream)

In general adding it to groovy at the current state also feels much safer than when it was late in the focal cycle.

And maybe some of the things above are also already resolved.
@Desktop-team would you mind rechecking if those are also handled already (and if not tackle them or explain why they are not needed).

@Rex - hi I see you added a priority tag.

This is waiting for further input/explanation on the open tasks/questions that I have re-summarized on comment #21. You could either work with the Desktop team which will own this or do it on your own, but the task is to track those open things down one by one and for each of them either do:
a) implement them
b) explain that they got implemented by the existing uploads
b) explain that they are covered by other now existing bits (e.g. if there was a different solution that achieves the same)
c) explain why it is not needed

Once these are clarified this would be ready to be promoted (after a change to pull it in is made).

P.S. nobody yet replied confirming that security wants to own continuous maintenance of this (security is the currently subscribed team) or if that needs to be passed to e.g. Desktop for that? I asked that way back in comment #4, but so far no one confirmed/denied.

Sorry for the lack of status update here, we probably own one from a desktop perspective. The feature is still something we would like to see landing but it has disruption potential and Desktop is currently busy with other things, I don't think we have the capacity to deal with the integration and fallouts of usbguard this cycle. We might try to get that done next cycle but it's not sure at this point, I will update the bug when we revisit our backlog and determine what we will be working on in the next cycles.

Thanks for letting us know, in that case (until really ready) let us mark this incomplete.

Once re-started please add a team subscriber, answer the questions I outlined and set it back to new.

There has been not further update for too long, for now we consider it invalid.
Feel free to re-open if there is effort backing it up and motivation to bring it to main.