diff mbox series

security: fix integer overflow in lsm_set_self_attr() syscall

Message ID 20240214160538.1086089-1-jannh@google.com (mailing list archive)
State Accepted
Delegated to: Paul Moore
Headers show
Series security: fix integer overflow in lsm_set_self_attr() syscall | expand

Commit Message

Jann Horn Feb. 14, 2024, 4:05 p.m. UTC
security_setselfattr() has an integer overflow bug that leads to
out-of-bounds access when userspace provides bogus input:
`lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
redundantly, also against `size`), but there are no checks on
`lctx->ctx_len`.
Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
will then be passed to an LSM module as a buffer length, causing LSM
modules to perform out-of-bounds accesses.

The following reproducer will demonstrate this under ASAN (if AppArmor is
loaded as an LSM):
```
#define _GNU_SOURCE
#include <unistd.h>
#include <stdint.h>
#include <stdlib.h>
#include <sys/syscall.h>

struct lsm_ctx {
  uint64_t id;
  uint64_t flags;
  uint64_t len;
  uint64_t ctx_len;
  char ctx[];
};

int main(void) {
  size_t size = sizeof(struct lsm_ctx);
  struct lsm_ctx *ctx = malloc(size);
  ctx->id = 104/*LSM_ID_APPARMOR*/;
  ctx->flags = 0;
  ctx->len = size;
  ctx->ctx_len = -sizeof(struct lsm_ctx);
  syscall(
    460/*__NR_lsm_set_self_attr*/,
    /*attr=*/  100/*LSM_ATTR_CURRENT*/,
    /*ctx=*/   ctx,
    /*size=*/  size,
    /*flags=*/ 0
  );
}
```

(I'm including an ASAN splat in the patch notes sent to the list.)

Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
Signed-off-by: Jann Horn <jannh@google.com>
---
ASAN splat from the reproducer:
==================================================================
BUG: KASAN: slab-out-of-bounds in do_setattr (security/apparmor/lsm.c:860) 
Read of size 1 at addr ffff888006163abf by task setselfattr/548

CPU: 0 PID: 548 Comm: setselfattr Not tainted 6.8.0-rc4-00014-g7e90b5c295ec-dirty #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl (lib/dump_stack.c:107) 
print_report (mm/kasan/report.c:378 mm/kasan/report.c:488) 
[...]
kasan_report (mm/kasan/report.c:603) 
[...]
do_setattr (security/apparmor/lsm.c:860) 
[...]
apparmor_setselfattr (security/apparmor/lsm.c:935) 
security_setselfattr (security/security.c:4038) 
__x64_sys_lsm_set_self_attr (security/lsm_syscalls.c:55) 
do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 
RIP: 0033:0x7f29a170ff59
Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
All code
========
   0:	00 c3                	add    %al,%bl
   2:	66 2e 0f 1f 84 00 00 	cs nopw 0x0(%rax,%rax,1)
   9:	00 00 00 
   c:	0f 1f 44 00 00       	nopl   0x0(%rax,%rax,1)
  11:	48 89 f8             	mov    %rdi,%rax
  14:	48 89 f7             	mov    %rsi,%rdi
  17:	48 89 d6             	mov    %rdx,%rsi
  1a:	48 89 ca             	mov    %rcx,%rdx
  1d:	4d 89 c2             	mov    %r8,%r10
  20:	4d 89 c8             	mov    %r9,%r8
  23:	4c 8b 4c 24 08       	mov    0x8(%rsp),%r9
  28:	0f 05                	syscall
  2a:*	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax		<-- trapping instruction
  30:	73 01                	jae    0x33
  32:	c3                   	ret
  33:	48 8b 0d 07 6f 0c 00 	mov    0xc6f07(%rip),%rcx        # 0xc6f41
  3a:	f7 d8                	neg    %eax
  3c:	64 89 01             	mov    %eax,%fs:(%rcx)
  3f:	48                   	rex.W

Code starting with the faulting instruction
===========================================
   0:	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax
   6:	73 01                	jae    0x9
   8:	c3                   	ret
   9:	48 8b 0d 07 6f 0c 00 	mov    0xc6f07(%rip),%rcx        # 0xc6f17
  10:	f7 d8                	neg    %eax
  12:	64 89 01             	mov    %eax,%fs:(%rcx)
  15:	48                   	rex.W
RSP: 002b:00007ffd41c781a8 EFLAGS: 00000202 ORIG_RAX: 00000000000001cc
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f29a170ff59
RDX: 0000000000000020 RSI: 000056518c581260 RDI: 0000000000000064
RBP: 00007ffd41c781c0 R08: 00000000000a3330 R09: 000056518c581260
R10: 0000000000000000 R11: 0000000000000202 R12: 000056518bd95060
R13: 00007ffd41c782a0 R14: 0000000000000000 R15: 0000000000000000
</TASK>

Allocated by task 548 on cpu 0 at 61.045304s:
kasan_save_stack (mm/kasan/common.c:48) 
kasan_save_track (mm/kasan/common.c:68) 
__kasan_kmalloc (mm/kasan/common.c:372 mm/kasan/common.c:389) 
__kmalloc (./include/linux/kasan.h:211 mm/slub.c:3981 mm/slub.c:3994) 
load_elf_binary (./include/linux/slab.h:594 fs/binfmt_elf.c:880) 
bprm_execve (fs/exec.c:1783 fs/exec.c:1825 fs/exec.c:1877 fs/exec.c:1853) 
do_execveat_common.isra.0 (fs/exec.c:1984) 
__x64_sys_execve (fs/exec.c:2129 (discriminator 1)) 
do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 

Freed by task 548 on cpu 0 at 61.045380s:
kasan_save_stack (mm/kasan/common.c:48) 
kasan_save_track (mm/kasan/common.c:68) 
kasan_save_free_info (mm/kasan/generic.c:643 (discriminator 1)) 
poison_slab_object (mm/kasan/common.c:243) 
__kasan_slab_free (mm/kasan/common.c:257 (discriminator 1)) 
kfree (mm/slub.c:4299 (discriminator 3) mm/slub.c:4409 (discriminator 3)) 
load_elf_binary (fs/binfmt_elf.c:896 (discriminator 1)) 
bprm_execve (fs/exec.c:1783 fs/exec.c:1825 fs/exec.c:1877 fs/exec.c:1853) 
do_execveat_common.isra.0 (fs/exec.c:1984) 
__x64_sys_execve (fs/exec.c:2129 (discriminator 1)) 
do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 

The buggy address belongs to the object at ffff888006163a80
which belongs to the cache kmalloc-32 of size 32
The buggy address is located 31 bytes to the right of
allocated 32-byte region [ffff888006163a80, ffff888006163aa0)

The buggy address belongs to the physical page:
page:0000000021a8da3a refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x6163
flags: 0x100000000000800(slab|node=0|zone=1)
page_type: 0xffffffff()
raw: 0100000000000800 ffff888001042500 dead000000000122 0000000000000000
raw: 0000000000000000 0000000080400040 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff888006163980: fa fb fb fb fc fc fc fc 00 00 00 00 fc fc fc fc
ffff888006163a00: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
>ffff888006163a80: fa fb fb fb fc fc fc fc 00 00 00 00 fc fc fc fc
^
ffff888006163b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888006163b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


 security/security.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


base-commit: 7e90b5c295ec1e47c8ad865429f046970c549a66

Comments

Casey Schaufler Feb. 14, 2024, 4:53 p.m. UTC | #1
On 2/14/2024 8:05 AM, Jann Horn wrote:
> security_setselfattr() has an integer overflow bug that leads to
> out-of-bounds access when userspace provides bogus input:
> `lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
> redundantly, also against `size`), but there are no checks on
> `lctx->ctx_len`.
> Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
> value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
> will then be passed to an LSM module as a buffer length, causing LSM
> modules to perform out-of-bounds accesses.
>
> The following reproducer will demonstrate this under ASAN (if AppArmor is
> loaded as an LSM):
> ```
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <sys/syscall.h>
>
> struct lsm_ctx {
>   uint64_t id;
>   uint64_t flags;
>   uint64_t len;
>   uint64_t ctx_len;
>   char ctx[];
> };
>
> int main(void) {
>   size_t size = sizeof(struct lsm_ctx);
>   struct lsm_ctx *ctx = malloc(size);
>   ctx->id = 104/*LSM_ID_APPARMOR*/;
>   ctx->flags = 0;
>   ctx->len = size;
>   ctx->ctx_len = -sizeof(struct lsm_ctx);
>   syscall(
>     460/*__NR_lsm_set_self_attr*/,
>     /*attr=*/  100/*LSM_ATTR_CURRENT*/,
>     /*ctx=*/   ctx,
>     /*size=*/  size,
>     /*flags=*/ 0
>   );
> }
> ```
>
> (I'm including an ASAN splat in the patch notes sent to the list.)
>
> Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
> ASAN splat from the reproducer:
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in do_setattr (security/apparmor/lsm.c:860) 
> Read of size 1 at addr ffff888006163abf by task setselfattr/548
>
> CPU: 0 PID: 548 Comm: setselfattr Not tainted 6.8.0-rc4-00014-g7e90b5c295ec-dirty #5
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl (lib/dump_stack.c:107) 
> print_report (mm/kasan/report.c:378 mm/kasan/report.c:488) 
> [...]
> kasan_report (mm/kasan/report.c:603) 
> [...]
> do_setattr (security/apparmor/lsm.c:860) 
> [...]
> apparmor_setselfattr (security/apparmor/lsm.c:935) 
> security_setselfattr (security/security.c:4038) 
> __x64_sys_lsm_set_self_attr (security/lsm_syscalls.c:55) 
> do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 
> RIP: 0033:0x7f29a170ff59
> Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> All code
> ========
>    0:	00 c3                	add    %al,%bl
>    2:	66 2e 0f 1f 84 00 00 	cs nopw 0x0(%rax,%rax,1)
>    9:	00 00 00 
>    c:	0f 1f 44 00 00       	nopl   0x0(%rax,%rax,1)
>   11:	48 89 f8             	mov    %rdi,%rax
>   14:	48 89 f7             	mov    %rsi,%rdi
>   17:	48 89 d6             	mov    %rdx,%rsi
>   1a:	48 89 ca             	mov    %rcx,%rdx
>   1d:	4d 89 c2             	mov    %r8,%r10
>   20:	4d 89 c8             	mov    %r9,%r8
>   23:	4c 8b 4c 24 08       	mov    0x8(%rsp),%r9
>   28:	0f 05                	syscall
>   2a:*	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax		<-- trapping instruction
>   30:	73 01                	jae    0x33
>   32:	c3                   	ret
>   33:	48 8b 0d 07 6f 0c 00 	mov    0xc6f07(%rip),%rcx        # 0xc6f41
>   3a:	f7 d8                	neg    %eax
>   3c:	64 89 01             	mov    %eax,%fs:(%rcx)
>   3f:	48                   	rex.W
>
> Code starting with the faulting instruction
> ===========================================
>    0:	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax
>    6:	73 01                	jae    0x9
>    8:	c3                   	ret
>    9:	48 8b 0d 07 6f 0c 00 	mov    0xc6f07(%rip),%rcx        # 0xc6f17
>   10:	f7 d8                	neg    %eax
>   12:	64 89 01             	mov    %eax,%fs:(%rcx)
>   15:	48                   	rex.W
> RSP: 002b:00007ffd41c781a8 EFLAGS: 00000202 ORIG_RAX: 00000000000001cc
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f29a170ff59
> RDX: 0000000000000020 RSI: 000056518c581260 RDI: 0000000000000064
> RBP: 00007ffd41c781c0 R08: 00000000000a3330 R09: 000056518c581260
> R10: 0000000000000000 R11: 0000000000000202 R12: 000056518bd95060
> R13: 00007ffd41c782a0 R14: 0000000000000000 R15: 0000000000000000
> </TASK>
>
> Allocated by task 548 on cpu 0 at 61.045304s:
> kasan_save_stack (mm/kasan/common.c:48) 
> kasan_save_track (mm/kasan/common.c:68) 
> __kasan_kmalloc (mm/kasan/common.c:372 mm/kasan/common.c:389) 
> __kmalloc (./include/linux/kasan.h:211 mm/slub.c:3981 mm/slub.c:3994) 
> load_elf_binary (./include/linux/slab.h:594 fs/binfmt_elf.c:880) 
> bprm_execve (fs/exec.c:1783 fs/exec.c:1825 fs/exec.c:1877 fs/exec.c:1853) 
> do_execveat_common.isra.0 (fs/exec.c:1984) 
> __x64_sys_execve (fs/exec.c:2129 (discriminator 1)) 
> do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 
>
> Freed by task 548 on cpu 0 at 61.045380s:
> kasan_save_stack (mm/kasan/common.c:48) 
> kasan_save_track (mm/kasan/common.c:68) 
> kasan_save_free_info (mm/kasan/generic.c:643 (discriminator 1)) 
> poison_slab_object (mm/kasan/common.c:243) 
> __kasan_slab_free (mm/kasan/common.c:257 (discriminator 1)) 
> kfree (mm/slub.c:4299 (discriminator 3) mm/slub.c:4409 (discriminator 3)) 
> load_elf_binary (fs/binfmt_elf.c:896 (discriminator 1)) 
> bprm_execve (fs/exec.c:1783 fs/exec.c:1825 fs/exec.c:1877 fs/exec.c:1853) 
> do_execveat_common.isra.0 (fs/exec.c:1984) 
> __x64_sys_execve (fs/exec.c:2129 (discriminator 1)) 
> do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:129) 
>
> The buggy address belongs to the object at ffff888006163a80
> which belongs to the cache kmalloc-32 of size 32
> The buggy address is located 31 bytes to the right of
> allocated 32-byte region [ffff888006163a80, ffff888006163aa0)
>
> The buggy address belongs to the physical page:
> page:0000000021a8da3a refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x6163
> flags: 0x100000000000800(slab|node=0|zone=1)
> page_type: 0xffffffff()
> raw: 0100000000000800 ffff888001042500 dead000000000122 0000000000000000
> raw: 0000000000000000 0000000080400040 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888006163980: fa fb fb fb fc fc fc fc 00 00 00 00 fc fc fc fc
> ffff888006163a00: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
>> ffff888006163a80: fa fb fb fb fc fc fc fc 00 00 00 00 fc fc fc fc
> ^
> ffff888006163b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff888006163b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ==================================================================
>
>
>  security/security.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 3aaad75c9ce8..7035ee35a393 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -29,6 +29,7 @@
>  #include <linux/backing-dev.h>
>  #include <linux/string.h>
>  #include <linux/msg.h>
> +#include <linux/overflow.h>
>  #include <net/flow.h>
>  
>  /* How many LSMs were built into the kernel? */
> @@ -4015,6 +4016,7 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  	struct security_hook_list *hp;
>  	struct lsm_ctx *lctx;
>  	int rc = LSM_RET_DEFAULT(setselfattr);
> +	u64 required_len;
>  
>  	if (flags)
>  		return -EINVAL;
> @@ -4027,8 +4029,9 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  	if (IS_ERR(lctx))
>  		return PTR_ERR(lctx);
>  
> -	if (size < lctx->len || size < lctx->ctx_len + sizeof(*lctx) ||
> -	    lctx->len < lctx->ctx_len + sizeof(*lctx)) {
> +	if (size < lctx->len ||
> +	    check_add_overflow(sizeof(*lctx), lctx->ctx_len, &required_len) ||
> +	    lctx->len < required_len) {
>  		rc = -EINVAL;
>  		goto free_out;
>  	}
>
> base-commit: 7e90b5c295ec1e47c8ad865429f046970c549a66
Paul Moore Feb. 14, 2024, 6:53 p.m. UTC | #2
On Feb 14, 2024 Jann Horn <jannh@google.com> wrote:
> 
> security_setselfattr() has an integer overflow bug that leads to
> out-of-bounds access when userspace provides bogus input:
> `lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
> redundantly, also against `size`), but there are no checks on
> `lctx->ctx_len`.
> Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
> value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
> will then be passed to an LSM module as a buffer length, causing LSM
> modules to perform out-of-bounds accesses.
> 
> The following reproducer will demonstrate this under ASAN (if AppArmor is
> loaded as an LSM):
> ```
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <sys/syscall.h>
> 
> struct lsm_ctx {
>   uint64_t id;
>   uint64_t flags;
>   uint64_t len;
>   uint64_t ctx_len;
>   char ctx[];
> };
> 
> int main(void) {
>   size_t size = sizeof(struct lsm_ctx);
>   struct lsm_ctx *ctx = malloc(size);
>   ctx->id = 104/*LSM_ID_APPARMOR*/;
>   ctx->flags = 0;
>   ctx->len = size;
>   ctx->ctx_len = -sizeof(struct lsm_ctx);
>   syscall(
>     460/*__NR_lsm_set_self_attr*/,
>     /*attr=*/  100/*LSM_ATTR_CURRENT*/,
>     /*ctx=*/   ctx,
>     /*size=*/  size,
>     /*flags=*/ 0
>   );
> }
> ```
> 
> (I'm including an ASAN splat in the patch notes sent to the list.)
> 
> Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
> Signed-off-by: Jann Horn <jannh@google.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good to me, thanks Jann.  I'm going to merge this into
lsm/stable-6.8 and send this up to Linus soon (likely tomorrow).

--
paul-moore.com
Kees Cook Feb. 14, 2024, 11:24 p.m. UTC | #3
On Wed, Feb 14, 2024 at 05:05:38PM +0100, Jann Horn wrote:
> security_setselfattr() has an integer overflow bug that leads to
> out-of-bounds access when userspace provides bogus input:
> `lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
> redundantly, also against `size`), but there are no checks on
> `lctx->ctx_len`.
> Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
> value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
> will then be passed to an LSM module as a buffer length, causing LSM
> modules to perform out-of-bounds accesses.

Ugh. Thanks for catching this. I continue to want to get the unsigned
integer overflow sanitizer rolled out, which would have caught this.

> 
> The following reproducer will demonstrate this under ASAN (if AppArmor is
> loaded as an LSM):
> ```
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <sys/syscall.h>
> 
> struct lsm_ctx {
>   uint64_t id;
>   uint64_t flags;
>   uint64_t len;
>   uint64_t ctx_len;
>   char ctx[];
> };
> 
> int main(void) {
>   size_t size = sizeof(struct lsm_ctx);
>   struct lsm_ctx *ctx = malloc(size);
>   ctx->id = 104/*LSM_ID_APPARMOR*/;
>   ctx->flags = 0;
>   ctx->len = size;
>   ctx->ctx_len = -sizeof(struct lsm_ctx);
>   syscall(
>     460/*__NR_lsm_set_self_attr*/,
>     /*attr=*/  100/*LSM_ATTR_CURRENT*/,
>     /*ctx=*/   ctx,
>     /*size=*/  size,
>     /*flags=*/ 0
>   );
> }
> ```
> 
> (I'm including an ASAN splat in the patch notes sent to the list.)
> 
> Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

> ---
> ASAN splat from the reproducer:
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in do_setattr (security/apparmor/lsm.c:860) 
> Read of size 1 at addr ffff888006163abf by task setselfattr/548

I'd rather prefer that this splat (or some portion of it) stay in the
actual commit log. It makes it easier to scan for sanitizer-related
fixes, etc.

-Kees
Kees Cook Feb. 15, 2024, 12:45 a.m. UTC | #4
On Wed, Feb 14, 2024 at 08:53:52AM -0800, Casey Schaufler wrote:
> On 2/14/2024 8:05 AM, Jann Horn wrote:
> > security_setselfattr() has an integer overflow bug that leads to
> > out-of-bounds access when userspace provides bogus input:
> > `lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
> > redundantly, also against `size`), but there are no checks on
> > `lctx->ctx_len`.
> > Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
> > value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
> > will then be passed to an LSM module as a buffer length, causing LSM
> > modules to perform out-of-bounds accesses.
> >
> > The following reproducer will demonstrate this under ASAN (if AppArmor is
> > loaded as an LSM):
> > ```
> > #define _GNU_SOURCE
> > #include <unistd.h>
> > #include <stdint.h>
> > #include <stdlib.h>
> > #include <sys/syscall.h>
> >
> > struct lsm_ctx {
> >   uint64_t id;
> >   uint64_t flags;
> >   uint64_t len;
> >   uint64_t ctx_len;

Do we want to take the opportunity to reduce this to u32 for len and u32
for ctx_len? All FS operations are limited to INT_MAX anyway...

-Kees
Paul Moore Feb. 15, 2024, 1:02 p.m. UTC | #5
On February 14, 2024 7:45:43 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 14, 2024 at 08:53:52AM -0800, Casey Schaufler wrote:
>> On 2/14/2024 8:05 AM, Jann Horn wrote:
>>> security_setselfattr() has an integer overflow bug that leads to
>>> out-of-bounds access when userspace provides bogus input:
>>> `lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
>>> redundantly, also against `size`), but there are no checks on
>>> `lctx->ctx_len`.
>>> Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
>>> value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
>>> will then be passed to an LSM module as a buffer length, causing LSM
>>> modules to perform out-of-bounds accesses.
>>>
>>> The following reproducer will demonstrate this under ASAN (if AppArmor is
>>> loaded as an LSM):
>>> ```
>>> #define _GNU_SOURCE
>>> #include <unistd.h>
>>> #include <stdint.h>
>>> #include <stdlib.h>
>>> #include <sys/syscall.h>
>>>
>>> struct lsm_ctx {
>>> uint64_t id;
>>> uint64_t flags;
>>> uint64_t len;
>>> uint64_t ctx_len;
>
> Do we want to take the opportunity to reduce this to u32 for len and u32
> for ctx_len? All FS operations are limited to INT_MAX anyway...

Not at this point, no.

--
paul-moore.com
diff mbox series

Patch

diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..7035ee35a393 100644
--- a/security/security.c
+++ b/security/security.c
@@ -29,6 +29,7 @@ 
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <linux/msg.h>
+#include <linux/overflow.h>
 #include <net/flow.h>
 
 /* How many LSMs were built into the kernel? */
@@ -4015,6 +4016,7 @@  int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 	struct security_hook_list *hp;
 	struct lsm_ctx *lctx;
 	int rc = LSM_RET_DEFAULT(setselfattr);
+	u64 required_len;
 
 	if (flags)
 		return -EINVAL;
@@ -4027,8 +4029,9 @@  int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 	if (IS_ERR(lctx))
 		return PTR_ERR(lctx);
 
-	if (size < lctx->len || size < lctx->ctx_len + sizeof(*lctx) ||
-	    lctx->len < lctx->ctx_len + sizeof(*lctx)) {
+	if (size < lctx->len ||
+	    check_add_overflow(sizeof(*lctx), lctx->ctx_len, &required_len) ||
+	    lctx->len < required_len) {
 		rc = -EINVAL;
 		goto free_out;
 	}