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

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

[Austin S. Hemmelgarn]

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, or a symlink pointing to a
regular file or 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 inspect-internal dump-tree

stat(2) is used instead of lstat(2), as stat(2) follows symlinks just
like open(2) does, which means we check the same inode that open(2)
opens, and thus don't need special handling for symlinks.

Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
---

Changes from v1:
    * Updated commit message to use the new name for btrfs-debug-tree
    * Added a bit of clarity to the commit message to explain that stat(2)
      follows symlinks just like open(2) does, so we don't need special
      handling for symlinks..
    * Added the btrfs-progs developers to the recipient list, because
      I somehow forgot to add them on the first version.

This has been both build and runtime tested on an x86-64 system with
glibc.  It has been build but not runtime tested with uClibc on x86-64
and ARMv7.  It has not been tested on Android or with musl, although it
should work there also.

There are other tools that have similarly poor error 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.

Whitelisting is used instead of blacklisting because I feel it provides
a more concise and more easily readable conditional, and because I
think it's a lot less likely that a new file type will be added that
can contain a filesystem image than it is that one will be added which
can't contain a filesystem image (it's more likely that Solaris doors
get ported to Linux than that we get some new file type).

 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;
+	}
 
 	if (!(flags & OPEN_CTREE_WRITES))
 		oflags = O_RDONLY;
-- 
2.7.3

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

[David Sterba]

On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote:
> 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, or a symlink pointing to a
> regular file or 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 inspect-internal dump-tree
> 
> stat(2) is used instead of lstat(2), as stat(2) follows symlinks just
> like open(2) does, which means we check the same inode that open(2)
> opens, and thus don't need special handling for symlinks.
> 
> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>

Applied, thanks.

> This has been both build and runtime tested on an x86-64 system with
> glibc.  It has been build but not runtime tested with uClibc on x86-64
> and ARMv7.  It has not been tested on Android or with musl, although it
> should work there also.

I would not expect any difference among the other arches and libc, it's
using a common interface.

> There are other tools that have similarly poor error 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.

Yeah, it would be good to fix all.

> Whitelisting is used instead of blacklisting because I feel it provides
> a more concise and more easily readable conditional, and because I
> think it's a lot less likely that a new file type will be added that
> can contain a filesystem image than it is that one will be added which
> can't contain a filesystem image (it's more likely that Solaris doors
> get ported to Linux than that we get some new file type).

Agreed.

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

[Austin S. Hemmelgarn]

On 2016-03-18 11:17, David Sterba wrote:
> On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote:
>> This has been both build and runtime tested on an x86-64 system with
>> glibc.  It has been build but not runtime tested with uClibc on x86-64
>> and ARMv7.  It has not been tested on Android or with musl, although it
>> should work there also.
>
> I would not expect any difference among the other arches and libc, it's
> using a common interface.
The only one I would think might possibly be an issue is Android (bionic 
has some serious oddities in some places), but I don't think it's likely 
that they deviated from POSIX semantics here (I would check the 
documentation, but I don't have it readily available).  Mention of the 
arch was more for the sake of completeness in specifying what I tested 
than anything else.
>
>> There are other tools that have similarly poor error 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.
>
> Yeah, it would be good to fix all.
I should have time to take a closer look at btrfs rescue early next 
week, so I may have a patch for that relatively soon.  I can't readily 
think of anything else that might be doing similar things without 
calling open_ctree_fs_info though, so it may be a bit longer before I 
actually post a patch (ideally, I'd like to get all the other things 
that parse unmounted filesystems at the same time).

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

[Austin S. Hemmelgarn]

On 2016-03-18 11:17, David Sterba wrote:
> On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote:
>> There are other tools that have similarly poor error 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.
>
> Yeah, it would be good to fix all.
I had a bit of spare time over lunch, and decided to take a cursory 
look.  I hadn't realized that open_ctree uses open_ctree_fs_info, and it 
looks like almost everything that doesn't call open_ctree_fs_info calls 
open_ctree, which means this actually added error checking to a lot more 
than I realized, possibly everything that expects a filesystem image to 
operate on.  I've checked btrfs rescue now, and with this change, it now 
gives a nice error for something other than a a file or block device. 
It looks like a follow up patch may not be necessary after all, I'll try 
and double check the rest of the commands that I haven't checked already 
over the weekend, and hopefully have something by Monday or Tuesday if 
there's anything else that needs this check added.