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

[Tyler Hicks]

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.

Tyler