[libvirt] [PATCH 1/3] Only check for IP forwarding, do not enable it

Enabling IP forwarding without the administrator's consent is bad. Only check for forwarding, do not enable it. --- src/network/bridge_driver.c | 56 +++++++++++++++++++++++++++++++++---------- 1 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e1846ee..e3e8dc2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1853,19 +1853,51 @@ networkReloadIptablesRules(struct network_driver *driver) } } -/* Enable IP Forwarding. Return 0 for success, -1 for failure. */ +#define SYSCTL_PATH "/proc/sys" + +/* Check for IP Forwarding. Return 0 if required family is enabled, -1 for failure. */ static int -networkEnableIpForwarding(bool enableIPv4, bool enableIPv6) +networkCheckIpForwarding(bool checkIPv4, bool checkIPv6) { int ret = 0; - if (enableIPv4) - ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n", 0); - if (enableIPv6 && ret == 0) - ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding", "1\n", 0); - return ret; -} + char *buf = NULL; -#define SYSCTL_PATH "/proc/sys" + if (checkIPv4) { + ret = virFileReadAll(SYSCTL_PATH "/net/ipv4/ip_forward", 2, &buf); + if (ret != 2) + goto error; + if (STRNEQ(buf, "1\n")) { + networkReportError(VIR_ERR_SYSTEM_ERROR, + _("IP forwarding is not enabled on your system")); + goto cleanup; + } + } + if (checkIPv6) { + if (ret > 0) + VIR_FREE(buf); + ret = virFileReadAll(SYSCTL_PATH "/net/ipv6/conf/all/forwarding", 2, &buf); + if (ret != 2) + goto error; + if (STRNEQ(buf, "1\n")) { + networkReportError(VIR_ERR_SYSTEM_ERROR, + _("IPv6 forwarding is not enabled on your system")); + goto cleanup; + } + } + + VIR_FREE(buf); + return 0; + +error: + virReportSystemError(errno, "%s", _("cannot check for IP forwarding")); +cleanup: + if (ret > 0) { + VIR_FREE(buf); + return -1; + } else { + return ret; + } +} static int networkSetIPv6Sysctls(virNetworkObjPtr network) @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver, if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2; - /* If forwardType != NONE, turn on global IP forwarding */ + /* If forwardType != NONE, check for IP forwarding */ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && - networkEnableIpForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); + networkCheckIpForwarding(v4present, v6present) < 0) { goto err3; } -- 1.7.2.5

On 15.10.2012 12:26, Benjamin Cama wrote:
Enabling IP forwarding without the administrator's consent is bad. Only check for forwarding, do not enable it. --- src/network/bridge_driver.c | 56 +++++++++++++++++++++++++++++++++---------- 1 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e1846ee..e3e8dc2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1853,19 +1853,51 @@ networkReloadIptablesRules(struct network_driver *driver) } }
-/* Enable IP Forwarding. Return 0 for success, -1 for failure. */ +#define SYSCTL_PATH "/proc/sys" + +/* Check for IP Forwarding. Return 0 if required family is enabled, -1 for failure. */ static int -networkEnableIpForwarding(bool enableIPv4, bool enableIPv6) +networkCheckIpForwarding(bool checkIPv4, bool checkIPv6) { int ret = 0; - if (enableIPv4) - ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n", 0); - if (enableIPv6 && ret == 0) - ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding", "1\n", 0); - return ret; -} + char *buf = NULL;
-#define SYSCTL_PATH "/proc/sys" + if (checkIPv4) { + ret = virFileReadAll(SYSCTL_PATH "/net/ipv4/ip_forward", 2, &buf); + if (ret != 2) + goto error; + if (STRNEQ(buf, "1\n")) { + networkReportError(VIR_ERR_SYSTEM_ERROR, + _("IP forwarding is not enabled on your system"));
We dropped networkReportError and use virReportError instead. Moreover, you miss "%s" there: virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("blah"));
+ goto cleanup; + } + } + if (checkIPv6) { + if (ret > 0)
This check is useless since VIR_FREE(NULL) is safe.
+ VIR_FREE(buf); + ret = virFileReadAll(SYSCTL_PATH "/net/ipv6/conf/all/forwarding", 2, &buf); + if (ret != 2) + goto error; + if (STRNEQ(buf, "1\n")) { + networkReportError(VIR_ERR_SYSTEM_ERROR, + _("IPv6 forwarding is not enabled on your system")); + goto cleanup; + } + } + + VIR_FREE(buf); + return 0; + +error: + virReportSystemError(errno, "%s", _("cannot check for IP forwarding")); +cleanup: + if (ret > 0) { + VIR_FREE(buf); + return -1; + } else { + return ret; + } +}
We rather stick with flow: static int func(...) { int ret = -1; ... <commands> ... if (cond) goto cleanup; ... <commands> ... ret = 0; cleanup: return ret; }
static int networkSetIPv6Sysctls(virNetworkObjPtr network) @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver, if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2;
- /* If forwardType != NONE, turn on global IP forwarding */ + /* If forwardType != NONE, check for IP forwarding */ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && - networkEnableIpForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); + networkCheckIpForwarding(v4present, v6present) < 0) { goto err3; }
Well, I am not sure if we can do this. What would happen if some of our users rely on this already? I mean, it's there since ages. Michal

On 10/15/2012 10:54 AM, Michal Privoznik wrote:
On 15.10.2012 12:26, Benjamin Cama wrote:
static int networkSetIPv6Sysctls(virNetworkObjPtr network) @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver, if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2;
- /* If forwardType != NONE, turn on global IP forwarding */ + /* If forwardType != NONE, check for IP forwarding */ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && - networkEnableIpForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); + networkCheckIpForwarding(v4present, v6present) < 0) { goto err3; }
Well, I am not sure if we can do this. What would happen if some of our users rely on this already? I mean, it's there since ages.
Michal
Indeed this kinda defeats the purpose of the default virtual network that should 'just work' out of the box. Maybe we could add some libvirtd.conf option to enable this check-if-set behavior, but we can't change the default here. - Cole

On 10/15/2012 12:04 PM, Cole Robinson wrote:
On 15.10.2012 12:26, Benjamin Cama wrote:
static int networkSetIPv6Sysctls(virNetworkObjPtr network) @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver, if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2;
- /* If forwardType != NONE, turn on global IP forwarding */ + /* If forwardType != NONE, check for IP forwarding */ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && - networkEnableIpForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); + networkCheckIpForwarding(v4present, v6present) < 0) { goto err3; }
Well, I am not sure if we can do this. What would happen if some of our users rely on this already? I mean, it's there since ages.
Michal Indeed this kinda defeats the purpose of the default virtual network that should 'just work' out of the box. Maybe we could add some libvirtd.conf
On 10/15/2012 10:54 AM, Michal Privoznik wrote: option to enable this check-if-set behavior, but we can't change the default here.
We've had this discussion before: http://www.redhat.com/archives/libvir-list/2010-October/msg00030.html and in particular this response: http://www.redhat.com/archives/libvir-list/2010-October/msg00183.html In the end, the presence of a network with a forward mode that requires L3 packet forwarding indicates tacit approval for ip_forward to be turned on. The problem in the past has been that the default network (which has <forward mode='nat'>) was a part of *all* libvirt installs. That is now separated into its own sub-package, though. So, the "config option" is to simply not install the default network (or to remove it if it's there).

Hi everyone, Le lundi 15 octobre 2012 à 09:36 -0700, Cole Robinson a écrit :
On 10/15/2012 12:04 PM, Cole Robinson wrote:
On 15.10.2012 12:26, Benjamin Cama wrote:
static int networkSetIPv6Sysctls(virNetworkObjPtr network) @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver, if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2;
- /* If forwardType != NONE, turn on global IP forwarding */ + /* If forwardType != NONE, check for IP forwarding */ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && - networkEnableIpForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); + networkCheckIpForwarding(v4present, v6present) < 0) { goto err3; }
Well, I am not sure if we can do this. What would happen if some of our users rely on this already? I mean, it's there since ages.
Michal Indeed this kinda defeats the purpose of the default virtual network that should 'just work' out of the box. Maybe we could add some libvirtd.conf
On 10/15/2012 10:54 AM, Michal Privoznik wrote: option to enable this check-if-set behavior, but we can't change the default here.
We've had this discussion before:
http://www.redhat.com/archives/libvir-list/2010-October/msg00030.html
and in particular this response:
http://www.redhat.com/archives/libvir-list/2010-October/msg00183.html
Thanks for the links.
In the end, the presence of a network with a forward mode that requires L3 packet forwarding indicates tacit approval for ip_forward to be turned on. The problem in the past has been that the default network (which has <forward mode='nat'>) was a part of *all* libvirt installs. That is now separated into its own sub-package, though.
So, the "config option" is to simply not install the default network (or to remove it if it's there).
I understand that changing the behavior of a function that has been “just working” for years sounds unacceptable. It's just that for IPv6, enabling forwarding has far more consequences than for IPv4. But I understand that my use case may be rare enough not to change the default behavior. Still, I would like to implement some big warning when changing the forwarding state. I will work on that. Regards, -- Benjamin Cama <benjamin.cama@telecom-bretagne.eu>
participants (4)
-
Benjamin Cama
-
Cole Robinson
-
Laine Stump
-
Michal Privoznik