[apparmor] Note: NVIDIA drivers are mapping user-writable files by default

[Vincas Dargis]

On 2/8/18 11:25 PM, Jamie Strandboge wrote:
>> There is a choice to deny it.
> 
> Of course. My point was that an nvidia user of the profiled application
> is going to expect 3d acceleration from the drivers so a profile that
> is meant to work with nvidia should do that (but see below where I
> respond to your upstream docs). An admin or profile author can choose
> to use nvidia-strict, nvidia, neither, etc.
I'm afraid lazy profile writer might just include `nvidia` without 
needing to write `deny @{HOME}/#[0-9]*` manually. `aa-logprof` probably 
will suggest `nvidia` by default.

This will be better when (uhm, if?) overriding denies will be 
implemented though. `nvidia-strict` would deny, `nvidia` include and 
override to allow.

> For the named shared memory files, yes this is potentially an issue,
> but an exploitable issue would be a bug in the attacked application.

Well, AppArmor is promoted to provide protection from bugs: "enforcing 
good behavior and preventing even unknown application flaws from being 
exploited" [0] :)

> Applications using shared memory will use an unpredictable path to open
> with O_CREAT|O_EXCL, then unlink the file, then pass the fd to the
> processes it cares about coordinating with (look in /dev/shm now-- you
> won't see anything, as opposed to `sudo lsof|grep '/shm/'` which should
> show a lot if you have a browser open).
> 
> The paths chosen by the nvidia driver are not ideal, but in practice,
> barring bugs, it is mainly that there isn't perfect isolation between
> applications (ie, app A and app B can coordinate to share with the
> globbed file rules). The other thing it allows is that if an attacker
> took control of the profiled application, it could then write out files
> that could be mmap'd PROT_EXEC by the application. This is a valid
> point, but if you have that much control of the application you don't
> need to write out those files.
> 
> My point is that the abstractions are meant to be generally usable and
> erring on the side of security in an abstraction that is clearly meant
> to work with something such that it breaks that something, isn't useful
> to anyone. I also mentioned that these would need to be investigated,
> which you've done below, which I responded to.

About "The paths chosen by the nvidia driver are not ideal" - could we 
(try to) communicate with NVIDIA to suggest better solution? What could 
that be?

Meanwhile, if I understood correctly, improvement from AppArmor side 
could be introduced that would allow to specify rules for shared memory 
"stuff" only? So that allowing `owner [shared?] @{HOME}/#[0-9] rwm` 
would not actually allow to mmap "real" file?


>> I'm betting on this because it doesn't seem we have a lot of
>> graphic-intensive applications with AppArmor profile that might feel
>> a
>> hit due to disabled optional NVIDIA optimizations.
> 
> The AppArmor project doesn't nor does Ubuntu, Debian, OpenSUSE, Tails,
> etc, but there are consumers of apparmor that absolutely do (ie, there
> are a lot of games packaged as snaps, and snappy uses AppArmor).

True.

> With this information, I would then suggest adding comments to the
> nvidia abstraction on the above. Then someone profiling blender could
> see that and add the necessary rules to the blender profile for the
> optimizations.
> 
> So yes, whenever we get deny overrides, we could add deny rules to the
> nvidia abstraction to silence the denials, but not sooner IMO.
> 
>>
>>> I would instead suggest that *if* we wanted
>>> to split these accesses, we do something more like:
>>>
>>>    * abstractions/nvidia-strict: all the safe accesses
>>>    * abstractions/nvidia: #include nvidia-strict plus less desirable
>>>      accesses
>>>
>>> In this manner, profiles continue to work with new nvidia accesses,
>>> but
>>>    profilers can choose to do something more strict if they desire.
>>
>> So if profile includes `<abstractions/nvidia-strict>`, they would
>> have
>> to add these
>> `deny @{HOME}/#[0-8]* rwm`,
>> `deny /tmp/.gl* rwm`
>> manually, in addition to the include?
> 
> With what is available today, yes, but see above.

I guess I have to agree that probably the best we can do.

So to wrap up, plan would be:

1. Move `abstactions/nvidia` content into `nvidia-strict`. 
`nvidia-strict` should have comment that it does not provide some NVIDIA 
optimizations and some `deny` rules are recommended to be added 
manually. Else, suggest to use `nvidia` if really needed.

2. Create new `abstractions/nvidia` that includes `nvidia-strict`. Add a 
_big_ warning documenting that it provides NVIDIA optimization that 
could potentially reduce security, suggest to use `nvidia-strict` for 
non-performance-critical applications instead.

In the future:

3. Deny these optimizations in `nvidia-strict` by default, add overrides 
into `nvidia` abstraction when that's becomes possible.

ACK?

Any more alternatives?

[0] https://gitlab.com/apparmor/apparmor/wikis/home#description