linux-ext4 - Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <5EC6633E-9B27-44D5-8308-0B540D367948@dilger.ca>
Date:   Tue, 27 Mar 2018 14:13:20 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>, Wen Xu <wen.xu@...ech.edu>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE_MAX

On Mar 26, 2018, at 9:38 PM, Eric Biggers <ebiggers3@...il.com> wrote:
> 
> From: Eric Biggers <ebiggers@...gle.com>
> 
> This is a replacement for the broken patch "ext4: add better range
> checking for e_value_size in xattrs" currently in ext4/dev.
> 
> -----8<-----
> 
> ext4 isn't validating the sizes of xattrs that are stored in external
> inodes.  This is problematic because ->e_value_size is a u32, but
> ext4_xattr_get() returns an int.  A very large size is misinterpreted as
> an error code, which ext4_get_acl() translates into a bogus ERR_PTR()
> for which IS_ERR() returns false, causing a crash.
> 
> Fix this by validating that all xattrs are <= XATTR_SIZE_MAX bytes.
> (It's not strictly needed for non-EA-inode xattrs, but it doesn't hurt.)

Hmm, I'm not so keen on this check.  XATTR_SIZE_MAX is a current, rather
arbitrary, limit in the kernel, but there is no reason the on-disk format
can't allow a larger xattr to be stored.  Large xattrs would potentially
be useful for in-ext4 storage of per-block checksums on an xattr for each
file, or other uses beyond what the xattr userspace interface allows.

We used to define another (also arbitrary) limit of 1MB for xattr inodes
in the Lustre code, but it was not actually needed and was dropped.

IMHO, it would make more sense to validate e_value_size against i_size
(if we want to pay the expense of reading the external xattr at lookup
time, otherwise only do it when the external xattr inode is actually read),
and return -EFSCORRUPTED if they don't match, or if the xattr is over
2^31 bytes in size.  Otherwise, the filesystem will be mounted read-only,
but this may be a legitimate xattr from a newer kernel and a needless DOS.

At most this should return -ERANGE if the xattr is larger than the passed
buffer (which will currently be 64KB, but might be larger in the future),
but it can't be done from within ext4_xattr_check_entries(), since the caller
will call __ext4_error_inode() if any error is returned.

Cheers, Andreas

> https://bugzilla.kernel.org/show_bug.cgi?id=199185
> 
> Reported-by: Wen Xu <wen.xu@...ech.edu>
> Fixes: e50e5129f384 ("ext4: xattr-in-inode support")
> Cc: <stable@...r.kernel.org> # v4.13+
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> fs/ext4/xattr.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 63656dbafdc45..8c9ade64aea2a 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -195,10 +195,13 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> 
> 	/* Check the values */
> 	while (!IS_LAST_ENTRY(entry)) {
> -		if (entry->e_value_size != 0 &&
> -		    entry->e_value_inum == 0) {
> +		u32 size = le32_to_cpu(entry->e_value_size);
> +
> +		if (size > XATTR_SIZE_MAX)
> +			return -EFSCORRUPTED;
> +
> +		if (size != 0 && entry->e_value_inum == 0) {
> 			u16 offs = le16_to_cpu(entry->e_value_offs);
> -			u32 size = le32_to_cpu(entry->e_value_size);
> 			void *value;
> 
> 			/*
> --
> 2.16.2
> 

Cheers, Andreas

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)