| 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
| ||
|
Date: Sat, 27 Aug 2022 15:51:44 +0800
From: Hawkins Jiawei <yin31149@...il.com>
To: chenxiaosong2@...wei.com
Cc: akpm@...ux-foundation.org, anton@...era.com,
linux-kernel@...r.kernel.org, linux-ntfs-dev@...ts.sourceforge.net,
syzbot+5f8dcabe4a3b2c51c607@...kaller.appspotmail.com,
syzkaller-bugs@...glegroups.com, yin31149@...il.com
Subject: Re: [PATCH] ntfs: change check order in ntfs_attr_find
On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@...wei.com> wrote:
>
> 在 2022/8/26 20:27, Hawkins Jiawei 写道:
> > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > Looks like it is improper check order that causes this bug.
> >
> > Signed-off-by: Hawkins Jiawei <yin31149@...il.com>
> > ---
> > fs/ntfs/attrib.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > u8 *mrec_end = (u8 *)ctx->mrec +
> > le32_to_cpu(ctx->mrec->bytes_allocated);
> > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > + break;
> > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > a->name_length * sizeof(ntfschar);
> > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > - name_end > mrec_end)
> > + if (name_end > mrec_end)
> > break;
> > ctx->attr = a;
> > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> >
>
> The reason is that a->length is 0, it will occur uaf when deref any
> field of ATTR_RECORD.
>
> It seems that changing check order will not fix root cause, if the
> condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false,
> uaf will still occur.
>
> Do you have any thoughts on this ?
Hi, chenxiaosong
I think changing the check order is able to fix this bug. But we may need
to check whether mft record header is out of bounds, maybe
"if ((u8*)a < (u8*)ctx->mrec || (u8*)a + sizeof(MFT_RECORD) > mrec_end)"
Because we just need to check if this ATTR_RECORD is in valid addresss. As for
situation that a->length is 0, there seems already a check in the loop
(Correct me if I am wrong):
> static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> const u32 name_len, const IGNORE_CASE_BOOL ic,
> const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
> {
> ...
>
> for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> u8 *mrec_end = (u8 *)ctx->mrec +
> le32_to_cpu(ctx->mrec->bytes_allocated);
> u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> a->name_length * sizeof(ntfschar);
> if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> name_end > mrec_end)
> break;
> ctx->attr = a;
> if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> a->type == AT_END))
> return -ENOENT;
> if (unlikely(!a->length))
> break;
> ...
> }
And as for the root cause for use-after-free read, I think it is the
ctx->attr->length to be blamed.
To be more specific, when kernel loads the struct MFT_RECORD from disk
in ntfs_read_inode_mount(), m's attrs_offset field should less than
m's bytes_allocated field, or it may out of the bounds:
int ntfs_read_inode_mount(struct inode *vi)
{
...
MFT_RECORD *m = NULL;
i = vol->mft_record_size;
...
m = (MFT_RECORD*)ntfs_malloc_nofs(i);
/* Determine the first block of the $MFT/$DATA attribute. */
block = vol->mft_lcn << vol->cluster_size_bits >>
sb->s_blocksize_bits;
nr_blocks = vol->mft_record_size >> sb->s_blocksize_bits;
...
/* Load $MFT/$DATA's first mft record. */
for (i = 0; i < nr_blocks; i++) {
bh = sb_bread(sb, block++);
if (!bh) {
ntfs_error(sb, "Device read failed.");
goto err_out;
}
memcpy((char*)m + (i << sb->s_blocksize_bits), bh->b_data,
sb->s_blocksize);
brelse(bh);
}
if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
goto err_out;
}
...
ctx = ntfs_attr_get_search_ctx(ni, m);
if (!ctx) {
err = -ENOMEM;
goto err_out;
}
/* Find the attribute list attribute if present. */
err = ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, 0, 0, NULL, 0, ctx);
...
}
> ==================================================================
> BUG: KASAN: use-after-free in ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> Read of size 2 at addr ffff88807e352009 by task syz-executor153/3607
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> print_address_description mm/kasan/report.c:317 [inline]
> print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
> kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
> ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> ntfs_attr_lookup+0x1056/0x2070 fs/ntfs/attrib.c:1193
> ntfs_read_inode_mount+0x89a/0x2580 fs/ntfs/inode.c:1845
> ntfs_fill_super+0x1799/0x9320 fs/ntfs/super.c:2854
> mount_bdev+0x34d/0x410 fs/super.c:1400
> legacy_get_tree+0x105/0x220 fs/fs_context.c:610
> vfs_get_tree+0x89/0x2f0 fs/super.c:1530
> do_new_mount fs/namespace.c:3040 [inline]
> path_mount+0x1326/0x1e20 fs/namespace.c:3370
> do_mount fs/namespace.c:3383 [inline]
> __do_sys_mount fs/namespace.c:3591 [inline]
> __se_sys_mount fs/namespace.c:3568 [inline]
> __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [...]
> </TASK>
> Memory state around the buggy address:
> ffff88807e351f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff88807e351f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff88807e352000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff88807e352080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff88807e352100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
So check these fields as follow should fix the root cause for this
use-after-free bug(Correct me if I am wrong). I test this patch locally,
it doesn't trigger any issue.
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index db0f1995aedd..6ba99e109ca9 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1822,6 +1822,11 @@ int ntfs_read_inode_mount(struct inode *vi)
goto err_out;
}
+ if (m->attrs_offset >= le32_to_cpu(m->bytes_allocated)) {
+ ntfs_error(sb, "Incorrect mft record attrs_offset %u", m->attrs_offset);
+ goto err_out;
+ }
+
/* Apply the mst fixups. */
if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
/* FIXME: Try to use the $MFTMirr now. */
What's your opinion?
Powered by blists - more mailing lists