[libvirt] [PATCH v2] storage: support all file permissions

[Laine Stump]

On 07/26/2012 04:52 AM, Ján Tomko wrote:
> sticky, setuid and setgid are no longer ignored.

I'm always automatically wary of any code that allows setting the suid
bit, in case it may allow some new security hole. I can't think of
anything specific that could be allowed by setting the suid bit of a
directory containing a disk image, but then again I haven't thought
about it very hard :-) Can anyone convince me one way or the other?

It might help to learn why you want to be able to set these bits.
libvirt is generally very specific about explicitly setting the uid of
disk images properly as required at runtime...


>
> ---
> v2: reading these permissions from XML works too
> ---
>  src/conf/storage_conf.c       |    9 ++++++---
>  src/storage/storage_backend.c |    3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 7944555..0335f99 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -665,7 +665,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>      } else {
>          int tmp;
>  
> -        if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
> +        if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 ||
> +            (tmp & ~(S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO))) {
>              VIR_FREE(mode);
>              virReportError(VIR_ERR_XML_ERROR,
>                             "%s", _("malformed octal mode"));
> @@ -1035,7 +1036,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) {
>  
>          virBufferAddLit(&buf,"    <permissions>\n");
>          virBufferAsprintf(&buf,"      <mode>0%o</mode>\n",
> -                          def->target.perms.mode);
> +                          def->target.perms.mode &
> +                          (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO));
>          virBufferAsprintf(&buf,"      <owner>%u</owner>\n",
>                            (unsigned int) def->target.perms.uid);
>          virBufferAsprintf(&buf,"      <group>%u</group>\n",
> @@ -1275,7 +1277,8 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
>  
>      virBufferAddLit(buf,"    <permissions>\n");
>      virBufferAsprintf(buf,"      <mode>0%o</mode>\n",
> -                      def->perms.mode);
> +                      def->perms.mode &
> +                      (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO));
>      virBufferAsprintf(buf,"      <owner>%u</owner>\n",
>                        (unsigned int) def->perms.uid);
>      virBufferAsprintf(buf,"      <group>%u</group>\n",
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index e677cda..e9ec7a5 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1210,7 +1210,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
>          }
>      }
>  
> -    target->perms.mode = sb.st_mode & S_IRWXUGO;
> +    target->perms.mode = sb.st_mode &
> +                         (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO);
>      target->perms.uid = sb.st_uid;
>      target->perms.gid = sb.st_gid;
>