| Message ID | 20180101152831.13085-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Eric Biggers (ebiggers3@gmail.com): > From: Eric Biggers <ebiggers@google.com> > > 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@vger.kernel.org> # v4.14+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks, Eric! Reviewed-by: Serge Hallyn <serge@hallyn.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;