Re: [PATCH] security: fix the default value of secid_to_secctx hook

Re: [PATCH] security: fix the default value of secid_to_secctx hook

[James Morris]

On Tue, 12 May 2020, Anders Roxell wrote:

> security_secid_to_secctx is called by the bpf_lsm hook and a successful
> return value (i.e 0) implies that the parameter will be consumed by the
> LSM framework. The current behaviour return success when the pointer
> isn't initialized when CONFIG_BPF_LSM is enabled, with the default
> return from kernel/bpf/bpf_lsm.c.
> 
> This is the internal error:
> 
> [ 1229.341488][ T2659] usercopy: Kernel memory exposure attempt detected from null address (offset 0, size 280)!
> [ 1229.374977][ T2659] ------------[ cut here ]------------
> [ 1229.376813][ T2659] kernel BUG at mm/usercopy.c:99!
> [ 1229.378398][ T2659] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [ 1229.380348][ T2659] Modules linked in:
> [ 1229.381654][ T2659] CPU: 0 PID: 2659 Comm: systemd-journal Tainted: G    B   W         5.7.0-rc5-next-20200511-00019-g864e0c6319b8-dirty #13
> [ 1229.385429][ T2659] Hardware name: linux,dummy-virt (DT)
> [ 1229.387143][ T2659] pstate: 80400005 (Nzcv daif +PAN -UAO BTYPE=--)
> [ 1229.389165][ T2659] pc : usercopy_abort+0xc8/0xcc
> [ 1229.390705][ T2659] lr : usercopy_abort+0xc8/0xcc
> [ 1229.392225][ T2659] sp : ffff000064247450
> [ 1229.393533][ T2659] x29: ffff000064247460 x28: 0000000000000000
> [ 1229.395449][ T2659] x27: 0000000000000118 x26: 0000000000000000
> [ 1229.397384][ T2659] x25: ffffa000127049e0 x24: ffffa000127049e0
> [ 1229.399306][ T2659] x23: ffffa000127048e0 x22: ffffa000127048a0
> [ 1229.401241][ T2659] x21: ffffa00012704b80 x20: ffffa000127049e0
> [ 1229.403163][ T2659] x19: ffffa00012704820 x18: 0000000000000000
> [ 1229.405094][ T2659] x17: 0000000000000000 x16: 0000000000000000
> [ 1229.407008][ T2659] x15: 0000000000000000 x14: 003d090000000000
> [ 1229.408942][ T2659] x13: ffff80000d5b25b2 x12: 1fffe0000d5b25b1
> [ 1229.410859][ T2659] x11: 1fffe0000d5b25b1 x10: ffff80000d5b25b1
> [ 1229.412791][ T2659] x9 : ffffa0001034bee0 x8 : ffff00006ad92d8f
> [ 1229.414707][ T2659] x7 : 0000000000000000 x6 : ffffa00015eacb20
> [ 1229.416642][ T2659] x5 : ffff0000693c8040 x4 : 0000000000000000
> [ 1229.418558][ T2659] x3 : ffffa0001034befc x2 : d57a7483a01c6300
> [ 1229.420610][ T2659] x1 : 0000000000000000 x0 : 0000000000000059
> [ 1229.422526][ T2659] Call trace:
> [ 1229.423631][ T2659]  usercopy_abort+0xc8/0xcc
> [ 1229.425091][ T2659]  __check_object_size+0xdc/0x7d4
> [ 1229.426729][ T2659]  put_cmsg+0xa30/0xa90
> [ 1229.428132][ T2659]  unix_dgram_recvmsg+0x80c/0x930
> [ 1229.429731][ T2659]  sock_recvmsg+0x9c/0xc0
> [ 1229.431123][ T2659]  ____sys_recvmsg+0x1cc/0x5f8
> [ 1229.432663][ T2659]  ___sys_recvmsg+0x100/0x160
> [ 1229.434151][ T2659]  __sys_recvmsg+0x110/0x1a8
> [ 1229.435623][ T2659]  __arm64_sys_recvmsg+0x58/0x70
> [ 1229.437218][ T2659]  el0_svc_common.constprop.1+0x29c/0x340
> [ 1229.438994][ T2659]  do_el0_svc+0xe8/0x108
> [ 1229.440587][ T2659]  el0_svc+0x74/0x88
> [ 1229.441917][ T2659]  el0_sync_handler+0xe4/0x8b4
> [ 1229.443464][ T2659]  el0_sync+0x17c/0x180
> [ 1229.444920][ T2659] Code: aa1703e2 aa1603e1 910a8260 97ecc860 (d4210000)
> [ 1229.447070][ T2659] ---[ end trace 400497d91baeaf51 ]---
> [ 1229.448791][ T2659] Kernel panic - not syncing: Fatal exception
> [ 1229.450692][ T2659] Kernel Offset: disabled
> [ 1229.452061][ T2659] CPU features: 0x240002,20002004
> [ 1229.453647][ T2659] Memory Limit: none
> [ 1229.455015][ T2659] ---[ end Kernel panic - not syncing: Fatal exception ]---
> 
> Rework the so the default return value is -EOPNOTSUPP.
> 
> There are likely other callbacks such as security_inode_getsecctx() that
> may have the same problem, and that someone that understand the code
> better needs to audit them.
> 
> Thank you Arnd for helping me figure out what went wrong.
> 
> CC: Arnd Bergmann <arnd@arndb.de>
> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

Note, this patch should have been sent to me and cc'd the LSM list.


Acked-by: James Morris <jamorris@linux.microsoft.com>



-- 
James Morris
<jmorris@namei.org>

RE: [PATCH] security: fix the default value of secid_to_secctx hook

[Schaufler, Casey]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of Arnd Bergmann
> Sent: Saturday, May 16, 2020 1:05 AM
> To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: James Morris <jamorris@linux.microsoft.com>; Anders Roxell
> <anders.roxell@linaro.org>; Alexei Starovoitov <ast@kernel.org>; Daniel
> Borkmann <daniel@iogearbox.net>; LKML <linux-kernel@vger.kernel.org>;
> Network Development <netdev@vger.kernel.org>; bpf
> <bpf@vger.kernel.org>
> Subject: Re: [PATCH] security: fix the default value of secid_to_secctx hook

I would *really* appreciate it if discussions about the LSM infrastructure
where done on the linux-security-module mail list. (added to CC).

> 
> On Sat, May 16, 2020 at 1:29 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, May 14, 2020 at 12:47 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, May 14, 2020 at 12:43 PM James Morris
> > > <jamorris@linux.microsoft.com> wrote:
> > > >
> > > > On Wed, 13 May 2020, Alexei Starovoitov wrote:
> > > >
> > > > > James,
> > > > >
> > > > > since you took the previous similar patch are you going to pick this
> > > > > one up as well?
> > > > > Or we can route it via bpf tree to Linus asap.
> > > >
> > > > Routing via your tree is fine.
> > >
> > > Perfect.
> > > Applied to bpf tree. Thanks everyone.
> >
> > Looks like it was a wrong fix.
> > It breaks audit like this:
> > sudo auditctl -e 0
> > [   88.400296] audit: error in audit_log_task_context
> > [   88.400976] audit: error in audit_log_task_context
> > [   88.401597] audit: type=1305 audit(1589584951.198:89): op=set
> > audit_enabled=0 old=1 auid=0 ses=1 res=0
> > [   88.402691] audit: type=1300 audit(1589584951.198:89):
> > arch=c000003e syscall=44 success=yes exit=52 a0=3 a1=7ffe42a37400
> > a2=34 a3=0 items=0 ppid=2250 pid=2251 auid=0 uid=0 gid=0 euid=0 suid=0
> > fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 se)
> > [   88.405587] audit: type=1327 audit(1589584951.198:89):
> > proctitle=617564697463746C002D650030
> > Error sending enable request (Operation not supported)
> >
> > when CONFIG_LSM= has "bpf" in it.
> 
> Do you have more than one LSM enabled? It looks like
> the problem with security_secid_to_secctx() is now that it
> returns an error if any of the LSMs fail and the caller expects
> it to succeed if at least one of them sets the secdata pointer.
> 
> The problem earlier was that the call succeeded even though
> no LSM had set the pointer.
> 
> What is the behavior we actually expect from this function if
> multiple LSM are loaded?
> 
>        Arnd

Re: [PATCH] security: fix the default value of secid_to_secctx hook

[Casey Schaufler]

On 5/18/2020 2:43 PM, Schaufler, Casey wrote:
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
>> owner@vger.kernel.org> On Behalf Of Arnd Bergmann
>> Sent: Saturday, May 16, 2020 1:05 AM
>> To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Cc: James Morris <jamorris@linux.microsoft.com>; Anders Roxell
>> <anders.roxell@linaro.org>; Alexei Starovoitov <ast@kernel.org>; Daniel
>> Borkmann <daniel@iogearbox.net>; LKML <linux-kernel@vger.kernel.org>;
>> Network Development <netdev@vger.kernel.org>; bpf
>> <bpf@vger.kernel.org>
>> Subject: Re: [PATCH] security: fix the default value of secid_to_secctx hook
> I would *really* appreciate it if discussions about the LSM infrastructure
> where done on the linux-security-module mail list. (added to CC).
>
>> On Sat, May 16, 2020 at 1:29 AM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Thu, May 14, 2020 at 12:47 PM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>> On Thu, May 14, 2020 at 12:43 PM James Morris
>>>> <jamorris@linux.microsoft.com> wrote:
>>>>> On Wed, 13 May 2020, Alexei Starovoitov wrote:
>>>>>
>>>>>> James,
>>>>>>
>>>>>> since you took the previous similar patch are you going to pick this
>>>>>> one up as well?
>>>>>> Or we can route it via bpf tree to Linus asap.
>>>>> Routing via your tree is fine.
>>>> Perfect.
>>>> Applied to bpf tree. Thanks everyone.
>>> Looks like it was a wrong fix.
>>> It breaks audit like this:
>>> sudo auditctl -e 0
>>> [   88.400296] audit: error in audit_log_task_context
>>> [   88.400976] audit: error in audit_log_task_context
>>> [   88.401597] audit: type=1305 audit(1589584951.198:89): op=set
>>> audit_enabled=0 old=1 auid=0 ses=1 res=0
>>> [   88.402691] audit: type=1300 audit(1589584951.198:89):
>>> arch=c000003e syscall=44 success=yes exit=52 a0=3 a1=7ffe42a37400
>>> a2=34 a3=0 items=0 ppid=2250 pid=2251 auid=0 uid=0 gid=0 euid=0 suid=0
>>> fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 se)
>>> [   88.405587] audit: type=1327 audit(1589584951.198:89):
>>> proctitle=617564697463746C002D650030
>>> Error sending enable request (Operation not supported)
>>>
>>> when CONFIG_LSM= has "bpf" in it.
>> Do you have more than one LSM enabled? It looks like
>> the problem with security_secid_to_secctx() is now that it
>> returns an error if any of the LSMs fail and the caller expects
>> it to succeed if at least one of them sets the secdata pointer.

security_secid_to_secctx() is not currently stackable (I'm
looking at 5.7-rc6) even for this simple case. call_int_hook()
does bail-on-fail and will try all hooks registered, looking for
a failure.

You need to replace the call_int_hook() with an explicit
hlist_for_each_entry(), as is done in security_inode_getsecurity().

>>
>> The problem earlier was that the call succeeded even though
>> no LSM had set the pointer.
>>
>> What is the behavior we actually expect from this function if
>> multiple LSM are loaded?
>>
>>        Arnd