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; }

It's not worth even worrying about a temporary file, unless we ever expect the script to exceed maximum command-line argument length limits. * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI): Run the commands as an argument to /bin/sh, rather than worrying about a temporary file. (ebiptablesWriteToTempFile): Delete unused function. --- src/nwfilter/nwfilter_ebiptables_driver.c | 88 +--------------------------- 1 files changed, 4 insertions(+), 84 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index c9c194c..aacbd02 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2476,65 +2476,6 @@ ebiptablesDisplayRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, /** - * ebiptablesWriteToTempFile: - * @string : the string to write into the file - * - * Returns the tempory filename where the string was written into, - * 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 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[] = LOCALSTATEDIR "/run/libvirt/nwfilt-XXXXXX"; - size_t len; - char *filnam; - size_t written; - - int fd = mkstemp(filename); - - if (fd< 0) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("cannot create temporary file")); - goto err_exit; - } - - len = strlen(string); - written = safewrite(fd, string, len); - if (written != len) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("cannot write string to file")); - goto err_exit; - } - - if (VIR_CLOSE(fd)< 0) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("cannot write string to file")); - goto err_exit; - } - - filnam = strdup(filename); - if (!filnam) { - virReportOOMError(); - goto err_exit; - } - - return filnam; - -err_exit: - VIR_FORCE_CLOSE(fd); - unlink(filename); - return NULL; -} - - -/** * ebiptablesExecCLI: * @buf : pointer to virBuffer containing the string with the commands to * execute. @@ -2546,36 +2487,20 @@ err_exit: * script. * * Execute a sequence of commands (held in the given buffer) as a /bin/sh - * script and return the status of the execution. + * script and return the status of the execution in *status (if status is + * NULL, then the script must exit with status 0). */ static int ebiptablesExecCLI(virBufferPtr buf, int *status) { - char *cmds; - char *filename; int rc = -1; virCommandPtr cmd; - if (virBufferError(buf)) { - virReportOOMError(); - virBufferFreeAndReset(buf); - return -1; - } - *status = 0;

- cmds = virBufferContentAndReset(buf); - VIR_DEBUG("%s", NULLSTR(cmds)); - if (!cmds) - return 0; - - filename = ebiptablesWriteToTempFile(cmds); - if (!filename) - goto cleanup; - - cmd = virCommandNew("/bin/sh"); - virCommandAddArg(cmd, filename); + cmd = virCommandNewArgList("/bin/sh", "-c", NULL); + virCommandAddArgBuffer(cmd, buf); virMutexLock(&execCLIMutex); @@ -2583,11 +2508,6 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexUnlock(&execCLIMutex); - unlink(filename); - VIR_FREE(filename); - -cleanup: - VIR_FREE(cmds); virCommandFree(cmd); return rc;