Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

Austin S. Hemmelgarn wrote on 2016/03/18 07:17 -0400:
> On 2016-03-17 20:38, Qu Wenruo wrote:
>>
>>
>> Austin S. Hemmelgarn wrote on 2016/03/17 07:22 -0400:
>>> On 2016-03-17 05:04, Qu Wenruo wrote:
>>>>
>>>>
>>>> Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
>>>>> Currently, open_ctree_fs_info will open whatever path you pass it and
>>>>> try to interpret it as a BTRFS filesystem.  While this is not
>>>>> nessecarily dangerous (except possibly if done on a character device),
>>>>> it does result in some rather cryptic and non-sensical error messages
>>>>> when trying to run certain commands in ways they weren't intended
>>>>> to be
>>>>> run.  Add a check using stat(2) to verify that the path we've been
>>>>> passed is in fact a regular file or a block device.
>>>>>
>>>>> This causes the following commands to provide a helpful error message
>>>>> when run on a FIFO, directory, character device, or socket:
>>>>>      * btrfs check
>>>>>      * btrfs restore
>>>>>      * btrfs-image
>>>>>      * btrfs-find-root
>>>>>      * btrfs-debug-tree
>>>>>
>>>>> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
>>>>> ---
>>>>> This has been build and runtime tested on an x86-64 system with glibc.
>>>>> It has been build tested on x86-64 with uclibc.
>>>>> It has not been tested on Android or with musl, although it should
>>>>> work
>>>>> there also.
>>>>>
>>>>> There are other tools that have similarly bad behavior when called
>>>>> incorrectly (btrfs rescue immediately comes to mind), but they don't
>>>>> use open_ctree_fs_info, so this doesn't affect them.  I may do
>>>>> followup
>>>>> patches to fix those too if I have the time.
>>>>>
>>>>> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
>>>>> at all sure what exactly is going on there, and btrfs filesystem
>>>>> appears
>>>>> from my testing to behave exactly the same with this change, so I
>>>>> don't
>>>>> think this will have any effect on any of the btrfs filesystem
>>>>> commands.
>>>>>
>>>>>   disk-io.c | 7 +++++++
>>>>>   1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/disk-io.c b/disk-io.c
>>>>> index e520d80..d35153d 100644
>>>>> --- a/disk-io.c
>>>>> +++ b/disk-io.c
>>>>> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const
>>>>> char *filename,
>>>>>       int fp;
>>>>>       struct btrfs_fs_info *info;
>>>>>       int oflags = O_CREAT | O_RDWR;
>>>>> +    struct stat sb;
>>>>> +
>>>>> +    stat(filename, &sb);
>>>>> +    if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode &
>>>>> S_IFMT) == S_IFBLK))) {
>>>>> +        fprintf (stderr, "%s is not a regular file or block
>>>>> device\n", filename);
>>>>> +        return NULL;
>>>>> +    }
>>>>
>>>> This one seems to be too restrict.
>>>>
>>>> I prefer to block char/pipe/dir and some other obvious wrong ones other
>>>> than only allowing regular and block ones.
>>> Running against a directory gives a cryptic error about the superblock
>>> having bad info.  Running against a pipe is nonsensical, as it can't
>>> contain a filesystem.  Running against a character device is potentially
>>> dangerous (read operations are not guaranteed to be idempotent on
>>> character devices, depending on what hardware it is connected to, you
>>> could cause all kinds of odd things to happen).
>>>
>>> Everything this function gets called on is trying to get info from a
>>> unmounted filesystem image, which means that it only makes sense to try
>>> to parse things that can contain a unmounted filesystem image.
>>
>> Yes, I understand what you are doing.
>>
>> Just as I alreayd mentioned, the problem is, your current patch only
>> allowing regular and block device and will block valid soft link.
>> Just as Duncan mentioned, soft link should be allowed too.
> Symbolic links are allowed though.  stat(2) follows symlinks and returns
> information on their target, not the link itself.  I also tested it on
> symlinks, and it still works to run `btrfs check` on a link to a block
> device containing a BTRFS filesystem

Then I'm completely OK with current implementation.

Thanks,
Qu
>>
>> I mean to *block/prevent* char/pipe/dir instead of *only allowing*
>> regular/block device.
> I apologize for the misunderstanding, I misread your original message.
>
> As to blacklisting invalid types instead of whitelisting valid ones, I'd
> personally would prefer whitelisting in this particular case, as it
> results in both a smaller conditional, and makes the intent of only
> allowing things that can contain a BTRFS filesystem  more clear (at
> least, it does to me).
>>
>> Thanks,
>> Qu
>>>>
>>>> Thanks,
>>>> Qu
>>>>>
>>>>>       if (!(flags & OPEN_CTREE_WRITES))
>>>>>           oflags = O_RDONLY;
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>