Comment 20 for bug 1748157

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