linux-ext4 - Re: [PATCH -v2] ext4: limit xattr size to INT

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: <20180330195016.GC180083@gmail.com>
Date:   Fri, 30 Mar 2018 12:50:16 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        adilger@...ger.ca, wen.xu@...ech.edu, ebiggers@...gle.com,
        stable@...r.kernel.org
Subject: Re: [PATCH -v2] ext4: limit xattr size to INT_MAX

Hi Ted,

On Fri, Mar 30, 2018 at 03:13:20PM -0400, Theodore Ts'o wrote:
>
> Also if the xattr block is corrupted, mark the file system as
> containing an error.

Weren't we doing that already?

> 
> This issue has been assigned CVE-2018-1095.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=199185
> https://bugzilla.redhat.com/show_bug.cgi?id=1560793
> 
> Reported-by: Wen Xu <wen.xu@...ech.edu>
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> Cc: stable@...r.kernel.org

I'm still confused why you removed my Fixes: line and the mentions of the bug
affecting external inode xattrs only.  Wasn't the size validation correct before
then?  We might want to send d7614cc16146 to the stable trees, but after that
the sizes were being validated correctly, right?

> ---
>  fs/ext4/xattr.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 63656dbafdc4..d2a9b078e121 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -195,10 +195,14 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
>  
>  	/* Check the values */
>  	while (!IS_LAST_ENTRY(entry)) {
> +		u32 size = le32_to_cpu(entry->e_value_size);
> +
> +		if (size > INT_MAX)
> +			return -EFSCORRUPTED;
> +
>  		if (entry->e_value_size != 0 &&
>  		    entry->e_value_inum == 0) {

Use 'size' instead of 'entry->e_value_size' here, like in my original patch?

>  			u16 offs = le16_to_cpu(entry->e_value_offs);
> -			u32 size = le32_to_cpu(entry->e_value_size);
>  			void *value;
>  
>  			/*
> @@ -523,8 +527,10 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
>  	if (error)
>  		goto cleanup;
>  	size = le32_to_cpu(entry->e_value_size);
> +	error = -ERANGE;
> +	if (unlikely(size > INT_MAX))
> +		goto cleanup;


>  	if (buffer) {
> -		error = -ERANGE;
>  		if (size > buffer_size)
>  			goto cleanup;
>  		if (entry->e_value_inum) {
> @@ -572,8 +578,10 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name,
>  	if (error)
>  		goto cleanup;
>  	size = le32_to_cpu(entry->e_value_size);
> +	error = -ERANGE;
> +	if (unlikely(size > INT_MAX))
> +		goto cleanup;

I still think the new checks here are misleading and shouldn't be added.  If
someone can actually modify the buffer_head concurrently then they could just
make the size larger than the block but <= INT_MAX, so that the following
page(s) are also copied to the xattr, disclosing memory or crashing.  Or they
could modify ->e_value_offs to point past the block.  Also since this is not
using a volatile memory access, the compiler is free to reload the value and
assume it's the same.

- Eric