| Message ID | 20180327033800.3081-1-ebiggers3@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ext4: limit external inode xattrs to XATTR_SIZE_MAX | expand |
On Mar 26, 2018, at 9:38 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > > From: Eric Biggers <ebiggers@google.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@gatech.edu> > Fixes: e50e5129f384 ("ext4: xattr-in-inode support") > Cc: <stable@vger.kernel.org> # v4.13+ > Signed-off-by: Eric Biggers <ebiggers@google.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
Hi Andreas, On Tue, Mar 27, 2018 at 02:13:20PM -0600, Andreas Dilger wrote: > On Mar 26, 2018, at 9:38 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > > > > From: Eric Biggers <ebiggers@google.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. > To be clear, we already return -ERANGE if the xattr is larger than the passed buffer. The question is what to do if the user requests the size of an xattr. In the code, this is the case where the buffer is NULL. We could allow returning any size up to INT_MAX. But large sizes are problematic because some callers will actually allocate that amount of memory. That's what ext4_get_acl() does, and it uses kmalloc(). Among the issues with that is that if the size is over KMALLOC_MAX_SIZE, then the WARN_ON() in kmalloc_slab() will be hit, which fuzzers will report as a kernel bug. So if we are going to allow very large sizes to be reported, we need to go through any places in the kernel that read variable-size xattrs and either limit the size for that type of xattr (e.g. saying that ACLs can only be up to 64K, or 1 MB, or whatever), or switch to kvmalloc() with GFP_NOWARN so that large allocations are handled more gracefully. Eric
On Mar 27, 2018, at 3:43 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > > Hi Andreas, > > On Tue, Mar 27, 2018 at 02:13:20PM -0600, Andreas Dilger wrote: >> On Mar 26, 2018, at 9:38 PM, Eric Biggers <ebiggers3@gmail.com> wrote: >>> >>> From: Eric Biggers <ebiggers@google.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. >> > > To be clear, we already return -ERANGE if the xattr is larger than the passed > buffer. The question is what to do if the user requests the size of an xattr. > In the code, this is the case where the buffer is NULL. > > We could allow returning any size up to INT_MAX. But large sizes are > problematic because some callers will actually allocate that amount of memory. > That's what ext4_get_acl() does, and it uses kmalloc(). Among the issues with > that is that if the size is over KMALLOC_MAX_SIZE, then the WARN_ON() in > kmalloc_slab() will be hit, which fuzzers will report as a kernel bug. So if we > are going to allow very large sizes to be reported, we need to go through any > places in the kernel that read variable-size xattrs and either limit the size > for that type of xattr (e.g. saying that ACLs can only be up to 64K, or 1 MB, or > whatever), or switch to kvmalloc() with GFP_NOWARN so that large allocations are > handled more gracefully. Given that kmalloc() of a 64KB ACL could already fail due to fragmentation of free memory, it probably makes sense that this be changed to kvmalloc/kvfree() in any case? Cheers, Andreas
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 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(). I agree with Andreas that that the primary check should be INT_MAX, although I'll note that in actual practice if we want to expand the XATTR_MAX_SIZE we probably wouldn't want to do that as a ro_compat feature bit. Cheers, - Ted
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
On Thu, Mar 29, 2018 at 08:43:02PM -0700, Eric Biggers wrote: > > 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. You're right, I misparsed your patch. It also wasn't how the code pre the Fixes: commit how the POC worked, since it wasn't as explicit. > > > > 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. Well, it's not necessary xattr code that could do this. For example,ht you can have a bitmap block pointing at an xattr block, or a extent tree leaf block and an xattr block mapped onto the same physical block. The first case can be detected with block_validity (which is now enabled by default, but which can be turned off). The second is one that we can't necessarily detect. It might be because of a bug, or a hardware issue (e.g., a media error), or due to a maliciously introduced 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. See above. The problem is if a block is concurrently modified by two different pieces of ext4 code, due to a maliciously crafted image. Some of this could be detected by annotating the buffer head if we notice that a block is used by two different use cases. That would be a non-trivial change, but that would eliminate most of these issues. It might be possible to eliminate all of them, but it gets tricky to make sure we handle all of the cases --- for example, in the the data=journalled mode where data blocks also get modified through buffer_heads. This wasn't what was going on here, but one of the other POC's it was the case that we ran into the problem where a bitmap allocation block was twiddling the superblock. So when I misinterpreted what had been going on above, I had assumed that this was another case where a metadata block was getting modified in two different ways as two different types of metadata. Cheers, - Ted
On Fri, Mar 30, 2018 at 01:02:45PM -0400, Theodore Y. Ts'o wrote: > On Thu, Mar 29, 2018 at 08:43:02PM -0700, Eric Biggers wrote: > > > > 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. > > You're right, I misparsed your patch. It also wasn't how the code pre > the Fixes: commit how the POC worked, since it wasn't as explicit. > For xattrs not stored in external inodes we validate that their size fits in the xattr block, or inline in the inode. So their sizes cannot get anywhere close to INT_MAX (though there were a couple bugs I fixed previously, see d7614cc16146). > > > > > > 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. > > Well, it's not necessary xattr code that could do this. For > example,ht you can have a bitmap block pointing at an xattr block, or > a extent tree leaf block and an xattr block mapped onto the same > physical block. The first case can be detected with block_validity > (which is now enabled by default, but which can be turned off). The > second is one that we can't necessarily detect. It might be because > of a bug, or a hardware issue (e.g., a media error), or due to a > maliciously introduced bug. > That's what I meant by someone else modifying the buffer_head concurrently. It can also happen if someone writes directly to the block device node. > > 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. > > See above. The problem is if a block is concurrently modified by two > different pieces of ext4 code, due to a maliciously crafted image. > Some of this could be detected by annotating the buffer head if we > notice that a block is used by two different use cases. That would be > a non-trivial change, but that would eliminate most of these issues. > It might be possible to eliminate all of them, but it gets tricky to > make sure we handle all of the cases --- for example, in the the > data=journalled mode where data blocks also get modified through > buffer_heads. > > This wasn't what was going on here, but one of the other POC's it was > the case that we ran into the problem where a bitmap allocation block > was twiddling the superblock. So when I misinterpreted what had been > going on above, I had assumed that this was another case where a > metadata block was getting modified in two different ways as two > different types of metadata. > But, if we're actually operating under the assumption that someone can modify the xattr block's buffer_head concurrently, we'd actually need to copy it to a temporary buffer and validate it there, rather than reading directly from the cache. Likewise for the other ext4 metadata such as directory blocks. Checking one specific field in one specific place is not nearly enough. And if we did read directly from the cache we'd have to use volatile memory accesses. Eric
On Fri, Mar 30, 2018 at 12:32:12PM -0700, Eric Biggers wrote: > But, if we're actually operating under the assumption that someone can modify > the xattr block's buffer_head concurrently, we'd actually need to copy it to a > temporary buffer and validate it there, rather than reading directly from the > cache. Likewise for the other ext4 metadata such as directory blocks. Checking > one specific field in one specific place is not nearly enough. And if we did > read directly from the cache we'd have to use volatile memory accesses. We don't need to copy *all* of the the block and validate it *all*. We just need to validate the bits which might cause a buffer overrun. For example: diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index b0a9ba8e576a..7b7e40a05419 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -547,8 +547,14 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name, if (error) goto cleanup; } else { - memcpy(buffer, bh->b_data + - le16_to_cpu(entry->e_value_offs), size); + __u16 offset = le16_to_cpu(entry->e_value_offs); + unsigned long blocksize = inode->i_sb->s_s_blocksize; + + error = -ERANGE; + if (unlikely((offset >= blocksize) || + ((offset + size) >= blocksize))) + goto cleanup; + memcpy(buffer, bh->b_data + offset, size); } } error = size; See? This is much more efficient, doesn't require that we do a memory allocation for the temp buffer (which might fail), doesn't require any locking (which would be the other way to try to solve the problem), etc. - Ted P.S. We should change all of the return -ERANGE to -EFSCORRUPTED and arrange to have ext4_error() getting called. Another cleanup patch. The fs/ext4/xattr.c file is ripe for a lot of cleanup / robustification patches...
I've done the following which should hopefully make you happier and things clearer. First of all, I've taken the extra checks and moved out of this commit. So it now looks pretty much like your original proposed patch. Then I've added two separate patches to add better bounds checking to the xattr read and find path. There is almost certainly more paranoia checks that could be added later --- in particular in the xattr set codepaths --- but this is the low-hanging fruit to make life more interesting for people doing research in file system fuzzing tools. :-) - Ted From ce3fd194fcc6fbdc00ce095a852f22df97baa401 Mon Sep 17 00:00:00 2001 From: Eric Biggers <ebiggers@google.com> Date: Thu, 29 Mar 2018 14:31:42 -0400 Subject: [PATCH] ext4: limit xattr size to INT_MAX ext4 isn't validating the sizes of xattrs where the value of the xattr is stored in an external inode. 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. 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 Fixes: e50e5129f384 ("ext4: xattr-in-inode support") --- 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 63656dbafdc4..2077d87b09f2 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 > INT_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; /*
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; /*