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

[John Johansen]

On 02/11/2018 02:42 AM, Vincas Dargis wrote:
> 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.
> 
yes that will happen. We also are trying to walk the fine line of providing some benefit without making people frustrated resulting in users just disabling the additional security.

> This will be better when (uhm, if?) overriding denies will be implemented though. `nvidia-strict` would deny, `nvidia` include and override to allow.
> 
sure but I am very wary of policy using overrides. It just makes policy harder to understand. Generally I have been of the opinion that denies should not be used in the includes, and having overrides doesn't really change it, as they just make policy harder to understand. I know there are exceptions but I really worried that policy is becoming intractable.

One potential solution is to wrap the deny in a conditional and allow turning it on/off with a tunable, because its easier to have a gui to toggle some conditionals than it is to edit a profile. Toggling conditionals can also work better with debian packaging which doesn't particularly like files being modified.


>> 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] :)

yep, as best we can within constraints, unfortunately "breaking" users generally means we get turned off, not that applications get fix :(

> 
>> 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?

we could, but we would need a better suggestion (probably something in /var/) and they would still want to support the current path as that is something people are already using

> 
> 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?
> 
yes there are plans to improve control of shared memory, atm its a vague pipe dream (ie not really designed) because there are so many other items in front of it. As for not actually mmapping a real file, that very much depends on how things are implemented.

> 
>>> 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.
>>
ugh, this is problematic, partly because our profiling tools currently strip out comments.

>>>
>>>> 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.

The third option is wrapping the strict portions in a conditional, there is a single include and the profile then just has to set a single variable to get the strict behavior

> 
> 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
> 

I'm not against the plan, I do worry about being too strict causing problems, and I think the tunable + conditional might (I am not sure yet) a better way to go