[PATCH 1/2] nwfilter: avoid failure with noexec /tmp

[Eric Blake <eblake@xxxxxxxxxx>]

If /tmp is mounted with the noexec flag (common on security-conscious
systems), then nwfilter will fail to initialize, because we cannot
run any temporary script via virRun("/tmp/script"); but we _can_
use "/bin/sh /tmp/script".  For that matter, using /tmp risks collisions
with other unrelated programs; we already have /var/run/libvirt as a
dedicated temporary directory for use by libvirt.

* src/nwfilter/nwfilter_ebiptables_driver.c
(ebiptablesWriteToTempFile): Use internal directory, not /tmp;
drop attempts to make script executable; and detect close error.
(ebiptablesExecCLI): Switch to virCommand, and invoke the shell to
read the script, rather than requiring an executable script.
---
 src/nwfilter/nwfilter_ebiptables_driver.c |   76 ++++++++---------------------
 1 files changed, 21 insertions(+), 55 deletions(-)

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c
index f87cfa1..c9c194c 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -40,6 +40,7 @@
 #include "nwfilter_ebiptables_driver.h"
 #include "virfile.h"
 #include "command.h"
+#include "configmake.h"


 #define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -2482,29 +2483,17 @@ ebiptablesDisplayRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED,
  * NULL in case of error with the error reported.
  *
  * Write the string into a temporary file and return the name of
- * the temporary file. The string is assumed to contain executable
- * commands. A line '#!/bin/sh' will automatically be written
- * as the first line in the file. The permissions of the file are
- * set so that the file can be run as an executable script.
+ * the temporary file. The file can then be read as a /bin/sh script.
+ * No '#!/bin/sh' header is needed, since the file will be read and not
+ * directly executed.
  */
 static char *
 ebiptablesWriteToTempFile(const char *string) {
-    char filename[] = "/tmp/virtdXXXXXX";
-    int len;
+    char filename[] = LOCALSTATEDIR "/run/libvirt/nwfilt-XXXXXX";
+    size_t len;
     char *filnam;
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
-    char *header;
     size_t written;

-    virBufferAddLit(&buf, "#!/bin/sh\n");
-
-    if (virBufferError(&buf)) {
-        virBufferFreeAndReset(&buf);
-        virReportOOMError();
-        return NULL;
-    }
-    header = virBufferContentAndReset(&buf);
-
     int fd = mkstemp(filename);

     if (fd < 0) {
@@ -2514,15 +2503,8 @@ ebiptablesWriteToTempFile(const char *string) {
         goto err_exit;
     }

-    if (fchmod(fd, S_IXUSR| S_IRUSR | S_IWUSR) < 0) {
-        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
-                               "%s",
-                               _("cannot change permissions on temp. file"));
-        goto err_exit;
-    }
-
-    len = strlen(header);
-    written = safewrite(fd, header, len);
+    len = strlen(string);
+    written = safewrite(fd, string, len);
     if (written != len) {
         virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                "%s",
@@ -2530,9 +2512,7 @@ ebiptablesWriteToTempFile(const char *string) {
         goto err_exit;
     }

-    len = strlen(string);
-    written = safewrite(fd, string, len);
-    if (written != len) {
+    if (VIR_CLOSE(fd) < 0) {
         virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                "%s",
                                _("cannot write string to file"));
@@ -2545,12 +2525,9 @@ ebiptablesWriteToTempFile(const char *string) {
         goto err_exit;
     }

-    VIR_FREE(header);
-    VIR_FORCE_CLOSE(fd);
     return filnam;

 err_exit:
-    VIR_FREE(header);
     VIR_FORCE_CLOSE(fd);
     unlink(filename);
     return NULL;
@@ -2564,7 +2541,7 @@ err_exit:
  * @status: Pointer to an integer for returning the WEXITSTATUS of the
  *        commands executed via the script the was run.
  *
- * Returns 0 in case of success, != 0 in case of an error. The returned
+ * Returns 0 in case of success, < 0 in case of an error. The returned
  * value is NOT the result of running the commands inside the shell
  * script.
  *
@@ -2577,53 +2554,42 @@ ebiptablesExecCLI(virBufferPtr buf,
 {
     char *cmds;
     char *filename;
-    int rc;
-    const char *argv[] = {NULL, NULL};
+    int rc = -1;
+    virCommandPtr cmd;

     if (virBufferError(buf)) {
         virReportOOMError();
         virBufferFreeAndReset(buf);
-        return 1;
+        return -1;
     }

     *status = 0;

     cmds = virBufferContentAndReset(buf);
-
     VIR_DEBUG("%s", NULLSTR(cmds));
-
     if (!cmds)
         return 0;

     filename = ebiptablesWriteToTempFile(cmds);
-    VIR_FREE(cmds);
-
     if (!filename)
-        return 1;
+        goto cleanup;

-    argv[0] = filename;
+    cmd = virCommandNew("/bin/sh");
+    virCommandAddArg(cmd, filename);

     virMutexLock(&execCLIMutex);

-    rc = virRun(argv, status);
+    rc = virCommandRun(cmd, status);

     virMutexUnlock(&execCLIMutex);

-    if (rc == 0) {
-        if (WIFEXITED(*status)) {
-            *status = WEXITSTATUS(*status);
-        } else {
-            rc = -1;
-            *status = 1;
-        }
-    }
-
-    VIR_DEBUG("rc = %d, status = %d", rc, *status);
-
     unlink(filename);
-
     VIR_FREE(filename);

+cleanup:
+    VIR_FREE(cmds);
+    virCommandFree(cmd);
+
     return rc;
 }

-- 
1.7.4.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list