NACK: [PATCH][SRU][xenial] apparmor: provide userspace flag indicating binfmt_elf_mmap change

[John Johansen]

On 8/8/19 11:07 AM, Tyler Hicks wrote:
> On 2019-08-06 15:29:11, Tyler Hicks wrote:
>> On 2019-08-06 13:19:34, John Johansen wrote:
>>> On 8/6/19 8:04 AM, Tyler Hicks wrote:
>>>> [+sbeattie]
>>>>
>>>> On 2019-08-05 16:25:28, John Johansen wrote:
>>>>> Commit 7b8d468e60d6 ("binfmt_elf: switch to new creds when switching to new mm") brought upstream
>>>>> Commit 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm")
>>>>> back to Xenial without the required apparmor flag to indicate the change.
>>>>>
>>>>> This is a backport of the apparmor fix
>>>>>
>>>>> Commit 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm")
>>>>> changed when the creds are installed by the binfmt_elf handler. This
>>>>> affects which creds are used to mmap the executable into the address
>>>>> space. Which can have an affect on apparmor policy.
>>>>>
>>>>> Add a flag to apparmor at
>>>>> /sys/kernel/security/apparmor/features/domain/fix_binfmt_elf_mmap
>>>>>
>>>>> to make it possible to detect this semantic change so that the userspace
>>>>> tools and the regression test suite can correctly deal with the change.
>>>>>
>>>>> BugLink: http://bugs.launchpad.net/bugs/1630069
>>>>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>>>>> (backported from commit 34c426acb75cc21bdf84685e106db0c1a3565057)
>>>>
>>>> Steve proposed this patch in late May:
>>>>
>>>>  https://lists.ubuntu.com/archives/kernel-team/2019-May/100977.html
>>>>
>>>> The security team discussed it and it was decided that causing a full
>>>> policy cache recompilation across all Xenial devices wasn't worth it:
>>>>
>>>>  https://lists.ubuntu.com/archives/kernel-team/2019-May/101031.html
>>>>
>>>> Steve then worked around this in QRT:
>>>>
>>>>  https://git.launchpad.net/qa-regression-testing/commit/?id=7631da6c8025e657732cb65bcaaf11ad8be162ab
>>>>
>>>> If the previous decision has changed, we should reconsider the patch but
>>>> I'm nack'ing for now so that it doesn't get merged without more
>>>> discussion.
>>>>
>>>
>>> Its the wrong approach that leaves us with a technical debt that has to
>>> be remembered/rediscovered every few months when we update the tests,
>>> or bring patches back to Xenial.
>>>
>>> Initial testing isn't done against UCT its done against the upstream
>>> test suite, where patch and test development is done.
>>
>> I'm not hard nack'ing this patch. I brought up the policy cache
>> regeneration topic in May as a, "did you all think about this?" type of
>> thing and then the answer I got back was that it wasn't worth causing
>> every Xenial system out there to regenerate its policy cache (both from
>> a CPU resources and risk aspect).
>>
>> I personally don't think it is worth it but if the security team
>> collectively thinks it should be merged, we can merge it. Please discuss
>> it internally.
> 
> Friendly ping. A final answer today would be ideal if this is going to
> make it into the next SRU cycle.
> 
While I think this is the right approach, there are practical reasons for
sbeattie's approach. I need to spend some more time investigating the
impact on the Ubuntu core devices before I push for this, so I am going
to let it drop for now.