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: <20180330034302.GA712@sol.localdomain>
Date:   Thu, 29 Mar 2018 20:43:02 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     Andreas Dilger <adilger@...ger.ca>,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        Wen Xu <wen.xu@...ech.edu>, Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] ext4: limit external inode xattrs to XATTR_SIZE_MAX

Hi Ted,

On Thu, Mar 29, 2018 at 03:41:25PM -0400, Theodore Y. Ts'o wrote:
> I tried a variation of this patch (checking against INT_MAX) instead
> of XATTR_MAX_SIZE, but it's not sufficient to address the BUG() in
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=199185
> 
> The reason for that is that initially the xattr block is OK:
> 
> [   15.236890] xattr_check_block: 2047 0
> 
> but afterwards, due to other file system corruptions (remember, this
> is a carefully corrupted file system by malicious attacker), the xattr
> block gets corrupted in memory, and then *subsequent* checks of the
> block are skipped because the "buffer verified" bit is set:
> 
> [   15.239204] EXT4-fs error (device vdc): mb_free_blocks:1456: group 0, inode 16: block 1985:freeing already freed block (bit 1984); block bitmap corrupt.
> [   15.243899] EXT4-fs error (device vdc): ext4_mb_generate_buddy:744: group 0, block bitmap and bg descriptor inconsistent: 960 vs 961 free clusters
> [   15.248943] xattr_check_block skip: 2047
> [   15.251442] xattr_check_block skip: 2047
> [   15.252642] ext4_xattr_block_get ERANGE: 2047
> 
> Hence, if we don't add a specific check in ext4_xattr_block_get(), we
> still return an overflowed size as an error check, and then kernel
> will still BUG.

I think this analysis is wrong.  The check in ext4_xattr_check_entries() is
sufficient for the given POC provided that the check is done for all xattrs,
like my original patch did.  Your modified version of my patch doesn't do the
check when entry->e_value_inum != 0, but that's exactly the case that was
missing the size check and is triggered by the POC.  Also note that this bug was
introduced with the EA inode feature; again, see my original patch, which gave
the Fixes: line and noted that the size validation was missing only for xattrs
stored in external inodes.

> 
> I think it's fair to add an INT_MAX check to
> ext4_xattr_check_entries(), but we're still going to need to have
> checks in ext4_xattr_block_get() and ext4_xattr_ibody_get().
> 

This isn't applicable given my comment above, but even if it was, this doesn't
make sense.  If the xattr code itself can corrupt the xattr list, then that is
its own, different bug.  Else, we are talking about someone else modifying the
buffer head concurrently, which seems to be well outside the threat model
assumed by ext4 for its metadata (not just xattrs but also directory blocks,
inodes, etc.).  For ext4 to actually parse its metadata securely under the
assumption that the copy in the block device's address_space can be concurrently
modified, it would need to be copied to a temporary buffer and re-validated on
every access.

Eric