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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-2
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 <[hidden email]>
(backported from commit 34c426acb75cc21bdf84685e106db0c1a3565057)
---
 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


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
[+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 <[hidden email]>
> (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
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-2
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 <[hidden email]>
>> (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.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
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 <[hidden email]>
> >> (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

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
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 <[hidden email]>
> > >> (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
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-2
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 <[hidden email]>
>>>>> (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.


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team