[-v2] ext4: limit xattr size to INT_MAX

From: Eric Biggers <ebiggers@google.com>

ext4 isn't validating the sizes of xattrs.  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 <= INT_MAX bytes.  Also add
explicit checks in ext4_xattr_block_get() and ext4_xattr_ibody_get()
just in case the xattr block is corrupted in memory.

Also if the xattr block is corrupted, mark the file system as
containing an error.

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@gatech.edu>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/xattr.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

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@gatech.edu>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.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

On Fri, Mar 30, 2018 at 12:50:16PM -0700, Eric Biggers wrote:
> 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?

Actually, not everywhere, but I decided to move that into a separate
commit and forgot to remove this from the description.  See the commit
"ext4: move call to ext4_error() into ext4_xattr_check_block()".

> > 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@gatech.edu>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Cc: stable@vger.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?

Sorry, I forgot to add back the Fixes line.   I'll fix that.

I removed the text because it was confusing me when I was originally
parsing it.  (The details of the ea-in-inode code had been swapped
out, I'm embarassed to say.)  I do see what you are driving at, and
I'll add some text that makes the point you are trying to make.

> 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.

The most common case where we run into this problem is where it's not
a CPU-CPU race, but rather where the buffer head gets read into memory
and is validated, and then minutes later, file system corruption
causes the buffer head to be modifeid.  So I wasn't worried about
races where we would need to copy the buffer to a temp buffer, and
validate it every time before using it.

You're right that against someone who has both malicoiusly crafted the
corrupted file system, *and* maliciously crafts the access patterns to
deliberately trigger a race, the check that I've added isn't
sufficient.  Unfortunately, doing the copy and validate every time is
a problem from a performance perspective.

The condition I added does protect us against a class of attacks.  But
it is not a universal protection, agreed.  The way to fix the concern
you raised would be to add a check for
le16_to_cpu(entry->e_value_offs) before we using it.  But that's for a
different attack, and it's something we should add in a separate
commit.

Looking at fs/ext4/xattr.c, there are a number of places where we are
relying on buffer_verified() bit --- I'd call that a "target rich
environment" for fixes.  In my opinion, the check I added to fix this
POC attack, or an explicit check for entry->e_value_offs in
ext4_xattr_block_get() should be the primary protection for malformed
on-disk data structures that might lead to buffer overflow attacks,
since it eliminates the TOCTTOU gap.  The buffer_verified bit should
be a backup just in case we missed a check.

Cheers,

						- Ted

P.S.  If you are concerned that the extra checks makes it hard to find
bugs, I would much rather add a #ifdef which disable the the
ext4_xattr_*verify() functions, and see if Wen Xu's can find problems
--- and if so, we should fix them.