[Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Laszlo Ersek]

I should have paid more attention to portability and error path cleanup
in the CVE-2013-2007 fix.

(We continue to assume, like the rest of qemu code, that
qemu_set_cloexec() never fails internally. This should be a reasonable
assumption when the input fd is valid.)

Laszlo Ersek (2):
  qga: distinguish binary modes in "guest_file_open_modes" map
  qga: unlink just created guest-file if fchmod() or fdopen() fails on
    it

 qga/commands-posix.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH v2 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Laszlo Ersek]

In Windows guests this may make a difference.

Since the original patch (commit c689b4f1) sought to be pedantic and to
consider theoretical corner cases of portability, we should fix it up
where it failed to come through in that pursuit.

Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 04c6951..2eec712 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -242,17 +242,27 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 
 typedef const char * const ccpc;
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
 static const struct {
     ccpc *forms;
     int oflag_base;
 } guest_file_open_modes[] = {
-    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
-    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
-    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
-    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+    { (ccpc[]){ "r",          NULL }, O_RDONLY                                 },
+    { (ccpc[]){ "rb",         NULL }, O_RDONLY                      | O_BINARY },
+    { (ccpc[]){ "w",          NULL }, O_WRONLY | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a",          NULL }, O_WRONLY | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND | O_BINARY },
+    { (ccpc[]){ "r+",         NULL }, O_RDWR                                   },
+    { (ccpc[]){ "rb+", "r+b", NULL }, O_RDWR                        | O_BINARY },
+    { (ccpc[]){ "w+",         NULL }, O_RDWR   | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a+",         NULL }, O_RDWR   | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND | O_BINARY }
 };
 
 static int
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/2] qga: unlink just created guest-file if fchmod() or fdopen() fails on it

[Laszlo Ersek]

We shouldn't allow guest filesystem pollution on error paths.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2eec712..e199738 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -355,6 +355,9 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
             }
 
             close(fd);
+            if (oflag & O_CREAT) {
+                unlink(path);
+            }
         }
     }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 2/2] qga: unlink just created guest-file if fchmod() or fdopen() fails on it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On 05/08/2013 09:31 AM, Laszlo Ersek wrote:
> We shouldn't allow guest filesystem pollution on error paths.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 2eec712..e199738 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -355,6 +355,9 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
>              }
>  
>              close(fd);
> +            if (oflag & O_CREAT) {
> +                unlink(path);
> +            }
>          }
>      }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Luiz Capitulino]

On Wed,  8 May 2013 17:31:34 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> I should have paid more attention to portability and error path cleanup
> in the CVE-2013-2007 fix.
> 
> (We continue to assume, like the rest of qemu code, that
> qemu_set_cloexec() never fails internally. This should be a reasonable
> assumption when the input fd is valid.)

Series:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Laszlo Ersek (2):
>   qga: distinguish binary modes in "guest_file_open_modes" map
>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
>     it
> 
>  qga/commands-posix.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[mdroth]

On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
> I should have paid more attention to portability and error path cleanup
> in the CVE-2013-2007 fix.
> 
> (We continue to assume, like the rest of qemu code, that
> qemu_set_cloexec() never fails internally. This should be a reasonable
> assumption when the input fd is valid.)
> 
> Laszlo Ersek (2):
>   qga: distinguish binary modes in "guest_file_open_modes" map
>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
>     it

Thanks, applied to qga branch:

https://github.com/mdroth/qemu/commits/qga

> 
>  qga/commands-posix.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Laszlo Ersek]

On 05/10/13 21:30, mdroth wrote:
> On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
>> I should have paid more attention to portability and error path cleanup
>> in the CVE-2013-2007 fix.
>>
>> (We continue to assume, like the rest of qemu code, that
>> qemu_set_cloexec() never fails internally. This should be a reasonable
>> assumption when the input fd is valid.)
>>
>> Laszlo Ersek (2):
>>   qga: distinguish binary modes in "guest_file_open_modes" map
>>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
>>     it
> 
> Thanks, applied to qga branch:
> 
> https://github.com/mdroth/qemu/commits/qga

Thanks!

Can you reword the second commit to include Eric's R-b?
<http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html>

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[mdroth]

On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote:
> On 05/10/13 21:30, mdroth wrote:
> > On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
> >> I should have paid more attention to portability and error path cleanup
> >> in the CVE-2013-2007 fix.
> >>
> >> (We continue to assume, like the rest of qemu code, that
> >> qemu_set_cloexec() never fails internally. This should be a reasonable
> >> assumption when the input fd is valid.)
> >>
> >> Laszlo Ersek (2):
> >>   qga: distinguish binary modes in "guest_file_open_modes" map
> >>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
> >>     it
> > 
> > Thanks, applied to qga branch:
> > 
> > https://github.com/mdroth/qemu/commits/qga
> 
> Thanks!
> 
> Can you reword the second commit to include Eric's R-b?
> <http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html>

Sure, missed that one. Should be fixed in tree now.

> 
> Thanks!
> Laszlo
> 

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[Andreas Färber]

Am 10.05.2013 22:09, schrieb mdroth:
> On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote:
>> On 05/10/13 21:30, mdroth wrote:
>>> On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
>>>> I should have paid more attention to portability and error path cleanup
>>>> in the CVE-2013-2007 fix.
>>>>
>>>> (We continue to assume, like the rest of qemu code, that
>>>> qemu_set_cloexec() never fails internally. This should be a reasonable
>>>> assumption when the input fd is valid.)
>>>>
>>>> Laszlo Ersek (2):
>>>>   qga: distinguish binary modes in "guest_file_open_modes" map
>>>>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
>>>>     it
>>>
>>> Thanks, applied to qga branch:
>>>
>>> https://github.com/mdroth/qemu/commits/qga
>>
>> Thanks!
>>
>> Can you reword the second commit to include Eric's R-b?
>> <http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html>
> 
> Sure, missed that one. Should be fixed in tree now.

Shouldn't at least the unlinking be backported to stable as well?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda

[mdroth]

On Sun, May 12, 2013 at 06:47:05PM +0200, Andreas Färber wrote:
> Am 10.05.2013 22:09, schrieb mdroth:
> > On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote:
> >> On 05/10/13 21:30, mdroth wrote:
> >>> On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote:
> >>>> I should have paid more attention to portability and error path cleanup
> >>>> in the CVE-2013-2007 fix.
> >>>>
> >>>> (We continue to assume, like the rest of qemu code, that
> >>>> qemu_set_cloexec() never fails internally. This should be a reasonable
> >>>> assumption when the input fd is valid.)
> >>>>
> >>>> Laszlo Ersek (2):
> >>>>   qga: distinguish binary modes in "guest_file_open_modes" map
> >>>>   qga: unlink just created guest-file if fchmod() or fdopen() fails on
> >>>>     it
> >>>
> >>> Thanks, applied to qga branch:
> >>>
> >>> https://github.com/mdroth/qemu/commits/qga
> >>
> >> Thanks!
> >>
> >> Can you reword the second commit to include Eric's R-b?
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html>
> > 
> > Sure, missed that one. Should be fixed in tree now.
> 
> Shouldn't at least the unlinking be backported to stable as well?

Yes, these are basically updates to the CVE fix, so I think they should all
be applied to stable. I'll send PULL today so hopefully we can get them
into 1.5 prior to patch freeze for 1.4.2. Otherwise I'll backport from the
qga tree.

> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>