linux-kernel - Re: [PATCH v3] exfat: integrates dir-entry getting and validation

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] [day] [month] [year] [list]

Message-ID: <2b4c4409-7cb8-1651-3966-569636bcc429@gmail.com>
Date:   Thu, 27 Aug 2020 18:59:48 +0900
From:   Tetsuhiro Kohada <kohada.t2@...il.com>
To:     Namjae Jeon <namjae.jeon@...sung.com>
Cc:     kohada.tetsuhiro@...mitsubishielectric.co.jp,
        mori.takahiro@...mitsubishielectric.co.jp,
        motai.hirotaka@...mitsubishielectric.co.jp,
        'Sungjong Seo' <sj1557.seo@...sung.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] exfat: integrates dir-entry getting and validation

>>>>>>> -	/* validiate cached dentries */
>>>>>>> -	for (i = 1; i < num_entries; i++) {
>>>>>>> -		ep = exfat_get_dentry_cached(es, i);
>>>>>>> -		if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
>>>>>>> +	ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
>>>>>>> +	if (!ep || ep->type != EXFAT_STREAM)
>>>>>>> +		goto free_es;
>>>>>>> +	es->de[ENTRY_STREAM] = ep;
>>>>>>
>>>>>> The value contained in stream-ext dir-entry should not be used
>>>>>> before validating the EntrySet
>>>> checksum.
>>>>>> So I would insert EntrySet checksum validation here.
>>>>>> In that case, the checksum verification loop would be followed by
>>>>>> the TYPE_NAME verification loop, can you acceptable?
>>>>> Yes. That would be great.
>>>>
>>>> OK.
>>>> I'll add TYPE_NAME verification after checksum verification, in next patch.
>>>> However, I think it is enough to validate TYPE_NAME when extracting name.
>>>> Could you please tell me why you think you need TYPE_NAME validation here?
>>> I've told you on previous mail. This function should return validated
>>> dentry set after checking
>>> file->stream->name in sequence.
>>
>> Yes. I understand that the current implementation checks in that order.
>> Sorry, my question was unclear.
>> Why do you think you should leave the TYPE_NAME validation in this function?
>> What kind of problem are you worried about if this function does not validate TYPE_NAME?
>> (for preserve the current behavior?)
> We have not checked the problem when it is removed because it was implemented
> according to the specification from the beginning.

I understand that the main reason to validate TYPE_NAME here is "according to the specification".
(No one knows the actual problem)

First, we should validate as 'dir-entry set' by SecondaryCount and SetChecksum described
in "6.3 Generic Primary DirectoryEntry Template".

Next, description about validity of 'File dir-entry set' is ...
7.4 File Directory Entry:
... For a File directory entry to be valid, exactly one Stream Extension directory entry and at least
one File Name directory entry must immediately follow the File directory entry.
7.7 File Name Directory Entry:
... File Name directory entries are valid only if they immediately follow the Stream Extension
directory entry as a consecutive series.

It is possible to validate the above correctly, with either exfat_get_dentry_set() or
exfat_get_uniname_from_name_entries().
Is this wrong?

> And your v3 patch are
> already checking the name entries as TYPE_SECONDARY. And it check them with
> TYPE_NAME again in exfat_get_uniname_from_ext_entry(). 

This is according to "6.3 Generic Primary DirectoryEntry Template".
"6.3 Generic Primary DirectoryEntry Template" only required TYPE_SECONDARY.
In v3, there is no checksum validation yet.

> If you check TYPE_NAME
> with stream->name_len, We don't need to perform the loop for extracting
> filename from the name entries if stream->name_len or name entry is invalid.

Don't worry, it's a rare case.
(Do you care about the run-time costs?)

> And I request to prove why we do not need to validate name entries in this
> function calling from somewhere. 

If you need, it's okey to validate in both.
However, name-length and type validation and name-extraction should not be separated.
These are closely related, so these should be placed physically and temporally close.

Well, why it's unnecessary.
Both can be validate correctly, as I wrote before.
And, I don't really trust the verification with TYPE_NAME.
(reliability of validation as 'file dir-entry set' by checksum is much higher)

> So as I suggested earlier, You can make it
> with an argument flags so that we skip the validation.

No need skip the validation, I think.
The run-time costs for validation are pretty low.
The reason I want to remove the validation is because I want to keep the code simple.
(KISS principle)

BR
---
Tetsuhiro Kohada <kohada.t2@...il.com>