[PATCH 0/4] export/fuse: Allow other users access to the export

[PATCH 0/4] export/fuse: Allow other users access to the export

[Max Reitz]

Hi,

With the default mount options, FUSE mounts are not accessible to any
users but the one who did the mount, not even to root.  To allow such
accesses, allow_other must be passed.

This is probably useful to some people (it certainly is to me, e.g. when
exporting some image as my normal user, and then trying to loop mount it
as root), so this series adds a QAPI allow-other bool that will make the
FUSE export code pass allow_other,default_permissions to FUSE.

(default_permissions will make the kernel do the usual UNIX permission
checks, which is something that makes a lot of sense when allowing other
users access to the export.)

This also requires our SETATTR code to be able to handle permission
changes, though, so the user can then run chmod/chown/chgrp on the
export to adjust its permissions to their need.

The final patch adds a test.


Max Reitz (4):
  export/fuse: Add allow-other option
  export/fuse: Give SET_ATTR_SIZE its own branch
  export/fuse: Let permissions be adjustable
  iotests/308: Test allow-other

 qapi/block-export.json     | 11 ++++-
 block/export/fuse.c        | 83 +++++++++++++++++++++++++---------
 tests/qemu-iotests/308     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/308.out | 47 ++++++++++++++++++++
 4 files changed, 210 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH 1/4] export/fuse: Add allow-other option

[Max Reitz]

Without the allow_other mount option, no user (not even root) but the
one who started qemu/the storage daemon can access the export.  Allow
users to configure the export such that such accesses are possible.

When we do pass allow_other, we should also pass default_permissions,
because our export code performs no permission checks.  With
default_permissions, we can just let the kernel do it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-export.json | 11 ++++++++++-
 block/export/fuse.c    | 17 +++++++++++++++--
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index e819e70cac..5d1cc04ac4 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -132,11 +132,20 @@
 # @growable: Whether writes beyond the EOF should grow the block node
 #            accordingly. (default: false)
 #
+# @allow-other: By default (if this is false), only qemu's user is allowed
+#               access to this export.  That cannot be changed even with
+#               chmod or chown.
+#               This option will allow other users access to the export with the
+#               FUSE mount options "allow_other,default_permissions".
+#               (default_permissions enables standard UNIX permission checks.)
+#               (since 6.1; default: false)
+#
 # Since: 6.0
 ##
 { 'struct': 'BlockExportOptionsFuse',
   'data': { 'mountpoint': 'str',
-            '*growable': 'bool' },
+            '*growable': 'bool',
+            '*allow-other': 'bool' },
   'if': 'defined(CONFIG_FUSE)' }
 
 ##
diff --git a/block/export/fuse.c b/block/export/fuse.c
index 38f74c94da..34a5836ece 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -46,6 +46,7 @@ typedef struct FuseExport {
     char *mountpoint;
     bool writable;
     bool growable;
+    bool allow_other;
 } FuseExport;
 
 static GHashTable *exports;
@@ -117,6 +118,7 @@ static int fuse_export_create(BlockExport *blk_exp,
     exp->mountpoint = g_strdup(args->mountpoint);
     exp->writable = blk_exp_args->writable;
     exp->growable = args->growable;
+    exp->allow_other = args->allow_other;
 
     ret = setup_fuse_export(exp, args->mountpoint, errp);
     if (ret < 0) {
@@ -150,11 +152,22 @@ static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
 {
     const char *fuse_argv[4];
     char *mount_opts;
+    const char *allow_other;
     struct fuse_args fuse_args;
     int ret;
 
-    /* Needs to match what fuse_init() sets.  Only max_read must be supplied. */
-    mount_opts = g_strdup_printf("max_read=%zu", FUSE_MAX_BOUNCE_BYTES);
+    if (exp->allow_other) {
+        allow_other = ",allow_other,default_permissions";
+    } else {
+        allow_other = "";
+    }
+
+    /*
+     * max_read needs to match what fuse_init() sets.
+     * max_write need not be supplied.
+     */
+    mount_opts = g_strdup_printf("max_read=%zu%s", FUSE_MAX_BOUNCE_BYTES,
+                                 allow_other);
 
     fuse_argv[0] = ""; /* Dummy program name */
     fuse_argv[1] = "-o";
-- 
2.31.1

[PATCH 2/4] export/fuse: Give SET_ATTR_SIZE its own branch

[Max Reitz]

In order to support changing other attributes than the file size in
fuse_setattr(), we have to give each its own independent branch.  This
also applies to the only attribute we do support right now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/export/fuse.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 34a5836ece..1d54286d90 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -408,20 +408,22 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
     FuseExport *exp = fuse_req_userdata(req);
     int ret;
 
-    if (!exp->writable) {
-        fuse_reply_err(req, EACCES);
-        return;
-    }
-
     if (to_set & ~FUSE_SET_ATTR_SIZE) {
         fuse_reply_err(req, ENOTSUP);
         return;
     }
 
-    ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
-    if (ret < 0) {
-        fuse_reply_err(req, -ret);
-        return;
+    if (to_set & FUSE_SET_ATTR_SIZE) {
+        if (!exp->writable) {
+            fuse_reply_err(req, EACCES);
+            return;
+        }
+
+        ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
+        if (ret < 0) {
+            fuse_reply_err(req, -ret);
+            return;
+        }
     }
 
     fuse_getattr(req, inode, fi);
-- 
2.31.1

[PATCH 3/4] export/fuse: Let permissions be adjustable

[Max Reitz]

Allow changing the file mode, UID, and GID through SETATTR.

This only really makes sense with allow-other, though (because without
it, the effective access mode is fixed to be 0600 (u+rw) with qemu's
user being the file's owner), so changing these stat fields is not
allowed without allow-other.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/export/fuse.c | 48 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 1d54286d90..742e0af657 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -47,6 +47,10 @@ typedef struct FuseExport {
     bool writable;
     bool growable;
     bool allow_other;
+
+    mode_t st_mode;
+    uid_t st_uid;
+    gid_t st_gid;
 } FuseExport;
 
 static GHashTable *exports;
@@ -120,6 +124,13 @@ static int fuse_export_create(BlockExport *blk_exp,
     exp->growable = args->growable;
     exp->allow_other = args->allow_other;
 
+    exp->st_mode = S_IFREG | S_IRUSR;
+    if (exp->writable) {
+        exp->st_mode |= S_IWUSR;
+    }
+    exp->st_uid = getuid();
+    exp->st_gid = getgid();
+
     ret = setup_fuse_export(exp, args->mountpoint, errp);
     if (ret < 0) {
         goto fail;
@@ -329,7 +340,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
     int64_t length, allocated_blocks;
     time_t now = time(NULL);
     FuseExport *exp = fuse_req_userdata(req);
-    mode_t mode;
 
     length = blk_getlength(exp->common.blk);
     if (length < 0) {
@@ -344,17 +354,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
         allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
     }
 
-    mode = S_IFREG | S_IRUSR;
-    if (exp->writable) {
-        mode |= S_IWUSR;
-    }
-
     statbuf = (struct stat) {
         .st_ino     = inode,
-        .st_mode    = mode,
+        .st_mode    = exp->st_mode,
         .st_nlink   = 1,
-        .st_uid     = getuid(),
-        .st_gid     = getgid(),
+        .st_uid     = exp->st_uid,
+        .st_gid     = exp->st_gid,
         .st_size    = length,
         .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
         .st_blocks  = allocated_blocks,
@@ -400,15 +405,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
 }
 
 /**
- * Let clients set file attributes.  Only resizing is supported.
+ * Let clients set file attributes.  With allow_other, only resizing and
+ * changing permissions (st_mode, st_uid, st_gid) is allowed.  Without
+ * allow_other, only resizing is supported.
  */
 static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
                          int to_set, struct fuse_file_info *fi)
 {
     FuseExport *exp = fuse_req_userdata(req);
+    int supported_attrs;
     int ret;
 
-    if (to_set & ~FUSE_SET_ATTR_SIZE) {
+    supported_attrs = FUSE_SET_ATTR_SIZE;
+    if (exp->allow_other) {
+        supported_attrs |= FUSE_SET_ATTR_MODE | FUSE_SET_ATTR_UID |
+            FUSE_SET_ATTR_GID;
+    }
+    if (to_set & ~supported_attrs) {
         fuse_reply_err(req, ENOTSUP);
         return;
     }
@@ -426,6 +439,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
         }
     }
 
+    if (to_set & FUSE_SET_ATTR_MODE) {
+        /* Only allow changing the file mode, not the type */
+        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
+    }
+
+    if (to_set & FUSE_SET_ATTR_UID) {
+        exp->st_uid = statbuf->st_uid;
+    }
+
+    if (to_set & FUSE_SET_ATTR_GID) {
+        exp->st_gid = statbuf->st_gid;
+    }
+
     fuse_getattr(req, inode, fi);
 }
 
-- 
2.31.1

[PATCH 4/4] iotests/308: Test allow-other

[Max Reitz]

We cannot reasonably test the main point of allow-other, which is to
allow users other than the current one to access the FUSE export,
because that would require access to sudo, which this test most likely
will not have.  (Also, we would need to figure out some user/group that
is on the machine and that is not the current user/group, which may
become a bit hairy.)

But we can test some byproducts: First, whether changing permissions
works (our FUSE code only allows so for allow-other=true), and second,
whether the kernel applies permission checks with allow-other=true
(because that implies default_permissions).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/308     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/308.out | 47 ++++++++++++++++++++
 2 files changed, 138 insertions(+)

diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index f122065d0f..1b2f908947 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -334,6 +334,97 @@ echo '=== Compare copy with original ==='
 
 $QEMU_IMG compare -f raw -F $IMGFMT "$COPIED_IMG" "$TEST_IMG"
 
+echo
+echo '=== Test permissions ==='
+
+# Test that you can only change permissions on the export with allow-other=true.
+# We cannot really test the primary reason behind allow-other (i.e. to allow
+# users other than the current one access to the export), because for that we
+# would need sudo, which realistically nobody will allow this test to use.
+# What we can do is test that allow-other=true also enables default_permissions,
+# i.e. whether we can still read from the file if we remove the read permission.
+
+# $1: allow-other value ('true' or 'false')
+run_permission_test()
+{
+    # Below, we want to test permission checks on the export.  To do that
+    # properly, we need to ensure that those checks are not bypassed, so we have
+    # to drop cap_dac_override.
+    # (Note that cap_dac_read_search bypasses read permission checks, which is
+    # why we try to open the file R/W below, so we do not have to care about
+    # cap_dac_read_search here.)
+    # $capsh is effectively a shell command, i.e. can be used like:
+    #   $capsh -c "$command $args..."
+    # By default it is just bash.
+    capsh='/usr/bin/env bash'
+    if type -p capsh > /dev/null; then
+        if ! capsh --has-p=cap_dac_override 2>/dev/null; then
+            # No cap_dac_override, we are good to go
+            true # pass
+        elif capsh --has-p=cap_setpcap 2>/dev/null; then
+            # We will need to drop cap_dac_override, but doing so requires
+            # cap_setpcap in turn
+            capsh='capsh --drop=cap_dac_override --'
+        else
+            # Hopefully a rare case
+            _notrun 'CAP_DAC_OVERRIDE must be dropped, but this cannot be' \
+                    'done without CAP_SETPCAP'
+        fi
+    elif [ "$(id -u)" -ne 0 ]; then
+        # Non-root users probably do not have those capabilities, so try to get
+        # by without capsh
+        true # pass
+    else
+        # Running this test as root without capsh on the system should be a rare
+        # case...
+        _notrun 'No capsh found while run as root'
+    fi
+
+    _launch_qemu \
+        -blockdev \
+        "$IMGFMT,node-name=node-format,file.driver=file,file.filename=$TEST_IMG"
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'qmp_capabilities'}" \
+        'return'
+
+    # Writable so we can open it R/W below
+    fuse_export_add 'export' \
+        "'mountpoint': '$EXT_MP',
+         'writable': true,
+         'allow-other': $1"
+
+    echo '(Invoking chmod)'
+    chmod 666 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+    stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+    echo '(Removing all permissions)'
+    chmod 000 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+
+    # We want a permission denied here (with allow-other=true)
+    # (Use $QEMU_IO_PROG, because $capsh will not know the wrapper that is
+    # $QEMU_IO)
+    # Use blkdebug to force-take the WRITE permission so this will definitely
+    # try to open the export with O_RDWR.
+    imgopts="driver=blkdebug,take-child-perms.0=write,"
+    imgopts+="image.driver=file,image.filename=$EXT_MP"
+    $capsh -c "$QEMU_IO_PROG -c quit --image-opts '$imgopts'" 2>&1 \
+        | _filter_qemu_io | _filter_testdir | _filter_imgfmt
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute': 'quit'}" \
+        'return'
+
+    wait=yes _cleanup_qemu
+}
+
+for ao in 'false' 'true'; do
+    echo
+    echo "--- allow-other=$ao ---"
+
+    run_permission_test "$ao"
+done
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index 466e7e0267..4620efbb0d 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -167,4 +167,51 @@ OK: Post-truncate image size is as expected
 
 === Compare copy with original ===
 Images are identical.
+
+=== Test permissions ===
+
+--- allow-other=false ---
+{'execute': 'qmp_capabilities'}
+{"return": {}}
+{'execute': 'block-export-add',
+          'arguments': {
+              'type': 'fuse',
+              'id': 'export',
+              'node-name': 'node-format',
+              'mountpoint': 'TEST_DIR/t.IMGFMT.fuse',
+         'writable': true,
+         'allow-other': false
+          } }
+{"return": {}}
+(Invoking chmod)
+chmod: changing permissions of 'TEST_DIR/t.IMGFMT.fuse': Operation not supported
+Permissions post-chmod: 600
+(Removing all permissions)
+chmod: changing permissions of 'TEST_DIR/t.IMGFMT.fuse': Operation not supported
+{'execute': 'quit'}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export"}}
+
+--- allow-other=true ---
+{'execute': 'qmp_capabilities'}
+{"return": {}}
+{'execute': 'block-export-add',
+          'arguments': {
+              'type': 'fuse',
+              'id': 'export',
+              'node-name': 'node-format',
+              'mountpoint': 'TEST_DIR/t.IMGFMT.fuse',
+         'writable': true,
+         'allow-other': true
+          } }
+{"return": {}}
+(Invoking chmod)
+Permissions post-chmod: 666
+(Removing all permissions)
+qemu-io: can't open: Could not open 'TEST_DIR/t.IMGFMT.fuse': Permission denied
+{'execute': 'quit'}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export"}}
 *** done
-- 
2.31.1

Re: [PATCH 0/4] export/fuse: Allow other users access to the export

[Kevin Wolf]

Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> Hi,
> 
> With the default mount options, FUSE mounts are not accessible to any
> users but the one who did the mount, not even to root.  To allow such
> accesses, allow_other must be passed.
> 
> This is probably useful to some people (it certainly is to me, e.g. when
> exporting some image as my normal user, and then trying to loop mount it
> as root), so this series adds a QAPI allow-other bool that will make the
> FUSE export code pass allow_other,default_permissions to FUSE.
> 
> (default_permissions will make the kernel do the usual UNIX permission
> checks, which is something that makes a lot of sense when allowing other
> users access to the export.)
> 
> This also requires our SETATTR code to be able to handle permission
> changes, though, so the user can then run chmod/chown/chgrp on the
> export to adjust its permissions to their need.
> 
> The final patch adds a test.

If there is even a use case for leaving the option off (not trusting
root?), it must certainly be the less common case? So I'm not sure if
allow-other should be an option at all, but if it is, enabling it by
default would make more sense to me.

Is there a reason why you picked false as the default, except that it is
the old behaviour?

Kevin

Re: [PATCH 0/4] export/fuse: Allow other users access to the export

[Max Reitz]

On 21.06.21 18:12, Kevin Wolf wrote:
> Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
>> Hi,
>>
>> With the default mount options, FUSE mounts are not accessible to any
>> users but the one who did the mount, not even to root.  To allow such
>> accesses, allow_other must be passed.
>>
>> This is probably useful to some people (it certainly is to me, e.g. when
>> exporting some image as my normal user, and then trying to loop mount it
>> as root), so this series adds a QAPI allow-other bool that will make the
>> FUSE export code pass allow_other,default_permissions to FUSE.
>>
>> (default_permissions will make the kernel do the usual UNIX permission
>> checks, which is something that makes a lot of sense when allowing other
>> users access to the export.)
>>
>> This also requires our SETATTR code to be able to handle permission
>> changes, though, so the user can then run chmod/chown/chgrp on the
>> export to adjust its permissions to their need.
>>
>> The final patch adds a test.
> If there is even a use case for leaving the option off (not trusting
> root?), it must certainly be the less common case? So I'm not sure if
> allow-other should be an option at all, but if it is, enabling it by
> default would make more sense to me.
>
> Is there a reason why you picked false as the default, except that it is
> the old behaviour?

No. :)

Well, mostly.  I also thought, if FUSE thinks allow_other shouldn’t be 
the default, who am I to decide otherwise.

Now that I tried to find out why FUSE has it as the default (I only 
remember vague “security reasons”), I still couldn’t find out why, but I 
did find that using this option as non-root user requires /etc/fuse.conf 
to have user_allow_other in it, which I don’t think we can require.

So I think it must be an option.  As for which value should be the 
default, that probably depends on how common having user_allow_other in 
/etc/fuse.conf is.  I know I never put it there, and it’s both on my 
Fedora and my Arch system.  So I guess it seems rather common?

Max

Re: [PATCH 1/4] export/fuse: Add allow-other option

[Kevin Wolf]

Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> Without the allow_other mount option, no user (not even root) but the
> one who started qemu/the storage daemon can access the export.  Allow
> users to configure the export such that such accesses are possible.
> 
> When we do pass allow_other, we should also pass default_permissions,
> because our export code performs no permission checks.  With
> default_permissions, we can just let the kernel do it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 2/4] export/fuse: Give SET_ATTR_SIZE its own branch

[Kevin Wolf]

Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> In order to support changing other attributes than the file size in
> fuse_setattr(), we have to give each its own independent branch.  This
> also applies to the only attribute we do support right now.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 3/4] export/fuse: Let permissions be adjustable

[Kevin Wolf]

Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> Allow changing the file mode, UID, and GID through SETATTR.
> 
> This only really makes sense with allow-other, though (because without
> it, the effective access mode is fixed to be 0600 (u+rw) with qemu's
> user being the file's owner), so changing these stat fields is not
> allowed without allow-other.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/export/fuse.c | 48 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index 1d54286d90..742e0af657 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -47,6 +47,10 @@ typedef struct FuseExport {
>      bool writable;
>      bool growable;
>      bool allow_other;
> +
> +    mode_t st_mode;
> +    uid_t st_uid;
> +    gid_t st_gid;
>  } FuseExport;
>  
>  static GHashTable *exports;
> @@ -120,6 +124,13 @@ static int fuse_export_create(BlockExport *blk_exp,
>      exp->growable = args->growable;
>      exp->allow_other = args->allow_other;
>  
> +    exp->st_mode = S_IFREG | S_IRUSR;
> +    if (exp->writable) {
> +        exp->st_mode |= S_IWUSR;
> +    }
> +    exp->st_uid = getuid();
> +    exp->st_gid = getgid();
> +
>      ret = setup_fuse_export(exp, args->mountpoint, errp);
>      if (ret < 0) {
>          goto fail;
> @@ -329,7 +340,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
>      int64_t length, allocated_blocks;
>      time_t now = time(NULL);
>      FuseExport *exp = fuse_req_userdata(req);
> -    mode_t mode;
>  
>      length = blk_getlength(exp->common.blk);
>      if (length < 0) {
> @@ -344,17 +354,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
>          allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
>      }
>  
> -    mode = S_IFREG | S_IRUSR;
> -    if (exp->writable) {
> -        mode |= S_IWUSR;
> -    }
> -
>      statbuf = (struct stat) {
>          .st_ino     = inode,
> -        .st_mode    = mode,
> +        .st_mode    = exp->st_mode,
>          .st_nlink   = 1,
> -        .st_uid     = getuid(),
> -        .st_gid     = getgid(),
> +        .st_uid     = exp->st_uid,
> +        .st_gid     = exp->st_gid,
>          .st_size    = length,
>          .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
>          .st_blocks  = allocated_blocks,
> @@ -400,15 +405,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>  }
>  
>  /**
> - * Let clients set file attributes.  Only resizing is supported.
> + * Let clients set file attributes.  With allow_other, only resizing and
> + * changing permissions (st_mode, st_uid, st_gid) is allowed.  Without
> + * allow_other, only resizing is supported.
>   */
>  static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
>                           int to_set, struct fuse_file_info *fi)
>  {
>      FuseExport *exp = fuse_req_userdata(req);
> +    int supported_attrs;
>      int ret;
>  
> -    if (to_set & ~FUSE_SET_ATTR_SIZE) {
> +    supported_attrs = FUSE_SET_ATTR_SIZE;
> +    if (exp->allow_other) {
> +        supported_attrs |= FUSE_SET_ATTR_MODE | FUSE_SET_ATTR_UID |
> +            FUSE_SET_ATTR_GID;
> +    }
> +    if (to_set & ~supported_attrs) {
>          fuse_reply_err(req, ENOTSUP);
>          return;
>      }
> @@ -426,6 +439,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
>          }
>      }
>  
> +    if (to_set & FUSE_SET_ATTR_MODE) {
> +        /* Only allow changing the file mode, not the type */
> +        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
> +    }

Should we check that the mode actually makes sense? Not sure if making
an image executable has a good use case, and making it writable in the
permissions for a read-only export isn't a good idea either.

> +
> +    if (to_set & FUSE_SET_ATTR_UID) {
> +        exp->st_uid = statbuf->st_uid;
> +    }
> +
> +    if (to_set & FUSE_SET_ATTR_GID) {
> +        exp->st_gid = statbuf->st_gid;
> +    }
> +
>      fuse_getattr(req, inode, fi);
>  }

Kevin

Re: [PATCH 4/4] iotests/308: Test allow-other

[Kevin Wolf]

Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> We cannot reasonably test the main point of allow-other, which is to
> allow users other than the current one to access the FUSE export,
> because that would require access to sudo, which this test most likely
> will not have.  (Also, we would need to figure out some user/group that
> is on the machine and that is not the current user/group, which may
> become a bit hairy.)
> 
> But we can test some byproducts: First, whether changing permissions
> works (our FUSE code only allows so for allow-other=true), and second,
> whether the kernel applies permission checks with allow-other=true
> (because that implies default_permissions).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

This seems to have the problem that you mentioned:

--- /home/kwolf/source/qemu/tests/qemu-iotests/308.out
+++ 308.out.bad
@@ -205,7 +205,9 @@
          'writable': true,
          'allow-other': true
           } }
-{"return": {}}
+fusermount3: option allow_other only allowed if 'user_allow_other' is set in /etc/fuse.conf
+{"error": {"class": "GenericError", "desc": "Failed to mount FUSE session to export"}}
+Timeout waiting for return on handle 2
 (Invoking chmod)
 Permissions post-chmod: 666
 (Removing all permissions)

Maybe it should be a separate test case that is skipped with
user_allow_other is disabled.

>  tests/qemu-iotests/308     | 91 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/308.out | 47 ++++++++++++++++++++
>  2 files changed, 138 insertions(+)
> 
> diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
> index f122065d0f..1b2f908947 100755
> --- a/tests/qemu-iotests/308
> +++ b/tests/qemu-iotests/308
> @@ -334,6 +334,97 @@ echo '=== Compare copy with original ==='
>  
>  $QEMU_IMG compare -f raw -F $IMGFMT "$COPIED_IMG" "$TEST_IMG"
>  
> +echo
> +echo '=== Test permissions ==='
> +
> +# Test that you can only change permissions on the export with allow-other=true.
> +# We cannot really test the primary reason behind allow-other (i.e. to allow
> +# users other than the current one access to the export), because for that we
> +# would need sudo, which realistically nobody will allow this test to use.
> +# What we can do is test that allow-other=true also enables default_permissions,
> +# i.e. whether we can still read from the file if we remove the read permission.

We already have other test cases that use sudo if available. Though I
guess it means that these tests aren't run very often.

Kevin

Re: [PATCH 3/4] export/fuse: Let permissions be adjustable

[Max Reitz]

On 22.06.21 17:02, Kevin Wolf wrote:
> Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
>> Allow changing the file mode, UID, and GID through SETATTR.
>>
>> This only really makes sense with allow-other, though (because without
>> it, the effective access mode is fixed to be 0600 (u+rw) with qemu's
>> user being the file's owner), so changing these stat fields is not
>> allowed without allow-other.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/export/fuse.c | 48 ++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/export/fuse.c b/block/export/fuse.c
>> index 1d54286d90..742e0af657 100644
>> --- a/block/export/fuse.c
>> +++ b/block/export/fuse.c
>> @@ -47,6 +47,10 @@ typedef struct FuseExport {
>>       bool writable;
>>       bool growable;
>>       bool allow_other;
>> +
>> +    mode_t st_mode;
>> +    uid_t st_uid;
>> +    gid_t st_gid;
>>   } FuseExport;
>>   
>>   static GHashTable *exports;
>> @@ -120,6 +124,13 @@ static int fuse_export_create(BlockExport *blk_exp,
>>       exp->growable = args->growable;
>>       exp->allow_other = args->allow_other;
>>   
>> +    exp->st_mode = S_IFREG | S_IRUSR;
>> +    if (exp->writable) {
>> +        exp->st_mode |= S_IWUSR;
>> +    }
>> +    exp->st_uid = getuid();
>> +    exp->st_gid = getgid();
>> +
>>       ret = setup_fuse_export(exp, args->mountpoint, errp);
>>       if (ret < 0) {
>>           goto fail;
>> @@ -329,7 +340,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
>>       int64_t length, allocated_blocks;
>>       time_t now = time(NULL);
>>       FuseExport *exp = fuse_req_userdata(req);
>> -    mode_t mode;
>>   
>>       length = blk_getlength(exp->common.blk);
>>       if (length < 0) {
>> @@ -344,17 +354,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
>>           allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
>>       }
>>   
>> -    mode = S_IFREG | S_IRUSR;
>> -    if (exp->writable) {
>> -        mode |= S_IWUSR;
>> -    }
>> -
>>       statbuf = (struct stat) {
>>           .st_ino     = inode,
>> -        .st_mode    = mode,
>> +        .st_mode    = exp->st_mode,
>>           .st_nlink   = 1,
>> -        .st_uid     = getuid(),
>> -        .st_gid     = getgid(),
>> +        .st_uid     = exp->st_uid,
>> +        .st_gid     = exp->st_gid,
>>           .st_size    = length,
>>           .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
>>           .st_blocks  = allocated_blocks,
>> @@ -400,15 +405,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>>   }
>>   
>>   /**
>> - * Let clients set file attributes.  Only resizing is supported.
>> + * Let clients set file attributes.  With allow_other, only resizing and
>> + * changing permissions (st_mode, st_uid, st_gid) is allowed.  Without
>> + * allow_other, only resizing is supported.
>>    */
>>   static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
>>                            int to_set, struct fuse_file_info *fi)
>>   {
>>       FuseExport *exp = fuse_req_userdata(req);
>> +    int supported_attrs;
>>       int ret;
>>   
>> -    if (to_set & ~FUSE_SET_ATTR_SIZE) {
>> +    supported_attrs = FUSE_SET_ATTR_SIZE;
>> +    if (exp->allow_other) {
>> +        supported_attrs |= FUSE_SET_ATTR_MODE | FUSE_SET_ATTR_UID |
>> +            FUSE_SET_ATTR_GID;
>> +    }
>> +    if (to_set & ~supported_attrs) {
>>           fuse_reply_err(req, ENOTSUP);
>>           return;
>>       }
>> @@ -426,6 +439,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
>>           }
>>       }
>>   
>> +    if (to_set & FUSE_SET_ATTR_MODE) {
>> +        /* Only allow changing the file mode, not the type */
>> +        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
>> +    }
> Should we check that the mode actually makes sense? Not sure if making
> an image executable has a good use case, and making it writable in the
> permissions for a read-only export isn't a good idea either.

I mean, I don’t mind what the user does.  It doesn’t really faze us, I 
believe.  If the image contains an executable ELF and the user wants to 
run it directly from FUSE...  I don’t mind.

As for +w on RO exports, I’m not sure.  This reminds me of `sudo chattr 
+i $file`, which effectively makes any regular file read-only, too, and 
it can still keep +w.  So the file permissions are basically just ACLs, 
getting permission for something doesn’t mean you can actually do it.

OTOH, the difference to `chattr +i` is that we’d allow opening the 
export R/W, only writing would then fail.  `chattr +i` does give EPERM 
when opening the file.

So I’m not quite sure.  I don’t really want to prevent the user from 
setting any access restrictions they want, but on the other hand, if 
writing can never work, then there really is no point in allowing +w.  
(Then I’m wondering, if we don’t allow +w, should we silently drop it or 
return an error?  I guess returning success but not actually succeeding 
is weird, so we probably should return EROFS.)

But +x can technically work, so I wouldn’t disallow it.

Max

Re: [PATCH 4/4] iotests/308: Test allow-other

[Max Reitz]

On 22.06.21 17:08, Kevin Wolf wrote:
> Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
>> We cannot reasonably test the main point of allow-other, which is to
>> allow users other than the current one to access the FUSE export,
>> because that would require access to sudo, which this test most likely
>> will not have.  (Also, we would need to figure out some user/group that
>> is on the machine and that is not the current user/group, which may
>> become a bit hairy.)
>>
>> But we can test some byproducts: First, whether changing permissions
>> works (our FUSE code only allows so for allow-other=true), and second,
>> whether the kernel applies permission checks with allow-other=true
>> (because that implies default_permissions).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> This seems to have the problem that you mentioned:
>
> --- /home/kwolf/source/qemu/tests/qemu-iotests/308.out
> +++ 308.out.bad
> @@ -205,7 +205,9 @@
>            'writable': true,
>            'allow-other': true
>             } }
> -{"return": {}}
> +fusermount3: option allow_other only allowed if 'user_allow_other' is set in /etc/fuse.conf
> +{"error": {"class": "GenericError", "desc": "Failed to mount FUSE session to export"}}
> +Timeout waiting for return on handle 2
>   (Invoking chmod)
>   Permissions post-chmod: 666
>   (Removing all permissions)
>
> Maybe it should be a separate test case that is skipped with
> user_allow_other is disabled.

Right.

>>   tests/qemu-iotests/308     | 91 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/308.out | 47 ++++++++++++++++++++
>>   2 files changed, 138 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
>> index f122065d0f..1b2f908947 100755
>> --- a/tests/qemu-iotests/308
>> +++ b/tests/qemu-iotests/308
>> @@ -334,6 +334,97 @@ echo '=== Compare copy with original ==='
>>   
>>   $QEMU_IMG compare -f raw -F $IMGFMT "$COPIED_IMG" "$TEST_IMG"
>>   
>> +echo
>> +echo '=== Test permissions ==='
>> +
>> +# Test that you can only change permissions on the export with allow-other=true.
>> +# We cannot really test the primary reason behind allow-other (i.e. to allow
>> +# users other than the current one access to the export), because for that we
>> +# would need sudo, which realistically nobody will allow this test to use.
>> +# What we can do is test that allow-other=true also enables default_permissions,
>> +# i.e. whether we can still read from the file if we remove the read permission.
> We already have other test cases that use sudo if available. Though I
> guess it means that these tests aren't run very often.

Yes, I know, but honestly I don’t really want to deal with user 
management either.  I had a paragraph about that in a preliminary 
version but decided to cut it, because, I thought it wouldn’t really matter.

That problem is that I’d need to run qemu-io as some different user, and 
the question is, who is a different user?  I suppose I could rely on 
“root” and “nobody” being valid users on any system, but I don’t think I 
can be sure that the user running the tests isn’t either of those.  So I 
would have to check whether the current user is “root”, and then run it 
as “nobody”, or otherwise run it as “root”, but that just seems like I’m 
getting in too deep for something that isn’t really useful anyway, 
because on developers’ machines, it will most likely be skipped anyway.

Max

Re: [PATCH 3/4] export/fuse: Let permissions be adjustable

[Kevin Wolf]

Am 22.06.2021 um 17:22 hat Max Reitz geschrieben:
> On 22.06.21 17:02, Kevin Wolf wrote:
> > Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> > > Allow changing the file mode, UID, and GID through SETATTR.
> > > 
> > > This only really makes sense with allow-other, though (because without
> > > it, the effective access mode is fixed to be 0600 (u+rw) with qemu's
> > > user being the file's owner), so changing these stat fields is not
> > > allowed without allow-other.
> > > 
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >   block/export/fuse.c | 48 ++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 37 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/block/export/fuse.c b/block/export/fuse.c
> > > index 1d54286d90..742e0af657 100644
> > > --- a/block/export/fuse.c
> > > +++ b/block/export/fuse.c
> > > @@ -47,6 +47,10 @@ typedef struct FuseExport {
> > >       bool writable;
> > >       bool growable;
> > >       bool allow_other;
> > > +
> > > +    mode_t st_mode;
> > > +    uid_t st_uid;
> > > +    gid_t st_gid;
> > >   } FuseExport;
> > >   static GHashTable *exports;
> > > @@ -120,6 +124,13 @@ static int fuse_export_create(BlockExport *blk_exp,
> > >       exp->growable = args->growable;
> > >       exp->allow_other = args->allow_other;
> > > +    exp->st_mode = S_IFREG | S_IRUSR;
> > > +    if (exp->writable) {
> > > +        exp->st_mode |= S_IWUSR;
> > > +    }
> > > +    exp->st_uid = getuid();
> > > +    exp->st_gid = getgid();
> > > +
> > >       ret = setup_fuse_export(exp, args->mountpoint, errp);
> > >       if (ret < 0) {
> > >           goto fail;
> > > @@ -329,7 +340,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
> > >       int64_t length, allocated_blocks;
> > >       time_t now = time(NULL);
> > >       FuseExport *exp = fuse_req_userdata(req);
> > > -    mode_t mode;
> > >       length = blk_getlength(exp->common.blk);
> > >       if (length < 0) {
> > > @@ -344,17 +354,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
> > >           allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
> > >       }
> > > -    mode = S_IFREG | S_IRUSR;
> > > -    if (exp->writable) {
> > > -        mode |= S_IWUSR;
> > > -    }
> > > -
> > >       statbuf = (struct stat) {
> > >           .st_ino     = inode,
> > > -        .st_mode    = mode,
> > > +        .st_mode    = exp->st_mode,
> > >           .st_nlink   = 1,
> > > -        .st_uid     = getuid(),
> > > -        .st_gid     = getgid(),
> > > +        .st_uid     = exp->st_uid,
> > > +        .st_gid     = exp->st_gid,
> > >           .st_size    = length,
> > >           .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
> > >           .st_blocks  = allocated_blocks,
> > > @@ -400,15 +405,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
> > >   }
> > >   /**
> > > - * Let clients set file attributes.  Only resizing is supported.
> > > + * Let clients set file attributes.  With allow_other, only resizing and
> > > + * changing permissions (st_mode, st_uid, st_gid) is allowed.  Without
> > > + * allow_other, only resizing is supported.
> > >    */
> > >   static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
> > >                            int to_set, struct fuse_file_info *fi)
> > >   {
> > >       FuseExport *exp = fuse_req_userdata(req);
> > > +    int supported_attrs;
> > >       int ret;
> > > -    if (to_set & ~FUSE_SET_ATTR_SIZE) {
> > > +    supported_attrs = FUSE_SET_ATTR_SIZE;
> > > +    if (exp->allow_other) {
> > > +        supported_attrs |= FUSE_SET_ATTR_MODE | FUSE_SET_ATTR_UID |
> > > +            FUSE_SET_ATTR_GID;
> > > +    }
> > > +    if (to_set & ~supported_attrs) {
> > >           fuse_reply_err(req, ENOTSUP);
> > >           return;
> > >       }
> > > @@ -426,6 +439,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
> > >           }
> > >       }
> > > +    if (to_set & FUSE_SET_ATTR_MODE) {
> > > +        /* Only allow changing the file mode, not the type */
> > > +        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
> > > +    }
> > Should we check that the mode actually makes sense? Not sure if making
> > an image executable has a good use case, and making it writable in the
> > permissions for a read-only export isn't a good idea either.
> 
> I mean, I don’t mind what the user does.  It doesn’t really faze us, I
> believe.  If the image contains an executable ELF and the user wants to run
> it directly from FUSE...  I don’t mind.
> 
> As for +w on RO exports, I’m not sure.  This reminds me of `sudo chattr +i
> $file`, which effectively makes any regular file read-only, too, and it can
> still keep +w.  So the file permissions are basically just ACLs, getting
> permission for something doesn’t mean you can actually do it.
> 
> OTOH, the difference to `chattr +i` is that we’d allow opening the export
> R/W, only writing would then fail.  `chattr +i` does give EPERM when opening
> the file.
> 
> So I’m not quite sure.  I don’t really want to prevent the user from setting
> any access restrictions they want, but on the other hand, if writing can
> never work, then there really is no point in allowing +w.  (Then I’m
> wondering, if we don’t allow +w, should we silently drop it or return an
> error?  I guess returning success but not actually succeeding is weird, so
> we probably should return EROFS.)

Yes, EROFS seems best.

> But +x can technically work, so I wouldn’t disallow it.

Fair enough.

Kevin