[apparmor] [patch] - fix exec_stack to work on pre 4.8 kernels

Wed Oct 5 17:17:56 UTC 2016

On 10/05/2016 07:36 AM, Tyler Hicks wrote:
> On 10/05/2016 02:46 AM, John Johansen wrote:
>> On 10/04/2016 07:32 PM, Tyler Hicks wrote:
>>> On 10/04/2016 06:31 PM, John Johansen wrote:
>>>> exec_stack picked up a fix to address a semantic change introduced in
>>>> 4.8 kernels. However this breaks the exec_stack test for kernel pre
>>>> 4.8. This patch uses an apparmor kernel flag to detect whether the
>>>> semantic change is present and adjusts the test accordingly.
>>>
>>> A couple questions below...
>>>
>>>>
>>>> ---
>>>>
>>>> === modified file 'tests/regression/apparmor/exec_stack.sh'
>>>> --- tests/regression/apparmor/exec_stack.sh	2016-09-29 04:11:29 +0000
>>>> +++ tests/regression/apparmor/exec_stack.sh	2016-10-04 21:15:48 +0000
>>>> @@ -43,6 +43,12 @@
>>>>  
>>>>  touch $file $otherfile $sharedfile $thirdfile
>>>>  
>>>> +if [ "$(kernel_features domain/fix_binfmt_elf_mmap)" == "true" ]; then
>>>
>>> Why is the kernel doing domain/fix_binfmt_elf_mmap instead of bumping
>>> the kABI? Maybe I'm misunderstanding the purpose of the kABI but I
>>> understood it to be bumped when there were was a change in mediation
>>> that causes policy change.
>>>
>>
>> For a few reasons.
>> 1. I brought up bumping the potential of bumping the abi and got no
>> feedback.
> 
> Sorry about that. I'm still trying to fully understand how kABI is
> intended to be used (I think I'm getting there now) so I don't know that
> I could have provided much feedback.
> 
>> 2. The abi bump here is odd in that it is kernel caused not apparmor.
>> That doesn't mean we don't bump the abi but it is less clear. Hence 1
>> 3. I am worried that the patch that introduced the semantic change
>> will show up in stable kernels as it is an information leak fix
>> for tracing. For none ubuntu kernels this will mean a different
>> abi on the apparmor side, so bumping the abi alone is problematic.
>> Ubuntu is at v7, while upstream is at v6. We can bump the abi to v8
>> but what do we do for upstream or other distro kernels? We can't
>> bump them to v7, nor v8 as they are missing other semantic changes.
> 
> Interesting and frustrating problem.
> 
>>
>> This is a case where a sub version would be nice.
>>
>> so with that issue in place so it can be properly detected and conditioned
>> in the regression tests. Since we can't currently tie it to a single
>> abi number.
>>
>> With that said I am still open to bumping to v8 for stacking based
>> kernels. But we need the flag to deal with v6 abi based kernels if
>> this ever happens.
>>
>> I am no also open to the idea of using one or two bits of the abi
>> for a sub version. So we can handle something like this better in the
>> future.
> 
> One or two bits doesn't seem like enough. If that's all that we have to
> spare, I think I would have chosen 'domain/fix_binfmt_elf_mmap', too.
> 
>>
>>>> +    elfmmap="m"
>>>> +else
>>>> +    elfmmap=""
>>>> +fi
>>>> +
>>>>  # Verify file access and contexts by an unconfined process
>>>>  runchecktest "EXEC_STACK (unconfined - file)" pass -f $file
>>>>  runchecktest "EXEC_STACK (unconfined - otherfile)" pass -f $otherfile
>>>> @@ -66,7 +72,7 @@
>>>>  
>>>>  # Verify file access and contexts by 2 stacked profiles
>>>>  genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
>>>> -	image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm
>>>> +	image=$othertest addimage:$test $otherok $sharedok $getcon $test:r$elfmmap
>>>
>>> The previous change (r3509) simply added 'm' to the existing '$test r,'
>>> rules but this patch description says, "this breaks the exec_stack test
>>> for kernel pre 4.8." Is it true that adding 'm' actually broke the tests
>>> in pre 4.8 or are you just trying to make the tests more accurate?
>>>
>> In this case yes. I originally was worried about it breaking over looking
>> that its just an addition. But even so these are the only tests we have
>> that caught the behavioral change, and I didn't want to loose that.
> 
> I asked which of two scenarios are true and you answered "yes". :)
> 
yes. They are both true, initially I did it because I was worried about
breaking old kernels, then it clicked.

I then wanted it to keep them for accuracy and a fallback for testing
until we made a test specifically for this.

> I assume that you were answering yes to "are you just trying to make the
> tests more accurate?".
> 
> My concern was that if adding 'm' to the target profile broke things, we
> have a larger problem on our hands in that if we update any profiles to
> add 'm' to the target profile, as required by 4.8 and newer kernels,
> then those profiles wouldn't work on older kernels any longer. I wanted
> to confirm that wasn't the case.
> 
It isn't. It is just an extra permission. However we have that problem
on the other side as well, where the m is needed on the calling profile
for pre 4.8 kernels, and not for 4.8 kernels.

So to have policy that works for both we now require the m in both the
calling and transition profile.

>>
>> Yes we need an separate test for this semantic, but its what I had because
>> I had done this based off of my original unfounded worries, and I wanted
>> to get it out asap. Partly because even in ubuntu we have users using
>> custom kernels, old kernels, backport kernels, and again I am worried
>> about the semantic change patch showing up and breaking things, leading
>> to bug reports, and time wasted.
>>
>> So yes we can withdraw this patch once we have a dedicated test.
> 
> No, I think that this patch is fine and I didn't meant to hold you up on
> committing it. I was only curious about the bigger questions that this
> patch raised. Please add my ack and commit ASAP.
> 
thanks I will update the description to make it less confusing

>   Acked-by: Tyler Hicks <tyhicks at canonical.com>
> 
> Thanks!
> 
> Tyler
> 
>>
>>
>>> Tyler
>>>
>>>>  runchecktest_errno EACCES "EXEC_STACK (2 stacked - file)" fail -- $test -f $file
>>>>  runchecktest_errno EACCES "EXEC_STACK (2 stacked - otherfile)" fail -- $test -f $otherfile
>>>>  runchecktest_errno EACCES "EXEC_STACK (2 stacked - thirdfile)" fail -- $test -f $thirdfile
>>>> @@ -79,7 +85,7 @@
>>>>  # Verify file access and contexts by 3 stacked profiles
>>>>  genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
>>>>  	image=$othertest addimage:$test $otherok $sharedok $getcon $test:"rix -> &$thirdtest" -- \
>>>> -	image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:rm
>>>> +	image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:r$elfmmap
>>>>  runchecktest_errno EACCES "EXEC_STACK (3 stacked - file)" fail -- $test -- $test -f $file
>>>>  runchecktest_errno EACCES "EXEC_STACK (3 stacked - otherfile)" fail -- $test -- $test -f $otherfile
>>>>  runchecktest_errno EACCES "EXEC_STACK (3 stacked - thirdfile)" fail -- $test -- $test -f $thirdfile
>>>> @@ -89,7 +95,7 @@
>>>>  
>>>>  genprofile -I $sharedok $stackotherok $stackthirdok $test:"rix -> &$othertest" -- \
>>>>  	image=$othertest addimage:$test $sharedok $stackthirdok $test:"rix -> &$thirdtest" -- \
>>>> -	image=$thirdtest addimage:$test $sharedok $stackthirdok $test:rm
>>>> +	image=$thirdtest addimage:$test $sharedok $stackthirdok $test:r$elfmmap
>>>>  # Triggered an AppArmor WARN in the initial stacking patch set
>>>>  runchecktest "EXEC_STACK (3 stacked - old AA WARN)" pass -p $othertest -- $test -p $thirdtest -f $sharedfile
>>>>  
>>>> @@ -120,7 +126,7 @@
>>>>  
>>>>  # Verify file access and contexts in mixed mode
>>>>  genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
>>>> -	image=$othertest flag:complain addimage:$test $otherok $sharedok $getcon $test:rm
>>>> +	image=$othertest flag:complain addimage:$test $otherok $sharedok $getcon $test:r$elfmmap
>>>>  runchecktest "EXEC_STACK (mixed mode - file)" pass -- $test -f $file
>>>>  runchecktest_errno EACCES "EXEC_STACK (mixed mode - otherfile)" fail -- $test -f $otherfile
>>>>  runchecktest "EXEC_STACK (mixed mode - sharedfile)" pass -- $test -f $sharedfile
>>>>
>>>>
>>>>
>>>
>>>
>>
> 
> 