Re: [PATCH] apparmor: Fix uninitialized value in aa_split_fqname

[Kees Cook]

On Thu, Sep 27, 2018 at 2:49 PM, Zubin Mithra <zsm@xxxxxxxxxxxx> wrote:
> Syzkaller reported a OOB-read with the stacktrace below. This occurs
> inside __aa_lookupn_ns as `n` is not initialized. `n` is obtained from
> aa_splitn_fqname. In cases where `name` is invalid, aa_splitn_fqname
> returns without initializing `ns_name` and `ns_len`.
>
> Fix this by always initializing `ns_name` and `ns_len`.
>
>         __dump_stack lib/dump_stack.c:77 [inline]
>         dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>         print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
>         kasan_report_error mm/kasan/report.c:354 [inline]
>         kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
>         __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:430
>         memcmp+0xe3/0x160 lib/string.c:861
>         strnstr+0x4b/0x70 lib/string.c:934
>         __aa_lookupn_ns+0xc1/0x570 security/apparmor/policy_ns.c:209
>         aa_lookupn_ns+0x88/0x1e0 security/apparmor/policy_ns.c:240
>         aa_fqlookupn_profile+0x1b9/0x1010 security/apparmor/policy.c:468
>         fqlookupn_profile+0x80/0xc0 security/apparmor/label.c:1844
>         aa_label_strn_parse+0xa3a/0x1230 security/apparmor/label.c:1908
>         aa_label_parse+0x42/0x50 security/apparmor/label.c:1943
>         aa_change_profile+0x513/0x3510 security/apparmor/domain.c:1362
>         apparmor_setprocattr+0xaa4/0x1150 security/apparmor/lsm.c:658
>         security_setprocattr+0x66/0xc0 security/security.c:1298
>         proc_pid_attr_write+0x301/0x540 fs/proc/base.c:2555
>         __vfs_write+0x119/0x9f0 fs/read_write.c:485
>         vfs_write+0x1fc/0x560 fs/read_write.c:549
>         ksys_write+0x101/0x260 fs/read_write.c:598
>         __do_sys_write fs/read_write.c:610 [inline]
>         __se_sys_write fs/read_write.c:607 [inline]
>         __x64_sys_write+0x73/0xb0 fs/read_write.c:607
>         do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>         entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Fixes: 3b0aaf5866bf ("apparmor: add lib fn to find the "split" for fqnames")
> Reported-by: syzbot+61e4b490d9d2da591b50@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Zubin Mithra <zsm@xxxxxxxxxxxx>

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees


> ---
>  security/apparmor/lib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
> index 974affe50531..76491e7f4177 100644
> --- a/security/apparmor/lib.c
> +++ b/security/apparmor/lib.c
> @@ -90,10 +90,12 @@ const char *aa_splitn_fqname(const char *fqname, size_t n, const char **ns_name,
>         const char *end = fqname + n;
>         const char *name = skipn_spaces(fqname, n);
>
> -       if (!name)
> -               return NULL;
>         *ns_name = NULL;
>         *ns_len = 0;
> +
> +       if (!name)
> +               return NULL;
> +
>         if (name[0] == ':') {
>                 char *split = strnchr(&name[1], end - &name[1], ':');
>                 *ns_name = skipn_spaces(&name[1], end - &name[1]);
> --
> 2.19.0.605.g01d371f741-goog
>



-- 
Kees Cook
Pixel Security