[MIR] thunderbolt-tools

[Colin Ian King]

== Overview ==

Intel Thunderbolt userspace components provides components for using Intel Thunderbolt controllers with security level features. Thunderbolt™ technology is a transformational high-speed, dual protocol I/O that provides unmatched performance with up to 40Gbps bi-directional transfer speeds. It provides flexibility and simplicity by supporting both data (PCIe, USB3.1) and video (DisplayPort) on a single cable connection that can daisy-chain up to six devices.

[ See https://github.com/intel/thunderbolt-software-user-space ]

== Answers to UbuntuMainInclusionRequirements ==

= Requirements =

1. Availability
   Package is in universe: https://launchpad.net/ubuntu/+source/thunderbolt-tools

2. Rationale
   Package a device enabler for users with Thunderbolt technology

3. Security:
   No security issues exposed so far. However, the tools have only been in Ubuntu since
   2017-12-09, so this currently is less than the 90 days threshold.

4. Quality assurance:
   * Manual is provided
   * No debconf questions higher than medium
   * No major outstanding bugs. I'm also helping Intel fix issues that I'm finding with
     static analysis tools such as scan-build, cppcheck and CoverityScan
     Bugs outstanding:
       #883857 please backport for stretch-backports
       #882525 thunderbolt-tools: FTBFS on kFreeBSD: _ZN5boost6system15system_categoryEv undefined
         - I can fix this, but it makes no sense to run on kFreeBSD
   * Exotic Hardware: Only Thunderbolt supported H/W is required, this is an industry standard
     and the support for the tools are in the 4.13+ kernels
   * No Test Suite shipped with the package
   * Does not rely on obsolete or demoted packages

5. UI standards:
   * This is a CLI tool. Tool has normal CLI style short help and man pages
   * No desktop file required as it is a CLI tool.

6. Binary Dependencies:
 libboost-dev (main)
 libboost-filesystem-dev (main)
 libboost-program-options-dev (main)
 udev (main)

7. Standards compliance:
   lintian clean and meets the FHS + Debian Policy standards to the best of my knowledge

8. Maintenance
   * Package owning team: The Ubuntu Kernel Team
   * Debian package maintained by Colin Ian King (myself from the Kernel Team)

9. Background Information
   The user-space components implement device approval support:

   a. Easier interaction with the kernel module for approving connected devices.
   b. ACL for auto-approving devices white-listed by the user.

   Tools provided by this package:

    tbtacl - triggered by udev (see the udev rules in tbtacl.rules). It auto-approves devices that are found in ACL.

    tbtadm - user-facing CLI tool. It provides operations for device approval, handling the ACL and more.

    The user-space components operate in coordination with the upstream Thunderbolt kernel driver (found in v4.13) to provide the Thunderbolt functionalities. These components are NOT compatible with the old out-of-tree Thunderbolt kernel module.

= Security checks =

  http://cve.mitre.org/cve/cve.html: Search in the National Vulnerability Database using the package as a keyword
  * No CVEs found

  http://secunia.com/advisories/search/: search for the package as a keyword
  * No security advisories found

  Ubuntu CVE Tracker
    http://people.ubuntu.com/~ubuntu-security/cve/main.html
    * No
    http://people.ubuntu.com/~ubuntu-security/cve/universe.html
    * No
    http://people.ubuntu.com/~ubuntu-security/cve/partner.html
    * No

    Check for security relevant binaries. If any are present, this
    requires a more in-depth security review.

    Executables which have the suid or sgid bit set.
      * Not applicable

    Executables in /sbin, /usr/sbin.
      * None in these paths

    Packages which install daemons (/etc/init.d/*)
      * No

    Packages which open privileged ports (ports < 1024).
      * No

     Add-ons and plugins to security-sensitive software (filters,
     scanners, UI skins, etc)
      * This does exec tbtacl from udev with new udev rules, so this
        needs security checking

[Ubuntu Kernel Bot (ubuntu-kernel-bot)]

This bug is missing log files that will aid in diagnosing the problem. While running an Ubuntu kernel (not a mainline or third-party kernel) please enter the following command in a terminal window:

apport-collect 1748157

and then change the status of the bug to 'Confirmed'.

If, due to the nature of the issue you have encountered, you are unable to run this command, please add a comment stating that fact and change the bug status to 'Confirmed'.

This change has been made by an automated script, maintained by the Ubuntu Kernel Team.

[Mario Limonciello (superm1)]

Logs N/A for this type of bug.

[Mathieu Trudel-Lapierre (cyphermox)]

Assigning to security as per the description...

[Seth Arnold (seth-arnold)]

This appears to be duplicating functionality from bolt: https://bugs.launchpad.net/ubuntu/+source/bolt/+bug/1752056

Are both packages truly desired?

Thanks

[Colin Ian King (colin-king)]

Seems like "bolt" was handled before my MIR, which is perplexing as my request was for a Dell requested package that was filed earlier back in 2018-02-08 where as bolt was filed in 2018-02-27. I'm not sure about bolt, this seems to be a Ubuntu package and not one that is in Debian.

@Mario, any comments since I'm kind of driving this for Dell for you?

[Colin Ian King (colin-king)]

@Seth, thunderbolt-tools was requested by Dell to be part of Ubuntu Bionic, I believe it's for some specific Dell laptops, but I can't provide any more specific details as Mario from Dell is on vacation right now.

[Seth Arnold (seth-arnold)]

Will, can you provide some advice?

Thanks

[Will Cooke (willcooke)]

AFAIK, Bolt (https://christian.kellner.me/2017/12/14/introducing-bolt-thunderbolt-3-security-levels-for-gnulinux/) will allow us to provide UI for managing security levels for Thunderbolt connected devices. It uses the Intel provided sysfs devices for configuration. If thunderbolt-tools also uses those devices, then the two should be able to co-exist, but I can see that there might end up being a fight over who owns the settings. From a desktop perspective we want the UI provided by the GNOME Shell extension and Bolt so that users don't have to drop to a command line to configure their devices.

I don't have any suitable hardware to test coexistence of both thunderbolt-tools and Bolt.

If the two can coexist, then I would like to use Bolt on the desktop for the ease of use. If not, and if Mario is happy, then just providing thunderbolt-tools will be more appropriate for generic Ubuntu since it will be usable on servers etc. It seems that thunderbolt-tools is more mature.

I think we need Mario to make a call on which one we should be using.

[Yehezkel Bernat (yehezkelshb)]

My 2 cents:
I think bolt and thunderbolt-tools must agree on the ACL (device whitelist for auto-approve on next connection) location and format and then I assume they can co-exist happily without stepping on each other's toes. Even if both try to authorize the same device on connection, it shouldn't matter much as the final result is that the device is authorized and ready to use.

[Mario Limonciello (superm1)]

At the time Colin and I discussed getting thunderbolt-tools into Debian and then eventually into Ubuntu (& of course main via this MIR) bolt's GUI wasn't available yet and bolt was still under some pretty heavy development.

From a client (general purpose laptop or desktop) system perspective it's better to offer something with a GUI, but there are other features that thunderbolt-tools offers that aren't in bolt as well. For example thunderbolt-tools provides some udev rules to assist with starting up thunderbolt networking in the proper situations and recently adopted some features for controlling what happens with the boot time ACL.

They are both great solutions that will continue to adopt different features at a different rate. My opinion is that both should be offered in main, but bolt and bolt GUI should probably be the one that gets seeded for Ubuntu desktop.

[Seth Arnold (seth-arnold)]

Mario, thanks for the feedback. I'll try to get to the Intel Thunderbolt tools in time for the Bionic release.

Yehezkel, any chance you can communicate your desire to upstream Bolt folks? The github issues for Bolt appears to have been disabled.

Thanks

[Yehezkel Bernat (yehezkelshb)]

Seth,
It was disabled as the project is now handled at freedesktop's GitLab.
I opened an issue there: https://github.com/intel/thunderbolt-software-user-space/issues/60

[Yehezkel Bernat (yehezkelshb)]

(Sorry, I meant: https://gitlab.freedesktop.org/bolt/bolt/issues/78)

[Sebastien Bacher (seb128)]

Bolt upstream has contacted the thunderbolt-tools upstream about using the same ACL before starting writting their code but never got a reply on the topic apparently, let's see what they say but bolt is providing desktop integration and a command line utility and they plan to have the networking part integrated to network-manager so it should be a full solution for Ubuntu Desktop installations.

[Seth Arnold (seth-arnold)]

One of the udev rules files appears to be untranslated:

./thunderbolt-tools_0.9.3-1_amd64/lib/udev/rules.d/tbtacl.rules:
# Thunderbolt udev rules for ACL (device auto approval)
SUBSYSTEM=="thunderbolt" ENV{DEVTYPE}=="thunderbolt_device" ACTION=="add" ATTR{authorized}=="0" RUN+="@UDEV_BIN_DIR@/tbtacl add $devpath"
SUBSYSTEM=="thunderbolt" ENV{DEVTYPE}=="thunderbolt_device" ACTION=="change" ATTR{authorized}!="0" RUN+="@UDEV_BIN_DIR@/tbtacl change $devpath"

The proper contents are in .../60-tbtacl.rules so probably the fix is easy.

Thanks

[Colin Ian King (colin-king)]

The above issue is reported against bug 1761757

[Seth Arnold (seth-arnold)]

I reviewed thunderbolt-tools version 0.9.3-1 as checked into bionic. This
shouldn't be considered a full security audit but rather a quick gauge of
maintainability.

- No CVEs in our database
- thunderbolt-tools provides a simple interface to authorize thunderbolt
  devices that are being added to a computer, since thunderbolt devices
  have immense control over the safety of the system
- Build-Depends: debhelper, libboost-dev, libboost-filesystem-dev,
  libboost-program-options-dev, cmake, pkg-config, udev, txt2tags
- Does not daemonize; udev hook scripts are used
- No pre/post inst/rm scripts
- No initscript / systemd unit files
- No DBus services
- No setuid files
- /usr/bin/tbtadm added to the PATH
- No sudo fragments
- udev rules -- appear to be configured for works-by-default behaviour,
  some examples on how to configure for authorization-required would be
  nice
- No tests, a bit unfortunate
- No cronjobs
- Clean build logs

- No subprocesses spawned
- C++ RAII memory management
- File IO done via RAII-C++ classes, not exactly obvious when it happens
- Some C++ exceptions, some C++ iostream
- No environment variable use
- The only privileged operations are file writes
- No cryptography
- No network connections
- No privileged portions of code
- No temporary files
- No WebKit
- No JavaScript
- No PolicyKit
- Clean cppcheck

thunderbolt-tools is short and sweet authorization tool. It's written in
modern C++, looks careful, and shouldn't be an undue maintenance burden.

It uses std::random_device for security uses -- I believe this is safe but
direct use of getrandom(2) would not have questions about underlying C++
library implementation choices.

Security team ACK for promoting thunderbolt-tools to main once the extra
udev configuration file is sorted out.

Thanks

[Yehezkel Bernat (yehezkelshb)]

Thanks for the review!

I'd like to get a bit more details about some of the comments to understand what steps I can take to improve it. Please let me know if you prefer to take this discussion to another medium.

> - udev rules -- appear to be configured for works-by-default behaviour,
> some examples on how to configure for authorization-required would be
> nice

I'm not sure I understand what examples and what configuration this comment refers to.

> - No tests, a bit unfortunate

This is about the package or the upstream project? Upstream we have some tests (using umockdev). See here for CI:
https://travis-ci.org/intel/thunderbolt-software-user-space

> - File IO done via RAII-C++ classes, not exactly obvious when it happens

Any specific question that I can answer (or maybe even document in the code)?

> - No PolicyKit

My assumption (and is mentioned here and there in the code) is that eventually the tool will be better if it does start using PolicyKit for the privileged actions. It'd be nice to here what do you think about it or if there are guidelines from the distro on where, when and how to use it.

> It uses std::random_device for security uses -- I believe this is safe but
> direct use of getrandom(2) would not have questions about underlying C++
> library implementation choices.

Noted. flags=0 is good enough, yes?
(I will may have to make sure first that all the relevant distros include this syscall already.)

Thanks again!

[Colin Ian King (colin-king)]

@Seth, Thanks for the MIR review. I removed the duplicated udev rules - bug 1762187 so I think we're now good to go.

[Seth Arnold (seth-arnold)]

On Sat, Apr 07, 2018 at 10:09:29PM -0000, Yehezkel Bernat wrote:
> I'd like to get a bit more details about some of the comments to
> understand what steps I can take to improve it. Please let me know if
> you prefer to take this discussion to another medium.

Hello Yehezkel,

> > - udev rules -- appear to be configured for works-by-default behaviour,
> > some examples on how to configure for authorization-required would be
> > nice
>
> I'm not sure I understand what examples and what configuration this
> comment refers to.

The udev rules that are shipped with the package look to me like they'll
automatically add every device that is plugged in to the ACLs:

$ cat ./thunderbolt-tools_0.9.3-1_amd64/lib/udev/rules.d/60-tbtacl.rules
# Thunderbolt udev rules for ACL (device auto approval)
SUBSYSTEM=="thunderbolt" ENV{DEVTYPE}=="thunderbolt_device" ACTION=="add" ATTR{authorized}=="0" RUN+="/lib/udev/tbtacl add $devpath"
SUBSYSTEM=="thunderbolt" ENV{DEVTYPE}=="thunderbolt_device" ACTION=="change" ATTR{authorized}!="0" RUN+="/lib/udev/tbtacl change $devpath"

Did I get the interpretation of these rules correct?

> > - No tests, a bit unfortunate
>
> This is about the package or the upstream project? Upstream we have some
> tests (using umockdev). See here for CI:
> https://travis-ci.org/intel/thunderbolt-software-user-space

Very nice.

This was specifically about tests that could be run during the build, and
fail the build if they fail, so that the security team (or SRU team) could
have some confidence that modifications as necessary don't have unintended
consequences. Upstream CI is wonderful but doesn't help us prepare
security updates five years later.

> > - File IO done via RAII-C++ classes, not exactly obvious when it happens
>
> Any specific question that I can answer (or maybe even document in the
> code)?

No, nothing needs to be done about this; it's just that for C-centric
folks like me accustomed to looking for open(), openat(), and fopen()
calls, now we should be looking for "/" (operator/()) as the best
indicator of file IO. It just takes some getting used to. ('File'
doesn't always work, as sometimes the type is 'auto'.)

> > - No PolicyKit
>
> My assumption (and is mentioned here and there in the code) is that
> eventually the tool will be better if it does start using PolicyKit for
> the privileged actions. It'd be nice to here what do you think about it
> or if there are guidelines from the distro on where, when and how to use
> it.

I really loved the simplicity of life here *without* PolicyKit. Some
PolicyKit APIs are unsafe and others are assumed safe, and figuring out
which is which takes time. PolicyKit complicates a codebase. I would
rather use thunderbolt-tools than bolt due to this simplicity.

Users who want PolicyKit integration would probably be happy with bolt.

So I suggest to leave thunderbolt-tools simple and skip PolicyKit. I
think we gain quite a lot with one tool with PolicyKit and one tool
without PolicyKit.

> > It uses std::random_device for security uses -- I believe this is safe but
> > direct use of getrandom(2) would not have questions about underlying C++
> > library implementation choices.
> ...

[Mario Limonciello (superm1)]

> Did I get the interpretation of these rules correct?
At a glance it looks like what you thought, but dig a little deeper.
1. The rule calls tbtacl add (https://github.com/intel/thunderbolt-software-user-space/blob/master/tbtacl/tbtacl.rules.in#L2)
2. tbtacl add calls authorize: https://github.com/intel/thunderbolt-software-user-space/blob/master/tbtacl/tbtacl.in#L117
3. Authorize looks if it's in the ACL: https://github.com/intel/thunderbolt-software-user-space/blob/master/tbtacl/tbtacl.in#L66

If it's not in the ACL it exits.

[Seth Arnold (seth-arnold)]

On Tue, Apr 10, 2018 at 05:12:22AM -0000, Mario Limonciello wrote:
> > Did I get the interpretation of these rules correct?
> At a glance it looks like what you thought, but dig a little deeper.
> If it's not in the ACL it exits.

Ah! Thanks Mario!

[Matthias Klose (doko)]

reconfirmed with Seth that this is good to go. Please add it to a seed or as a dependency to some package.

[Colin Ian King (colin-king)]

@ping, has this MIR been completed?

[Christian Ehrhardt  (paelzer)]

@cking - yes the comments by Seth and the later update of Doko a year ago are clear.
This is ready to land just someone needs to add a dependency or seed change to pull it in.

[Christian Ehrhardt  (paelzer)]

Per [1] this means "In Progress" until such a seed/dependency change is done.

[1]: https://wiki.ubuntu.com/MIRTeam#Process_states