| Message ID | 4ea25dc6-78f1-9111-de2d-48f863d5dacd@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,xenial] apparmor: provide userspace flag indicating binfmt_elf_mmap change | expand |
[+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@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. Tyler > --- > security/apparmor/apparmorfs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 6950d27105e6..f63fa9a4f25d 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -1533,6 +1533,7 @@ static struct aa_fs_entry aa_fs_entry_domain[] = { > AA_FS_FILE_BOOLEAN("change_profile", 1), > AA_FS_FILE_BOOLEAN("stack", 1), > AA_FS_FILE_STRING("version", "1.2"), > + AA_FS_FILE_BOOLEAN("fix_binfmt_elf_mmap", 1), > { } > }; > > -- > 2.17.1 >
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@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.
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@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
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@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. Tyler > > Tyler > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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@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.
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 6950d27105e6..f63fa9a4f25d 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -1533,6 +1533,7 @@ static struct aa_fs_entry aa_fs_entry_domain[] = { AA_FS_FILE_BOOLEAN("change_profile", 1), AA_FS_FILE_BOOLEAN("stack", 1), AA_FS_FILE_STRING("version", "1.2"), + AA_FS_FILE_BOOLEAN("fix_binfmt_elf_mmap", 1), { } };
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@canonical.com> (backported from commit 34c426acb75cc21bdf84685e106db0c1a3565057) --- security/apparmor/apparmorfs.c | 1 + 1 file changed, 1 insertion(+)