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.
>
> Noted. flags=0 is good enough, yes? (I will may have to make sure first
> that all the relevant distros include this syscall already.)
flags=0 is probably good enough. I'm just worried that std::random_device
*might* fallback to rand(3) or something terrible if we're not careful.
I suggested getrandom(2) over urandom(4) because the thunderbolt
authorization interface is significantly newer than the getrandom(2)
interface (added in Linux 3.17, glibc 2.25). It just seemed likely to
be available anywhere that this software would be useful.
Thanks for your repeated help with these reviews
--
You received this bug notification because you are a member of Kernel
Packages, which is subscribed to thunderbolt-tools in Ubuntu.
Matching subscriptions: Kernel Packages
https://bugs.launchpad.net/bugs/1748157
Title:
[MIR] thunderbolt-tools
Status in thunderbolt-tools package in Ubuntu:
Confirmed
Bug description:
== 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
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/thunderbolt-tools/+bug/1748157/+subscriptions
--
Mailing list: https://launchpad.net/~kernel-packages
Post to : kernel-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kernel-packages
More help : https://help.launchpad.net/ListHelp
