[apparmor] stack and stack_onexec semantics again

[John Johansen]

On 03/09/2016 07:35 AM, Tyler Hicks wrote:
> On 2016-03-08 23:03:40, John Johansen wrote:
>> On 03/08/2016 10:22 PM, Tyler Hicks wrote:
>>
>> << snip >>
>>
>>
>>>>> The stack_onexec api however is a different story dependent on when/how
>>>>> the stacking is applied. There are 3 places/ways it can be applied.
>>>>>
>>>>> 1. straight override applied to current confinement when stack_onexec is
>>>>>    called
>>>>>   ie.  A + stack_onexc(B) results in A//&B
>>>>
>>>> Is this the same behavior as change_onexec()? In other words, does
>>>> "A + change_onexec(B)" result in B or D when /e is exec'ed?
>>>
>>> I've verified that the result is B, meaning that change_onexec()
>>> also acts like a straight override much like #1.
>>>
>> correct
>>
>> << snip >>
>>
>>
>>>>> So in the end the questions are?
>>>>>
>>>>> 1. Do we want to provide access to stacking without rules, or reduced
>>>>> rule requirements.
>>>>> - this can be allowed in the straight stacking without much risk
>>>>
>>>> I think there is a fair amount of risk since we now have a large number
>>>> of userspace programs that are making policy decisions based on AppArmor
>>>> confinement contexts. In some cases, they're using the query API to have
>>>> the kernel make the decision for them and I assume that the kernel is
>>>> handling that appropriately. However, in other cases, they're making
>>>> their own decisions based on the confinement context and I highly doubt
>>>> that they're all ready for something crazy like
>>>> ":ns:foo//&bar//&:child_ns:baz (mixed)".
>>>>
>>>> (waiting to chime in on #2 until the question above is answered)
>>>>
>>>> Tyler
>>>>
>>>>> - this is problematic for stack_onexec, but see 2 below
>>>>>
>>>>>
>>>>> 2. Which semantics of stack_onexec should we go with?
>>>>> It seems clear to me that #1 is the way to go as it is equivalent to
>>>>> doing aa_getcon(), concat, aa_change_profile() as outlined above.
>>>
>>> I also think that #1 is the way to go.
>>>
>>>>> However this requires change_profile rules other wise it can be used to
>>>>> over ride policy transitions when it shouldn't. If we want to allow for
>>>>> stacking without requiring change_profile rules we could the requirements
>>>>> some, by
>>>>>   1. allowing the stack_onexec if the exec transition is respected
>>>>>      eg. if a task is confined by profile E and it stacks C
>>>>>         E -> E//&C
>>>>>      since the exec transition E -> E is specified in the profile (ix)
>>>>>      then stacking C on top is no different than doing a regular stack,
>>>>>      as it doesn't override the exec transition. This allows for
>>>>>      stacking without requiring the stub profile until the exec
>>>>>      transition. That is to say that it has a parallel to what is done
>>>>>      with change_profile vs change_onexec.
>>>>>      ie. in task 1 is confined by E then
>>>>>        E + stack(B) -> E//&B -> exec /e -> E//&C
>>>>>
>>>>>      where B is the stub profile and
>>>>>        E + stack_onexec(C) -> exec /e -> E//&C
>>>
>>> This is difficult for me to understand and I'm in the middle of writing
>>> tests cases for stacking. I don't see how we can easily document such
>>> special, corner case behavior in a meaningful way. I feel like this
>>> increases complexity (implementation, documentation, testing, etc.) for
>>> very little benefit.
>>>
>>
>> It is a small complexity. I don't think we actually have enough scope on
>> how this will be used in the real world to judge whether it is worth it.
> 
> Agreed that we can't judge whether it is worth it or not. That has me
> leaning towards leaving the mediation tight and possibly loosening it up
> later in regards to this behavior.
> 
>>
>>
>>>>>   2. allowing the stack_onexec if there is a change_profile rule for the
>>>>>   existing labeling.
>>>>>     This is similar to point 1 above but instead of relying on just the
>>>>>     exec we could also allow transitions allowed by change_profile for
>>>>>     the current confinement.
>>>>>       eg. if a task is confined by profile A' which allows
>>>>>         change_profile /e -> C,
>>>>>
>>>>>       then a change_onexec() or stack_onexec() that resulted in
>>>>>         C//&something would be allowed as
>>>>>         A'' is allowed to be replaced by C in the basic change_onexec() case.
>>>
>>> This feels more straightfoward to me. I wouldn't be against this but I
>>> also don't personally see a strong need for it.
>>>
>>
>> well that really comes down to your use case. Seth's use case is not having to
>> modify distro policy directly, which this tries to accommodate some. Honestly
>> I feel that not modifying the distro policy is more of a packaging problem but
>> I can see its uses. Whether it is worth supporting ...?
> 
> I don't understand the "not having to modify distro policy" use case.
> We're talking about the aa_stack_onexec() libapparmor API. Programs must
> be modified to call that libapparmor function. A small modification to
> the profile will be trivial in comparison to modifying the program.
> 

Well yes and no. You can launch applications via aa-exec or wrapper scripts and
have them set up the stack.  But again I think this shows that it more a
problem of how distro policy is handled that its preferable to do this than
just modify the policy



>> There are few other points to consider:
>> - it is rather easy to support both #1 and #3, though this would require having
>>   a couple extra api fns. I know there has been push back against doing so
>>   already but it is worth considering
> 
> Count me in as one who pushes back on this idea. :)
> 
>> - it is easy to support 1 option now and add the second later if it is deemed
>>   worth while.
> 
> That feels like a reasonable approach to me.
> 
> Tyler
> 
>> - it is easier to start tight, requiring the rules and then loosen up what is
>>   required of policy, than it is to decide to tighten policy after the fact
>>
>>
>> The last point has me leaning towards being conservative with the first
>> iteration. But I am finding it hard to reconcile that with how lxc is going
>> to want to use stacking