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

[Stefan Berger]

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 ...