Merge remote-tracking branch 'mdroth/qga-pull-2013-7-25' into staging

# By Laszlo Ersek
# Via Michael Roth
* mdroth/qga-pull-2013-7-25:
  qga: escape cmdline args when registering win32 service (CVE-2013-2231)
  ga_install_service(): nest error paths more idiomatically
  qga/service-win32.c: diagnostic output should go to stderr

Message-id: 1374784644-29078-1-git-send-email-mdroth@linux.vnet.ibm.com

qga/service-win32.c

@@ -29,36 +29,106 @@ 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;
 }
 
+/* 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)
 {
+    int ret = EXIT_FAILURE;
     SC_HANDLE manager;
     SC_HANDLE service;
     TCHAR module_fname[MAX_PATH];
+    GString *esc;
     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");
         return EXIT_FAILURE;
     }
 
-    cmdline = g_string_new(module_fname);
+    esc     = g_string_new("");
-    g_string_append(cmdline, " -d");
+    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);
@@ -66,28 +136,29 @@ 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);
+        goto out_strings;
-        return EXIT_FAILURE;
     }
 
     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 == NULL) {
-    if (service) {
-        SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
-        ChangeServiceConfig2(service, SERVICE_CONFIG_DESCRIPTION, &desc);
-
-        printf("Service was installed successfully.\n");
-    } else {
         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);
+    g_string_free(esc, TRUE);
+    return ret;
 }
 
 int ga_uninstall_service(void)
@@ -111,7 +182,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);