[libvirt] [PATCH 0/2] fix nwfilter when /tmp is mounted noexec
https://bugzilla.redhat.com/show_bug.cgi?id=752254 points out that libvirt cannot support nwfilter on a system with /tmp mounted noexec (which is a very common setup in security-conscious setups), all because we were trying to directly invoke a temporary script instead of invoking a shell to read the script. I've split this patch into 2 parts, on the off-chance that patch 2 would run afoul of command line length limits (if the total size of the generated nwfilter commands could possibly cause E2BIG, then we have to go through a temporary file). But my recollection is that modern Linux kernels support unlimited command-line length (that is, ARG_MAX is not a concern on Linux), and that nwfilter_ebiptables_driver only compiles on Linux, so my preference would be to squash these into a single commit, if others agree that we don't have to worry about length limits. At any rate, I'm quite impressed at the number of lines of code I was able to remove in order to fix a bug! Eric Blake (2): nwfilter: avoid failure with noexec /tmp nwfilter: simplify execution of ebiptables scripts src/nwfilter/nwfilter_ebiptables_driver.c | 134 ++-------------------------- 1 files changed, 10 insertions(+), 124 deletions(-) -- 1.7.4.4
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
On 11/09/2011 12:46 PM, Eric Blake wrote:
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; }
ACK, still works with this one ...
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; -- 1.7.4.4
On 11/09/2011 12:46 PM, Eric Blake wrote:
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; Here I had to insert:
if (!virBufferUse(buf)) return 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; ACK with above nit fixed so it still works.
On 11/09/2011 11:39 AM, Stefan Berger wrote:
On 11/09/2011 12:46 PM, Eric Blake wrote:
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.
*status = 0; Here I had to insert:
if (!virBufferUse(buf)) return 0;
- cmds = virBufferContentAndReset(buf); - VIR_DEBUG("%s", NULLSTR(cmds)); - if (!cmds) - return 0;
Ah, I see - the old code declared early success on no commands to run, while the new code passes an empty buffer to virCommand; and right now, virCommand has a bug that an empty buffer becomes NULL instead of an explicit empty argument (patch for that comming up next). But if there are no commands to run, then we can skip virCommand altogether (/bin/sh -c '' will always succeed).
ACK with above nit fixed so it still works.
I've pushed the two patches with that fixed; I decided to keep them separate for easier reversion of patch 2 if it turns out I was wrong and we ever hit an E2BIG error due to not having unlimited command line length. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/09/2011 07:12 PM, Eric Blake wrote:
ACK with above nit fixed so it still works.
I've pushed the two patches with that fixed; I decided to keep them separate for easier reversion of patch 2 if it turns out I was wrong and we ever hit an E2BIG error due to not having unlimited command line length.
Is 'getconf ARG_MAX' the command to use to determine the max. command line size? Stefan
On 11/09/2011 06:43 PM, Stefan Berger wrote:
On 11/09/2011 07:12 PM, Eric Blake wrote:
ACK with above nit fixed so it still works.
I've pushed the two patches with that fixed; I decided to keep them separate for easier reversion of patch 2 if it turns out I was wrong and we ever hit an E2BIG error due to not having unlimited command line length.
Is 'getconf ARG_MAX' the command to use to determine the max. command line size?
Thanks for the reminder. Yes, 'getconf ARG_MAX' is the current system limit, although getconf(1) isn't present on some systems (cygwin and mingw come rapidly to my mind). On my Fedora 14 system, I see a 2M limit for ARG_MAX; empirically, mingw has a much smaller limit of 32k. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/09/2011 10:46 AM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=752254 points out that libvirt cannot support nwfilter on a system with /tmp mounted noexec (which is a very common setup in security-conscious setups), all because we were trying to directly invoke a temporary script instead of invoking a shell to read the script.
I've split this patch into 2 parts, on the off-chance that patch 2 would run afoul of command line length limits (if the total size of the generated nwfilter commands could possibly cause E2BIG, then we have to go through a temporary file). But my recollection is that modern Linux kernels support unlimited command-line length (that is, ARG_MAX is not a concern on Linux), and that nwfilter_ebiptables_driver only compiles on Linux, so my preference would be to squash these into a single commit, if others agree that we don't have to worry about length limits.
At any rate, I'm quite impressed at the number of lines of code I was able to remove in order to fix a bug!
Eric Blake (2): nwfilter: avoid failure with noexec /tmp nwfilter: simplify execution of ebiptables scripts
src/nwfilter/nwfilter_ebiptables_driver.c | 134 ++-------------------------- 1 files changed, 10 insertions(+), 124 deletions(-)
This series does not solve a more fundamental issue that has been on my mind - are we still sure that the best design for manipulating network filters involves the creation of a series of shell scripting commands, where we have to worry about proper quoting and so forth? Is it possible to refactor this code to make more direct use of virCommand for every call to iptables and friends (that is, doing the glue logic in C rather than using C to generate shell scripting commands where the glue logic is in generated sh)? Or perhaps to even refactor things into a well-defined file format that we can feed to a helper executable, which would allow finer-grained SELinux labelling (by isolating the direct execution of iptables into a well-defined helper executable, then SELinux can enforce that libvirtd cannot alter the host firewall except by going through the helper executable, which has been audited to make only known sets of iptables calls based on well-formed input)? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Nov 09, 2011 at 11:01:58AM -0700, Eric Blake wrote:
This series does not solve a more fundamental issue that has been on my mind - are we still sure that the best design for manipulating network filters involves the creation of a series of shell scripting commands, where we have to worry about proper quoting and so forth? Is it possible to refactor this code to make more direct use of virCommand for every call to iptables and friends (that is, doing the glue logic in C rather than using C to generate shell scripting commands where the glue logic is in generated sh)? Or perhaps to even refactor things into a well-defined file format that we can feed to a helper executable, which would allow finer-grained SELinux labelling (by isolating the direct execution of iptables into a well-defined helper executable, then SELinux can enforce that libvirtd cannot alter the host firewall except by going through the helper executable, which has been audited to make only known sets of iptables calls based on well-formed input)?
I think virCommand is a little too low level to do what nwfilter wants here, but we can definitely build a higher level API around it to replace the shell scripting. I has been a while since I looked at the generated scripts, IIUC, the shell scripts generated are basically defining groups of iptables commands to make "interesting" changes, and periodically a "rollback" command which is run if something fails. So I was thinking you could design a libvirt level API that would be used something like: virCommandScriptPtr script = virCommandScriptNew(); virCommandScriptBeginGroup(script); virCommandScriptAddCommandArgList(script, "/some/command", args.....); virCommandScriptAddRollbackArgList(script, "/some/command", args...); virCommandScriptBeginGroup(script); virCommandScriptAddCommandArgList(script, "/some/command", args.....); virCommandScriptAddCommandArgList(script, "/some/command", args.....); virCommandScriptAddCommandArgList(script, "/some/command", args.....); ...repeat... virCommandScriptAddRollbackArgList(script, "/some/command", args...); virCommandScriptRun(); If one of the commands following the 'BeginGroup' failed, then it would execute the Rollback command for that group, and for every preceeding group. Personally I would dearly love to kill off this use of shell scripting in libvirt, because I fear it is only a matter of time before we introduce some flaw wrt escaping. In general I'd like to be able to say that libvirt *never* runs shell commands, always uses execve() directly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/09/2011 01:01 PM, Eric Blake wrote:
On 11/09/2011 10:46 AM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=752254 points out that libvirt cannot support nwfilter on a system with /tmp mounted noexec (which is a very common setup in security-conscious setups), all because we were trying to directly invoke a temporary script instead of invoking a shell to read the script.
I've split this patch into 2 parts, on the off-chance that patch 2 would run afoul of command line length limits (if the total size of the generated nwfilter commands could possibly cause E2BIG, then we have to go through a temporary file). But my recollection is that modern Linux kernels support unlimited command-line length (that is, ARG_MAX is not a concern on Linux), and that nwfilter_ebiptables_driver only compiles on Linux, so my preference would be to squash these into a single commit, if others agree that we don't have to worry about length limits.
At any rate, I'm quite impressed at the number of lines of code I was able to remove in order to fix a bug!
Eric Blake (2): nwfilter: avoid failure with noexec /tmp nwfilter: simplify execution of ebiptables scripts
src/nwfilter/nwfilter_ebiptables_driver.c | 134 ++-------------------------- 1 files changed, 10 insertions(+), 124 deletions(-)
This series does not solve a more fundamental issue that has been on my mind - are we still sure that the best design for manipulating network filters involves the creation of a series of shell scripting commands, where we have to worry about proper quoting and so forth? Is it possible to refactor this code to make more direct use of We should at least have the quoting under control... virCommand for every call to iptables and friends (that is, doing the glue logic in C rather than using C to generate shell scripting commands where the glue logic is in generated sh)? Or perhaps to even refactor things into a well-defined file format that we can feed to a helper executable, which would allow finer-grained SELinux labelling
I am sure the refactoring would be possible, but wouldn't it be more efficient if this type of post-processing was done in a batch with as many commands collected as possible ? So that would mean keeping the general code but finding a well-defined file format for probably ebtables and iptables rules.
(by isolating the direct execution of iptables into a well-defined helper executable, then SELinux can enforce that libvirtd cannot alter the host firewall except by going through the helper executable, which has been audited to make only known sets of iptables calls based on well-formed input)?
The Network filter subsystem only touches the FORWARD chain of iptables and none of the INPUT chains or its 'descendants'. Of course one can double -check that in another process. Stefan
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger