[Qemu-devel] [PULL 0/3] qemu-ga: security fixes for CVE-2013-2231

[Qemu-devel] [PULL 0/3] qemu-ga: security fixes for CVE-2013-2231

[Michael Roth]

More details on the nature of the CVE are available here:

http://seclists.org/oss-sec/2013/q3/161

The following changes since commit d2f5ea9704af781d4cf14e4be08bb4e37a180260:

  pc-testdev: add I/O port to test memory.c auto split/combine (2013-07-25 08:12:28 -0500)

are available in the git repository at:

  git://github.com/mdroth/qemu.git qga-pull-2013-7-25

for you to fetch changes up to 340d51df5592c5c11fc3885f7bdedbe581b87366:

  qga: escape cmdline args when registering win32 service (CVE-2013-2231) (2013-07-25 14:49:04 -0500)

----------------------------------------------------------------
Laszlo Ersek (3):
      qga/service-win32.c: diagnostic output should go to stderr
      ga_install_service(): nest error paths more idiomatically
      qga: escape cmdline args when registering win32 service (CVE-2013-2231)

 qga/service-win32.c |  105 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 88 insertions(+), 17 deletions(-)

[Qemu-devel] [PATCH 1/3] qga/service-win32.c: diagnostic output should go to stderr

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/service-win32.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qga/service-win32.c b/qga/service-win32.c
index 02926ab..6de8be4 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -29,7 +29,7 @@ static int printf_win_error(const char *text)
         MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
         (char *)&message, 0,
         NULL);
-    n = printf("%s. (Error: %d) %s", text, (int)err, message);
+    n = fprintf(stderr, "%s. (Error: %d) %s", text, (int)err, message);
     LocalFree(message);
 
     return n;
@@ -78,7 +78,7 @@ int ga_install_service(const char *path, const char *logfile,
         SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
         ChangeServiceConfig2(service, SERVICE_CONFIG_DESCRIPTION, &desc);
 
-        printf("Service was installed successfully.\n");
+        fprintf(stderr, "Service was installed successfully.\n");
     } else {
         printf_win_error("Failed to install service");
     }
@@ -111,7 +111,7 @@ int ga_uninstall_service(void)
     if (DeleteService(service) == FALSE) {
         printf_win_error("Failed to delete service");
     } else {
-        printf("Service was deleted successfully.\n");
+        fprintf(stderr, "Service was deleted successfully.\n");
     }
 
     CloseServiceHandle(service);
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/3] ga_install_service(): nest error paths more idiomatically

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/service-win32.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/qga/service-win32.c b/qga/service-win32.c
index 6de8be4..1cf81fc 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -38,10 +38,12 @@ static int printf_win_error(const char *text)
 int ga_install_service(const char *path, const char *logfile,
                        const char *state_dir)
 {
+    int ret = EXIT_FAILURE;
     SC_HANDLE manager;
     SC_HANDLE service;
     TCHAR module_fname[MAX_PATH];
     GString *cmdline;
+    SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
 
     if (GetModuleFileName(NULL, module_fname, MAX_PATH) == 0) {
         printf_win_error("No full path to service's executable");
@@ -66,28 +68,28 @@ int ga_install_service(const char *path, const char *logfile,
     manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
     if (manager == NULL) {
         printf_win_error("No handle to service control manager");
-        g_string_free(cmdline, TRUE);
-        return EXIT_FAILURE;
+        goto out_strings;
     }
 
     service = CreateService(manager, QGA_SERVICE_NAME, QGA_SERVICE_DISPLAY_NAME,
         SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS, SERVICE_AUTO_START,
         SERVICE_ERROR_NORMAL, cmdline->str, NULL, NULL, NULL, NULL, NULL);
-
-    if (service) {
-        SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
-        ChangeServiceConfig2(service, SERVICE_CONFIG_DESCRIPTION, &desc);
-
-        fprintf(stderr, "Service was installed successfully.\n");
-    } else {
+    if (service == NULL) {
         printf_win_error("Failed to install service");
+        goto out_manager;
     }
 
+    ChangeServiceConfig2(service, SERVICE_CONFIG_DESCRIPTION, &desc);
+    fprintf(stderr, "Service was installed successfully.\n");
+    ret = EXIT_SUCCESS;
     CloseServiceHandle(service);
+
+out_manager:
     CloseServiceHandle(manager);
 
+out_strings:
     g_string_free(cmdline, TRUE);
-    return (service == NULL);
+    return ret;
 }
 
 int ga_uninstall_service(void)
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/3] qga: escape cmdline args when registering win32 service (CVE-2013-2231)

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Reported-by: Lev Veyde <lveyde@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/service-win32.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 5 deletions(-)

diff --git a/qga/service-win32.c b/qga/service-win32.c
index 1cf81fc..aef41f0 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -35,6 +35,68 @@ static int printf_win_error(const char *text)
     return n;
 }
 
+/* Windows command line escaping. Based on
+ * <http://blogs.msdn.com/b/oldnewthing/archive/2010/09/17/10063629.aspx> and
+ * <http://msdn.microsoft.com/en-us/library/windows/desktop/17w5ykft%28v=vs.85%29.aspx>.
+ *
+ * The caller is responsible for initializing @buffer; prior contents are lost.
+ */
+static const char *win_escape_arg(const char *to_escape, GString *buffer)
+{
+    size_t backslash_count;
+    const char *c;
+
+    /* open with a double quote */
+    g_string_assign(buffer, "\"");
+
+    backslash_count = 0;
+    for (c = to_escape; *c != '\0'; ++c) {
+        switch (*c) {
+        case '\\':
+            /* The meaning depends on the first non-backslash character coming
+             * up.
+             */
+            ++backslash_count;
+            break;
+
+        case '"':
+            /* We must escape each pending backslash, then escape the double
+             * quote. This creates a case of "odd number of backslashes [...]
+             * followed by a double quotation mark".
+             */
+            while (backslash_count) {
+                --backslash_count;
+                g_string_append(buffer, "\\\\");
+            }
+            g_string_append(buffer, "\\\"");
+            break;
+
+        default:
+            /* Any pending backslashes are without special meaning, flush them.
+             * "Backslashes are interpreted literally, unless they immediately
+             * precede a double quotation mark."
+             */
+            while (backslash_count) {
+                --backslash_count;
+                g_string_append_c(buffer, '\\');
+            }
+            g_string_append_c(buffer, *c);
+        }
+    }
+
+    /* We're about to close with a double quote in string delimiter role.
+     * Double all pending backslashes, creating a case of "even number of
+     * backslashes [...] followed by a double quotation mark".
+     */
+    while (backslash_count) {
+        --backslash_count;
+        g_string_append(buffer, "\\\\");
+    }
+    g_string_append_c(buffer, '"');
+
+    return buffer->str;
+}
+
 int ga_install_service(const char *path, const char *logfile,
                        const char *state_dir)
 {
@@ -42,6 +104,7 @@ int ga_install_service(const char *path, const char *logfile,
     SC_HANDLE manager;
     SC_HANDLE service;
     TCHAR module_fname[MAX_PATH];
+    GString *esc;
     GString *cmdline;
     SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
 
@@ -50,17 +113,22 @@ int ga_install_service(const char *path, const char *logfile,
         return EXIT_FAILURE;
     }
 
-    cmdline = g_string_new(module_fname);
-    g_string_append(cmdline, " -d");
+    esc     = g_string_new("");
+    cmdline = g_string_new("");
+
+    g_string_append_printf(cmdline, "%s -d",
+                           win_escape_arg(module_fname, esc));
 
     if (path) {
-        g_string_append_printf(cmdline, " -p %s", path);
+        g_string_append_printf(cmdline, " -p %s", win_escape_arg(path, esc));
     }
     if (logfile) {
-        g_string_append_printf(cmdline, " -l %s -v", logfile);
+        g_string_append_printf(cmdline, " -l %s -v",
+                               win_escape_arg(logfile, esc));
     }
     if (state_dir) {
-        g_string_append_printf(cmdline, " -t %s", state_dir);
+        g_string_append_printf(cmdline, " -t %s",
+                               win_escape_arg(state_dir, esc));
     }
 
     g_debug("service's cmdline: %s", cmdline->str);
@@ -89,6 +157,7 @@ out_manager:
 
 out_strings:
     g_string_free(cmdline, TRUE);
+    g_string_free(esc, TRUE);
     return ret;
 }
 
-- 
1.7.9.5