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 |
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
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
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
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
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 --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; }