On 06/17/2010 06:20 AM, Daniel P. Berrange wrote:
On Wed, Jun 16, 2010 at 03:57:50PM -0500, Jamie Strandboge wrote:
On Wed, 2010-06-16 at 14:04 -0600, Eric Blake wrote:
I'm with Stefan - the whole point of mkstemp is that the created file
has 0600 permissions, and /tmp is restricted-deletion, so no other user
can either overwrite the file contents or unlink it and replace it with
an alternate file. Then again, gnulib documents that glibc 2.0.7 or
older would create a file with group/other permissions if the umask
wasn't set prior to the mkstemp() call, and gnulib's mkstemp() does not
work around this issue; but that's a rather old version of glibc to be
worrying about.
This has nothing to do with mkstemp(). As I said, libvirt's use of it is
fine and there is no symlink race or security vulnerability by itself.
The issue is that use of /tmp is not required *and* it becomes difficult
to properly confine libvirtd via a MAC if you must allow execution of
files in /tmp. See my answer to Stefan's question for an example
scenario.
ACK to your patch but really this whole set of code needs to be shot.
I had tried to get rid of the script creation a while ago, but couldn't
get rid of all of them, particularly those that create the 'anchors' for
the iptables are a bit more complicated where I run tests whether the
necessary rules already exists and only if they don't exist run a
certain set of iptables commands to create user-defined tables into
which libvirt then builds the rules pertaining to individual VMs.
Also, the whole XML filter tree is first translated into iptables rules
to detect errors there (references to filters that don't exist) and then
run, rather than executing the rules individually while translating a
single XML rule. Further, some of the rules may fail, particularly those
that do the tear-down, but don't cause the script to abort. Others may
not fail and would cause the abort and the cleanup of the rules
afterwards. So, what previously was a construct like this here that
checked the return code
cmd='iptables ...'
${cmd}
if [ $? -ne 0 ]; then
echo "Error: command ${cmd} failed."
exit 1;
fi
became something like this here:
iptables ...
STOPONERROR
where STOPONERROR was basically emulating the check of the iptables
status code above. So it was processing a command like iptables, split
its parameters into an array for passing to the libvirt api, then
checked whether the next command was a STOPONERROR and took appropriate
action if the status code was != 0.
The patch from a while ago is attached.
Stefan
Subject: Replace as much external scripting with calls to virRun
This patch replaces as much external scripting as possible with calls to
virRun(). The structure of the code is maintained as much as possible
and what previously ran an external script now executes the commands
one after the other using virRun(). Commands whose return value may not
indicate failure terminate the execution due to a string 'STOPONERROR' being
found in the 'batch'.
Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxx>
---
src/nwfilter/nwfilter_ebiptables_driver.c | 199 +++++++++++++++++++++++++++++-
1 file changed, 198 insertions(+), 1 deletion(-)
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -23,6 +23,7 @@
#include <config.h>
+#include <ctype.h>
#include <sys/stat.h>
#include "internal.h"
@@ -49,19 +50,17 @@
#define CHAINPREFIX_HOST_OUT_TEMP 'P'
+#define STOPONERROR_STR "STOPONERROR"
+
#define CMD_SEPARATOR "\n"
-#define CMD_DEF_PRE "cmd=\""
-#define CMD_DEF_POST "\""
+#define CMD_DEF_PRE ""
+#define CMD_DEF_POST ""
#define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST
-#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR
+#define CMD_EXEC ""
#define CMD_STOPONERR(X) \
- X ? "if [ $? -ne 0 ]; then" \
- " echo \"Failure to execute command '${cmd}'.\";" \
- " exit 1;" \
- "fi" CMD_SEPARATOR \
+ X ? STOPONERROR_STR CMD_SEPARATOR \
: ""
-
#define EBTABLES_CMD EBTABLES_PATH
#define IPTABLES_CMD IPTABLES_PATH
#define BASH_CMD BASH_PATH
@@ -330,8 +329,7 @@ static int iptablesLinkIPTablesBaseChain
virBufferPtr buf,
const char *udchain,
const char *syschain,
- unsigned int pos,
- int stopOnError)
+ unsigned int pos)
{
virBufferVSprintf(buf,
"res=$("
@@ -342,13 +340,19 @@ static int iptablesLinkIPTablesBaseChain
"else\n"
" r=$(echo $res | " GAWK_CMD " '{print $1}')\n"
" if [ \"${r}\" != \"%d\" ]; then\n"
- " " CMD_DEF(IPTABLES_CMD " -I %s %d -j %s") CMD_SEPARATOR
- " " CMD_EXEC
- " %s"
+ " cmd=\"" IPTABLES_CMD " -I %s %d -j %s\"" CMD_SEPARATOR
+ " res=`${cmd}`" CMD_SEPARATOR
+ " if [ $? -ne 0 ]; then\n"
+ " echo \"Failure to execute command '${cmd}'.\";\n"
+ " exit 1;\n"
+ " fi\n"
" let r=r+1\n"
- " " CMD_DEF(IPTABLES_CMD " -D %s ${r}") CMD_SEPARATOR
- " " CMD_EXEC
- " %s"
+ " cmd=\"" IPTABLES_CMD " -D %s ${r}\"" CMD_SEPARATOR
+ " res=`${cmd}`" CMD_SEPARATOR
+ " if [ $? -ne 0 ]; then\n"
+ " echo \"Failure to execute command '${cmd}'.\";\n"
+ " exit 1;\n"
+ " fi\n"
" fi\n"
"fi\n",
@@ -359,10 +363,8 @@ static int iptablesLinkIPTablesBaseChain
pos,
syschain, pos, udchain,
- CMD_STOPONERR(stopOnError),
- syschain,
- CMD_STOPONERR(stopOnError));
+ syschain);
return 0;
}
@@ -375,13 +377,13 @@ static int iptablesCreateBaseChains(virC
IPTABLES_CMD " -N " VIRT_IN_POST_CHAIN CMD_SEPARATOR
IPTABLES_CMD " -N " HOST_IN_CHAIN CMD_SEPARATOR);
iptablesLinkIPTablesBaseChain(conn, buf,
- VIRT_IN_CHAIN , "FORWARD", 1, 1);
+ VIRT_IN_CHAIN , "FORWARD", 1);
iptablesLinkIPTablesBaseChain(conn, buf,
- VIRT_OUT_CHAIN , "FORWARD", 2, 1);
+ VIRT_OUT_CHAIN , "FORWARD", 2);
iptablesLinkIPTablesBaseChain(conn, buf,
- VIRT_IN_POST_CHAIN, "FORWARD", 3, 1);
+ VIRT_IN_POST_CHAIN, "FORWARD", 3);
iptablesLinkIPTablesBaseChain(conn, buf,
- HOST_IN_CHAIN , "INPUT" , 1, 1);
+ HOST_IN_CHAIN , "INPUT" , 1);
return 0;
}
@@ -558,16 +560,18 @@ iptablesSetupVirtInPost(virConnectPtr co
virBufferVSprintf(buf,
"res=$(" IPTABLES_CMD " -L " VIRT_IN_POST_CHAIN
" | grep \"\\%s %s\")\n"
- "if [ \"${res}\" == \"\" ]; then "
- CMD_DEF(IPTABLES_CMD
+ "if [ \"${res}\" == \"\" ]; then\n"
+ " cmd=\"" IPTABLES_CMD
" -A " VIRT_IN_POST_CHAIN
- " %s %s -j ACCEPT") CMD_SEPARATOR
- CMD_EXEC
- "%s"
+ " %s %s -j ACCEPT\"" CMD_SEPARATOR
+ " res=`${cmd}`" CMD_SEPARATOR
+ " if [ $? -ne 0 ]; then\n"
+ " echo \"Failure to execute command '${cmd}'.\";\n"
+ " exit 1;\n"
+ " fi\n"
"fi\n",
PHYSDEV_IN, ifname,
- match, ifname,
- CMD_STOPONERR(1));
+ match, ifname);
return 0;
}
@@ -1959,6 +1963,110 @@ err_exit:
}
+static int
+ebiptablesExecScript(virConnectPtr conn,
+ virBufferPtr buf,
+ int *status)
+{
+ int rc = 0;
+ char *cmds;
+ char *filename;
+ const char *argv[] = {NULL, NULL};
+
+ if (virBufferError(buf)) {
+ virReportOOMError();
+ virBufferFreeAndReset(buf);
+ return 1;
+ }
+
+ *status = 0;
+
+ cmds = virBufferContentAndReset(buf);
+
+ VIR_DEBUG("%s", cmds);
+
+ if (!cmds)
+ return 0;
+
+ filename = ebiptablesWriteToTempFile(conn, cmds);
+ VIR_FREE(cmds);
+
+ if (!filename)
+ return 1;
+
+ argv[0] = filename;
+ rc = virRun(argv, status);
+
+ *status >>= 8;
+
+ VIR_DEBUG("rc = %d, status = %d\n",rc, *status);
+
+ unlink(filename);
+
+ VIR_FREE(filename);
+
+ return rc;
+}
+
+
+static const char **
+virStringToArgv(const char *line) {
+ int i,c = 0;
+ const char *next = line, *beg;
+ const char **ret;
+
+ while (1) {
+ while (isspace(*next))
+ next++;
+ if (*next == 0)
+ break;
+ while (*next && !isspace(*next))
+ next++;
+ c++;
+ }
+
+ c++;
+
+ if (VIR_ALLOC_N(ret, c) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+ c = 0;
+
+ next = line;
+ while (1) {
+ while (isspace(*next))
+ next++;
+ if (*next == 0)
+ break;
+ beg = next;
+ while (*next && !isspace(*next))
+ next++;
+ ret[c] = strndup(beg, next-beg);
+ if (!ret[c])
+ goto err_exit;
+ c++;
+ }
+ return ret;
+
+ err_exit:
+ for (i = 0; i < c; i++)
+ VIR_FREE(ret[c]);
+ VIR_FREE(ret);
+ return NULL;
+}
+
+
+static void
+virFreeStrArray(const char ** arr) {
+ int i = 0;
+ while (arr[i])
+ VIR_FREE(arr[i++]);
+ VIR_FREE(arr);
+}
+
+
/**
* ebiptablesExecCLI:
* @conn : pointer to virConnect object
@@ -1975,14 +2083,16 @@ err_exit:
* script and return the status of the execution.
*/
static int
-ebiptablesExecCLI(virConnectPtr conn,
+ebiptablesExecCLI(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferPtr buf,
int *status)
{
+ int rc = 0;
char *cmds;
- char *filename;
- int rc;
- const char *argv[] = {NULL, NULL};
+ char *cmd, *next;
+ const char **argv;
+ int cmd_sep_strlen = strlen(CMD_SEPARATOR);
+ int stoponerr_strlen = strlen(STOPONERROR_STR);
if (virBufferError(buf)) {
virReportOOMError();
@@ -1999,24 +2109,47 @@ ebiptablesExecCLI(virConnectPtr conn,
if (!cmds)
return 0;
- filename = ebiptablesWriteToTempFile(conn, cmds);
- VIR_FREE(cmds);
-
- if (!filename)
- return 1;
+ cmd = cmds;
+ while (cmd) {
+ next = strstr(cmd, CMD_SEPARATOR);
+ if (next) {
+ *next = 0;
+ next += cmd_sep_strlen;
+ }
+ argv = virStringToArgv(cmd);
- argv[0] = filename;
- rc = virRun(argv, status);
+ if (!argv)
+ return 1;
- *status >>= 8;
+ rc = virRun(argv, status);
- VIR_DEBUG("rc = %d, status = %d\n",rc, *status);
+ virFreeStrArray(argv);
+ if (rc)
+ return rc;
- unlink(filename);
+ VIR_DEBUG("cmd=%s rc=%d status=%d\n",cmd, rc, *status >> 8);
- VIR_FREE(filename);
+ while (isspace(*next))
+ next++;
- return rc;
+ if (STREQLEN(next, STOPONERROR_STR, stoponerr_strlen)) {
+ if (*status) {
+ *status >>= 8;
+ return rc;
+ }
+ next = strstr(next, CMD_SEPARATOR);
+ if (next)
+ next += cmd_sep_strlen;
+ }
+ if (next) {
+ while (isspace(*next))
+ next++;
+ }
+ if (!next || *next == 0)
+ break;
+ cmd = next;
+ }
+ return 0;
}
@@ -2454,9 +2587,13 @@ ebiptablesApplyNewRules(virConnectPtr co
iptablesUnlinkTmpRootChains(conn, &buf, ifname);
iptablesRemoveTmpRootChains(conn, &buf, ifname);
+ if (ebiptablesExecCLI(conn, &buf, &cli_status))
+ goto tear_down_tmpebchains;
+
iptablesCreateBaseChains(conn, &buf);
- if (ebiptablesExecCLI(conn, &buf, &cli_status) || cli_status != 0)
+ if (ebiptablesExecScript(conn, &buf, &cli_status) ||
+ cli_status != 0)
goto tear_down_tmpebchains;
iptablesCreateTmpRootChains(conn, &buf, ifname);
@@ -2465,10 +2602,15 @@ ebiptablesApplyNewRules(virConnectPtr co
goto tear_down_tmpiptchains;
iptablesLinkTmpRootChains(conn, &buf, ifname);
- iptablesSetupVirtInPost(conn, &buf, ifname);
+
if (ebiptablesExecCLI(conn, &buf, &cli_status) || cli_status != 0)
goto tear_down_tmpiptchains;
+ iptablesSetupVirtInPost(conn, &buf, ifname);
+
+ if (ebiptablesExecScript(conn, &buf, &cli_status) || cli_status != 0)
+ goto tear_down_tmpiptchains;
+
for (i = 0; i < nruleInstances; i++) {
if (inst[i]->ruleType == RT_IPTABLES)
iptablesInstCommand(conn, &buf,
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list