Re: [PATCH] capabilities: fix buffer overread on very short xattr

[Serge E. Hallyn]

Quoting Eric Biggers (ebiggers3@xxxxxxxxx):
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
> 
> If userspace attempted to set a "security.capability" xattr shorter than
> 4 bytes (e.g. 'setfattr -n security.capability -v x file'), then
> cap_convert_nscap() read past the end of the buffer containing the xattr
> value because it accessed the ->magic_etc field without verifying that
> the xattr value is long enough to contain that field.
> 
> Fix it by validating the xattr value size first.
> 
> This bug was found using syzkaller with KASAN.  The KASAN report was as
> follows (cleaned up slightly):
> 
>     BUG: KASAN: slab-out-of-bounds in cap_convert_nscap+0x514/0x630 security/commoncap.c:498
>     Read of size 4 at addr ffff88002d8741c0 by task syz-executor1/2852
> 
>     CPU: 0 PID: 2852 Comm: syz-executor1 Not tainted 4.15.0-rc6-00200-gcc0aac99d977 #253
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
>     Call Trace:
>      __dump_stack lib/dump_stack.c:17 [inline]
>      dump_stack+0xe3/0x195 lib/dump_stack.c:53
>      print_address_description+0x73/0x260 mm/kasan/report.c:252
>      kasan_report_error mm/kasan/report.c:351 [inline]
>      kasan_report+0x235/0x350 mm/kasan/report.c:409
>      cap_convert_nscap+0x514/0x630 security/commoncap.c:498
>      setxattr+0x2bd/0x350 fs/xattr.c:446
>      path_setxattr+0x168/0x1b0 fs/xattr.c:472
>      SYSC_setxattr fs/xattr.c:487 [inline]
>      SyS_setxattr+0x36/0x50 fs/xattr.c:483
>      entry_SYSCALL_64_fastpath+0x18/0x85
> 
> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>

Thanks, Eric!

Reviewed-by: Serge Hallyn <serge@xxxxxxxxxx>

> ---
>  security/commoncap.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 4f8e09340956..48620c93d697 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -348,21 +348,18 @@ static __u32 sansflags(__u32 m)
>  	return m & ~VFS_CAP_FLAGS_EFFECTIVE;
>  }
>  
> -static bool is_v2header(size_t size, __le32 magic)
> +static bool is_v2header(size_t size, const struct vfs_cap_data *cap)
>  {
> -	__u32 m = le32_to_cpu(magic);
>  	if (size != XATTR_CAPS_SZ_2)
>  		return false;
> -	return sansflags(m) == VFS_CAP_REVISION_2;
> +	return sansflags(le32_to_cpu(cap->magic_etc)) == VFS_CAP_REVISION_2;
>  }
>  
> -static bool is_v3header(size_t size, __le32 magic)
> +static bool is_v3header(size_t size, const struct vfs_cap_data *cap)
>  {
> -	__u32 m = le32_to_cpu(magic);
> -
>  	if (size != XATTR_CAPS_SZ_3)
>  		return false;
> -	return sansflags(m) == VFS_CAP_REVISION_3;
> +	return sansflags(le32_to_cpu(cap->magic_etc)) == VFS_CAP_REVISION_3;
>  }
>  
>  /*
> @@ -405,7 +402,7 @@ int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer,
>  
>  	fs_ns = inode->i_sb->s_user_ns;
>  	cap = (struct vfs_cap_data *) tmpbuf;
> -	if (is_v2header((size_t) ret, cap->magic_etc)) {
> +	if (is_v2header((size_t) ret, cap)) {
>  		/* If this is sizeof(vfs_cap_data) then we're ok with the
>  		 * on-disk value, so return that.  */
>  		if (alloc)
> @@ -413,7 +410,7 @@ int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer,
>  		else
>  			kfree(tmpbuf);
>  		return ret;
> -	} else if (!is_v3header((size_t) ret, cap->magic_etc)) {
> +	} else if (!is_v3header((size_t) ret, cap)) {
>  		kfree(tmpbuf);
>  		return -EINVAL;
>  	}
> @@ -470,9 +467,9 @@ static kuid_t rootid_from_xattr(const void *value, size_t size,
>  	return make_kuid(task_ns, rootid);
>  }
>  
> -static bool validheader(size_t size, __le32 magic)
> +static bool validheader(size_t size, const struct vfs_cap_data *cap)
>  {
> -	return is_v2header(size, magic) || is_v3header(size, magic);
> +	return is_v2header(size, cap) || is_v3header(size, cap);
>  }
>  
>  /*
> @@ -495,7 +492,7 @@ int cap_convert_nscap(struct dentry *dentry, void **ivalue, size_t size)
>  
>  	if (!*ivalue)
>  		return -EINVAL;
> -	if (!validheader(size, cap->magic_etc))
> +	if (!validheader(size, cap))
>  		return -EINVAL;
>  	if (!capable_wrt_inode_uidgid(inode, CAP_SETFCAP))
>  		return -EPERM;
> -- 
> 2.15.1