[Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Michael Tokarev]

Apparently the the mapped-* security models results in a raw bytes
being dumped to host without any architecture normalization (in
host byte order).  This may even lead to security issues in guest
when the same files are served from another host for example.

This bug has been initially submitted against debian qemu package, see
http://bugs.debian.org/755740

Thanks,

/mjt

Re: [Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Aneesh Kumar K.V]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Apparently the the mapped-* security models results in a raw bytes
> being dumped to host without any architecture normalization (in
> host byte order).  This may even lead to security issues in guest
> when the same files are served from another host for example.
>
> This bug has been initially submitted against debian qemu package, see
> http://bugs.debian.org/755740
>

Thanks for reporting the bug. Yes we do have issue with
mapped-xattr. But mapped-file should be ok. We record the uid/gid as
string in the file.

-aneesh

Re: [Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Aneesh Kumar K.V]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> Michael Tokarev <mjt@tls.msk.ru> writes:
>
>> Apparently the the mapped-* security models results in a raw bytes
>> being dumped to host without any architecture normalization (in
>> host byte order).  This may even lead to security issues in guest
>> when the same files are served from another host for example.
>>
>> This bug has been initially submitted against debian qemu package, see
>> http://bugs.debian.org/755740
>>
>
> Thanks for reporting the bug. Yes we do have issue with
> mapped-xattr. But mapped-file should be ok. We record the uid/gid as
> string in the file.

What would be the best way to fix this in a backward compatible way ?
Considering most of the users will be little endian host, we could do "always
store in little endian format" which of-course will break big-endian
hosts. We could possibly ask them to update xattr using external tools ?

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 3b0b6a9b1d7d..cd662410420e 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -135,17 +135,17 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
         mode_t tmp_mode;
         dev_t tmp_dev;
         if (getxattr(buffer, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
-            stbuf->st_uid = tmp_uid;
+            stbuf->st_uid = le32_to_cpu(tmp_uid);
         }
         if (getxattr(buffer, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
-            stbuf->st_gid = tmp_gid;
+            stbuf->st_gid = le32_to_cpu(tmp_gid);
         }
         if (getxattr(buffer, "user.virtfs.mode",
                     &tmp_mode, sizeof(mode_t)) > 0) {
-            stbuf->st_mode = tmp_mode;
+            stbuf->st_mode = le32_to_cpu(tmp_mode);
         }
         if (getxattr(buffer, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
-                stbuf->st_rdev = tmp_dev;
+            stbuf->st_rdev = le64_to_cpu(tmp_dev);
         }
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         local_mapped_file_attr(fs_ctx, path, stbuf);
@@ -255,29 +255,29 @@ static int local_set_xattr(const char *path, FsCred *credp)
     int err;
 
     if (credp->fc_uid != -1) {
-        err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t),
-                0);
+        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
+        err = setxattr(path, "user.virtfs.uid", &tmp_uid, sizeof(uid_t), 0);
         if (err) {
             return err;
         }
     }
     if (credp->fc_gid != -1) {
-        err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t),
-                0);
+        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
+        err = setxattr(path, "user.virtfs.gid", &tmp_gid, sizeof(gid_t), 0);
         if (err) {
             return err;
         }
     }
     if (credp->fc_mode != -1) {
-        err = setxattr(path, "user.virtfs.mode", &credp->fc_mode,
-                sizeof(mode_t), 0);
+        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
+        err = setxattr(path, "user.virtfs.mode", &tmp_mode, sizeof(mode_t), 0);
         if (err) {
             return err;
         }
     }
     if (credp->fc_rdev != -1) {
-        err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev,
-                sizeof(dev_t), 0);
+        uint64_t tmp_rdev = cpu_to_le32(credp->fc_rdev);
+        err = setxattr(path, "user.virtfs.rdev", &tmp_rdev, sizeof(dev_t), 0);
         if (err) {
             return err;
         }
@@ -630,21 +630,17 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
         mode_t tmp_mode;
         dev_t tmp_dev;
 
-        if (fgetxattr(fd, "user.virtfs.uid",
-                      &tmp_uid, sizeof(uid_t)) > 0) {
-            stbuf->st_uid = tmp_uid;
+        if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
+            stbuf->st_uid = le32_to_cpu(tmp_uid);
         }
-        if (fgetxattr(fd, "user.virtfs.gid",
-                      &tmp_gid, sizeof(gid_t)) > 0) {
-            stbuf->st_gid = tmp_gid;
+        if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
+            stbuf->st_gid = le32_to_cpu(tmp_gid);
         }
-        if (fgetxattr(fd, "user.virtfs.mode",
-                      &tmp_mode, sizeof(mode_t)) > 0) {
-            stbuf->st_mode = tmp_mode;
+        if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) {
+            stbuf->st_mode = le32_to_cpu(tmp_mode);
         }
-        if (fgetxattr(fd, "user.virtfs.rdev",
-                      &tmp_dev, sizeof(dev_t)) > 0) {
-                stbuf->st_rdev = tmp_dev;
+        if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
+            stbuf->st_rdev = le64_to_cpu(tmp_dev);
         }
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         errno = EOPNOTSUPP;

Re: [Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Michael Tokarev]

I haven't noticed this email - which is almost a month old now - until today.
So replying now...

30.07.2014 21:43, Aneesh Kumar K.V wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> 
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>
>>> Apparently the the mapped-* security models results in a raw bytes
>>> being dumped to host without any architecture normalization (in
>>> host byte order).  This may even lead to security issues in guest
>>> when the same files are served from another host for example.
>>>
>>> This bug has been initially submitted against debian qemu package, see
>>> http://bugs.debian.org/755740
>>>
>>
>> Thanks for reporting the bug. Yes we do have issue with
>> mapped-xattr. But mapped-file should be ok. We record the uid/gid as
>> string in the file.
> 
> What would be the best way to fix this in a backward compatible way ?
> Considering most of the users will be little endian host, we could do "always
> store in little endian format" which of-course will break big-endian
> hosts. We could possibly ask them to update xattr using external tools ?

If there's no way to _detect_ the used format (maybe doing some guessing, --
if that's possible to do in a reliable way, it should be good), that's
one of 2 possible options as I see it: that or introduce a new format
entirely, maybe with another attribute name.

It might not be even required to use an external tool for conversion.
Again, if qemu is able to detect "wrong" endiannes, it might just
update things itself, or print a warning and switch to an old format,
or something like that.

But the guessing idea might not be as bad really.  I haven't looked
closely which information is stored in there, -- but it is possible
that some fields should have zeros in some bytes for example, and
if these aren't zero but becomes zeros after endianness conversion
that might be a good indicator.

I'm not sure the runtime code should be able to work with both formats
at the same time.  Actually, I'm not sure this is a big issue to
start with -- indeed, you said it already, majority of users of 9pfs
should be little endian hosts, -- are there any big endian hosts
using this, at all? :)  How about trying to detect (preferrable at
init time) and refusing to start if old/wrong format is detected.

Maybe have a compile-time define to use native or little endian
format is a good idea too.

Bastian, since you discovered this issue, you might be using
a host with "uncommon" endianness, what do you think?

Thanks,

/mjt

> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
> index 3b0b6a9b1d7d..cd662410420e 100644
> --- a/hw/9pfs/virtio-9p-local.c
> +++ b/hw/9pfs/virtio-9p-local.c
> @@ -135,17 +135,17 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
>          mode_t tmp_mode;
>          dev_t tmp_dev;
>          if (getxattr(buffer, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
> -            stbuf->st_uid = tmp_uid;
> +            stbuf->st_uid = le32_to_cpu(tmp_uid);
>          }
>          if (getxattr(buffer, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
> -            stbuf->st_gid = tmp_gid;
> +            stbuf->st_gid = le32_to_cpu(tmp_gid);
>          }
>          if (getxattr(buffer, "user.virtfs.mode",
>                      &tmp_mode, sizeof(mode_t)) > 0) {
> -            stbuf->st_mode = tmp_mode;
> +            stbuf->st_mode = le32_to_cpu(tmp_mode);
>          }
>          if (getxattr(buffer, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
> -                stbuf->st_rdev = tmp_dev;
> +            stbuf->st_rdev = le64_to_cpu(tmp_dev);
>          }
>      } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>          local_mapped_file_attr(fs_ctx, path, stbuf);
> @@ -255,29 +255,29 @@ static int local_set_xattr(const char *path, FsCred *credp)
>      int err;
>  
>      if (credp->fc_uid != -1) {
> -        err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t),
> -                0);
> +        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
> +        err = setxattr(path, "user.virtfs.uid", &tmp_uid, sizeof(uid_t), 0);
>          if (err) {
>              return err;
>          }
>      }
>      if (credp->fc_gid != -1) {
> -        err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t),
> -                0);
> +        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
> +        err = setxattr(path, "user.virtfs.gid", &tmp_gid, sizeof(gid_t), 0);
>          if (err) {
>              return err;
>          }
>      }
>      if (credp->fc_mode != -1) {
> -        err = setxattr(path, "user.virtfs.mode", &credp->fc_mode,
> -                sizeof(mode_t), 0);
> +        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
> +        err = setxattr(path, "user.virtfs.mode", &tmp_mode, sizeof(mode_t), 0);
>          if (err) {
>              return err;
>          }
>      }
>      if (credp->fc_rdev != -1) {
> -        err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev,
> -                sizeof(dev_t), 0);
> +        uint64_t tmp_rdev = cpu_to_le32(credp->fc_rdev);
> +        err = setxattr(path, "user.virtfs.rdev", &tmp_rdev, sizeof(dev_t), 0);
>          if (err) {
>              return err;
>          }
> @@ -630,21 +630,17 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
>          mode_t tmp_mode;
>          dev_t tmp_dev;
>  
> -        if (fgetxattr(fd, "user.virtfs.uid",
> -                      &tmp_uid, sizeof(uid_t)) > 0) {
> -            stbuf->st_uid = tmp_uid;
> +        if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
> +            stbuf->st_uid = le32_to_cpu(tmp_uid);
>          }
> -        if (fgetxattr(fd, "user.virtfs.gid",
> -                      &tmp_gid, sizeof(gid_t)) > 0) {
> -            stbuf->st_gid = tmp_gid;
> +        if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
> +            stbuf->st_gid = le32_to_cpu(tmp_gid);
>          }
> -        if (fgetxattr(fd, "user.virtfs.mode",
> -                      &tmp_mode, sizeof(mode_t)) > 0) {
> -            stbuf->st_mode = tmp_mode;
> +        if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) {
> +            stbuf->st_mode = le32_to_cpu(tmp_mode);
>          }
> -        if (fgetxattr(fd, "user.virtfs.rdev",
> -                      &tmp_dev, sizeof(dev_t)) > 0) {
> -                stbuf->st_rdev = tmp_dev;
> +        if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
> +            stbuf->st_rdev = le64_to_cpu(tmp_dev);
>          }
>      } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>          errno = EOPNOTSUPP;
> 
> 

Re: [Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Aneesh Kumar K.V]

Michael Tokarev <mjt@tls.msk.ru> writes:

> I haven't noticed this email - which is almost a month old now - until today.
> So replying now...
>
> 30.07.2014 21:43, Aneesh Kumar K.V wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>> 
>>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>>
>>>> Apparently the the mapped-* security models results in a raw bytes
>>>> being dumped to host without any architecture normalization (in
>>>> host byte order).  This may even lead to security issues in guest
>>>> when the same files are served from another host for example.
>>>>
>>>> This bug has been initially submitted against debian qemu package, see
>>>> http://bugs.debian.org/755740
>>>>
>>>
>>> Thanks for reporting the bug. Yes we do have issue with
>>> mapped-xattr. But mapped-file should be ok. We record the uid/gid as
>>> string in the file.
>> 
>> What would be the best way to fix this in a backward compatible way ?
>> Considering most of the users will be little endian host, we could do "always
>> store in little endian format" which of-course will break big-endian
>> hosts. We could possibly ask them to update xattr using external tools ?
>
> If there's no way to _detect_ the used format (maybe doing some guessing, --
> if that's possible to do in a reliable way, it should be good), that's
> one of 2 possible options as I see it: that or introduce a new format
> entirely, maybe with another attribute name.
>
> It might not be even required to use an external tool for conversion.
> Again, if qemu is able to detect "wrong" endiannes, it might just
> update things itself, or print a warning and switch to an old format,
> or something like that.

I was not able to come up with a way to detect "wrong" endianness. 

>
> But the guessing idea might not be as bad really.  I haven't looked
> closely which information is stored in there, -- but it is possible
> that some fields should have zeros in some bytes for example, and
> if these aren't zero but becomes zeros after endianness conversion
> that might be a good indicator.


No, they are 32 bit numbers and we can't make any assumptions w.r.t
upper half/lower half being zero


>
> I'm not sure the runtime code should be able to work with both formats
> at the same time.  Actually, I'm not sure this is a big issue to
> start with -- indeed, you said it already, majority of users of 9pfs
> should be little endian hosts, -- are there any big endian hosts
> using this, at all? :)  How about trying to detect (preferrable at
> init time) and refusing to start if old/wrong format is detected.
>
> Maybe have a compile-time define to use native or little endian
> format is a good idea too.
>

That would confuse further. It also impact the interoperability of
export path across different build of qemus.


> Bastian, since you discovered this issue, you might be using
> a host with "uncommon" endianness, what do you think?
>

-aneesh