[PATCH] ntfs: Fix validity check for file name attribute

[PATCH] ntfs: Fix validity check for file name attribute

[Desmond Cheong Zhi Xi]

When checking the file name attribute, we want to ensure that it fits
within the bounds of ATTR_RECORD. To do this, we should check
that (attr record + file name offset + file name length) < (attr
record + attr record length).

However, the original check did not include the file name offset in
the calculation. This means that corrupted on-disk metadata might not
caught by the incorrect file name check, and lead to an invalid memory
access.

An example can be seen in the crash report of a memory corruption
error found by Syzbot:
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246

Adding the file name offset to the validity check fixes this error and
passes the Syzbot reproducer test.

Reported-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
Tested-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 fs/ntfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index f5c058b3192c..4474adb393ca 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
 		}
 		file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
 				le16_to_cpu(attr->data.resident.value_offset));
-		p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
+		p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
 		if (p2 < (u8*)attr || p2 > p)
 			goto err_corrupt_attr;
 		/* This attribute is ok, but is it in the $Extend directory? */
-- 
2.25.1

Re: [PATCH] ntfs: Fix validity check for file name attribute

[Desmond Cheong Zhi Xi]

On 14/6/21 1:05 pm, Desmond Cheong Zhi Xi wrote:
> When checking the file name attribute, we want to ensure that it fits
> within the bounds of ATTR_RECORD. To do this, we should check
> that (attr record + file name offset + file name length) < (attr
> record + attr record length).
> 
> However, the original check did not include the file name offset in
> the calculation. This means that corrupted on-disk metadata might not
> caught by the incorrect file name check, and lead to an invalid memory
> access.
> 
> An example can be seen in the crash report of a memory corruption
> error found by Syzbot:
> https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
> 
> Adding the file name offset to the validity check fixes this error and
> passes the Syzbot reproducer test.
> 
> Reported-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
> Tested-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>   fs/ntfs/inode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index f5c058b3192c..4474adb393ca 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
>   		}
>   		file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
>   				le16_to_cpu(attr->data.resident.value_offset));
> -		p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
> +		p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
>   		if (p2 < (u8*)attr || p2 > p)
>   			goto err_corrupt_attr;
>   		/* This attribute is ok, but is it in the $Extend directory? */
> 

Hi Anton,

Any chance to review this patch?

Best wishes,
Desmond

Re: [PATCH] ntfs: Fix validity check for file name attribute

[Anton Altaparmakov]

Hi,

Thanks for the patch!  Have asked Andrew to merge it.

Best regards,

	Anton

> On 28 Jun 2021, at 03:45, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
> 
> On 14/6/21 1:05 pm, Desmond Cheong Zhi Xi wrote:
>> When checking the file name attribute, we want to ensure that it fits
>> within the bounds of ATTR_RECORD. To do this, we should check
>> that (attr record + file name offset + file name length) < (attr
>> record + attr record length).
>> However, the original check did not include the file name offset in
>> the calculation. This means that corrupted on-disk metadata might not
>> caught by the incorrect file name check, and lead to an invalid memory
>> access.
>> An example can be seen in the crash report of a memory corruption
>> error found by Syzbot:
>> https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
>> Adding the file name offset to the validity check fixes this error and
>> passes the Syzbot reproducer test.
>> Reported-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
>> Tested-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>>  fs/ntfs/inode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
>> index f5c058b3192c..4474adb393ca 100644
>> --- a/fs/ntfs/inode.c
>> +++ b/fs/ntfs/inode.c
>> @@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
>>  		}
>>  		file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
>>  				le16_to_cpu(attr->data.resident.value_offset));
>> -		p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
>> +		p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
>>  		if (p2 < (u8*)attr || p2 > p)
>>  			goto err_corrupt_attr;
>>  		/* This attribute is ok, but is it in the $Extend directory? */
> 
> Hi Anton,
> 
> Any chance to review this patch?
> 
> Best wishes,
> Desmond


-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: [PATCH] ntfs: Fix validity check for file name attribute

[Anton Altaparmakov]

Hi Andrew,

Please can you merge this patch?  I am also marking it for stable.

Thanks a lot in advance!

Best regards,

	Anton

---

When checking the file name attribute, we want to ensure that it fits
within the bounds of ATTR_RECORD. To do this, we should check
that (attr record + file name offset + file name length) < (attr
record + attr record length).

However, the original check did not include the file name offset in
the calculation. This means that corrupted on-disk metadata might not
caught by the incorrect file name check, and lead to an invalid memory
access.

An example can be seen in the crash report of a memory corruption
error found by Syzbot:
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246

Adding the file name offset to the validity check fixes this error and
passes the Syzbot reproducer test.

Reported-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
Tested-by: syzbot+213ac8bb98f7f4420840@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Acked-by: Anton Altaparmakov <anton@tuxera.com>
Cc: stable@vger.kernel.org
---
fs/ntfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index f5c058b3192c..4474adb393ca 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
		}
		file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
				le16_to_cpu(attr->data.resident.value_offset));
-		p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
+		p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
		if (p2 < (u8*)attr || p2 > p)
			goto err_corrupt_attr;
		/* This attribute is ok, but is it in the $Extend directory? */
-- 
2.25.1



-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: [PATCH] ntfs: Fix validity check for file name attribute

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

Hi,

I was just scanning through some older vulnerabilities and came across 
CVE-2018-12929, CVE-2018-12930, and CVE-2018-12931, which are all still open 
according to linuxkernelcves.com (originally reported against 4.15 [1]). I 
looked into the commits in fs/ntfs/ from 4.15 onwards to see if they were just 
missed, but I can't spot anything there. RedHat claims to have them fixed in 
one of their kernels [2].

Which makes me wonder if the issue fixed here is a duplicate of the any of the 
above. Is there a reason I can't find any patches for the original issue in 
tree, like the issue only introduced in a custom patchset that Ubuntu/RedHat 
were using? Is this thing worth it's own CVE if it's no duplicate?

Greetings,

Eike

1) https://marc.info/?t=152407734400002&r=1&w=2
2) https://access.redhat.com/errata/RHSA-2019:0641
-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]

Re: [PATCH] ntfs: Fix validity check for file name attribute

[Desmond Cheong Zhi Xi]

On 29/7/21 4:31 pm, Rolf Eike Beer wrote:
> Hi,
> 
> I was just scanning through some older vulnerabilities and came across
> CVE-2018-12929, CVE-2018-12930, and CVE-2018-12931, which are all still open
> according to linuxkernelcves.com (originally reported against 4.15 [1]). I
> looked into the commits in fs/ntfs/ from 4.15 onwards to see if they were just
> missed, but I can't spot anything there. RedHat claims to have them fixed in
> one of their kernels [2].
> 
> Which makes me wonder if the issue fixed here is a duplicate of the any of the
> above. Is there a reason I can't find any patches for the original issue in
> tree, like the issue only introduced in a custom patchset that Ubuntu/RedHat
> were using? Is this thing worth it's own CVE if it's no duplicate?
> 
> Greetings,
> 
> Eike
> 
> 1) https://marc.info/?t=152407734400002&r=1&w=2
> 2) https://access.redhat.com/errata/RHSA-2019:0641
> 

Hi Eike,

Thanks for digging into this. From a first glance, this bug seems most 
similar to CVE-2018-12929.

However, from the logs, the root causes are probably different. The 
cause of this bug is specifically in the call to 
ntfs_is_extended_system_file [1], but from what I can see this is not 
the case for CVE-2018-12929. I don't know enough to comment whether it 
needs a CVE, but it has been patched on Linux stable (up to 4.4).

It's worth noting that there's another similar bug that was fixed by 
Rustam Kovhaev (+cc) in ntfs_read_locked_inode [2]. This may or may not 
have been the issue in CVE-2018-12929.

Link: 
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246 
[1]

Link: 
https://syzkaller.appspot.com/bug?id=933dab9c03ac47a3d09dd4b0563a0a8fcb35f282 
[2]

Best wishes,
Desmond