[libvirt] [PATCH v2] nwfilter: probe for inverted ctdir
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- v1->v2: - using virSocketAddrParseIPv4 --- src/nwfilter/nwfilter_ebiptables_driver.c | 121 ++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) 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 @@ -27,6 +27,10 @@ #include <string.h> #include <sys/stat.h> #include <fcntl.h> +#include <arpa/inet.h> +#include <sys/select.h> +#include <sys/time.h> +#include <unistd.h> #include "internal.h" @@ -85,6 +89,12 @@ static char *iptables_cmd_path; static char *ip6tables_cmd_path; static char *grep_cmd_path; +/* + * --ctdir original vs. --ctdir reply's meaning was inverted in netfilter + * at some point. We probe for it. + */ +static bool iptables_ctdir_corrected = false; + #define PRINT_ROOT_CHAIN(buf, prefix, ifname) \ snprintf(buf, sizeof(buf), "libvirt-%c-%s", prefix, ifname) #define PRINT_CHAIN(buf, prefix, ifname, suffix) \ @@ -1262,6 +1272,9 @@ iptablesEnforceDirection(int directionIn virNWFilterRuleDefPtr rule, virBufferPtr buf) { + if (iptables_ctdir_corrected) + directionIn = !directionIn; + if (rule->tt != VIR_NWFILTER_RULE_DIRECTION_INOUT) virBufferAsprintf(buf, " -m conntrack --ctdir %s", (directionIn) ? "Original" @@ -4304,6 +4317,111 @@ ebiptablesDriverTestCLITools(void) return ret; } +static void +ebiptablesDriverProbeCtdir(void) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + static const char cmdline[] = + "$IPT -%c INPUT %c -i lo -p udp --dport %hu " + "-m state --state ESTABLISHED -j ACCEPT " CMD_SEPARATOR + "$IPT -%c INPUT %c -i lo -p udp --dport %hu " + "-m conntrack --ctdir original -j ACCEPT " CMD_SEPARATOR + "$IPT -%c INPUT %c -i lo -p udp --dport %hu -j DROP"; + /* + * Above '--ctdir original' gets this test to receive a message on + * 'fixed' netfilter. + */ + unsigned short port; + int ssockfd = -1, csockfd = -1; + virSocketAddr saddr; + struct sockaddr_in *serveraddr = &saddr.data.inet4; + fd_set readfds; + struct timeval timeout = { + .tv_sec = 0, + .tv_usec = 1000 * 200, + }; + int n; + + if (virSocketAddrParseIPv4(&saddr, "127.0.0.1") < 0) { + VIR_ERROR(_("Could not parse IP address")); + goto cleanup; + } + + if ((ssockfd = socket(AF_INET, SOCK_DGRAM, 0)) < 0 || + (csockfd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { + VIR_ERROR(_("Could not open UDP socket")); + goto cleanup; + } + + for (port = 0xffff; port > 1024; port--) { + serveraddr->sin_port = htons(port); + if (bind(ssockfd, (struct sockaddr *)serveraddr, + sizeof(*serveraddr)) == 0) + break; + } + if (port == 1024) { + VIR_ERROR(_("Could not bind to any UDP socket")); + goto cleanup; + } + + NWFILTER_SET_IPTABLES_SHELLVAR(&buf); + virBufferAsprintf(&buf, cmdline, + 'I', '1', port, + 'I', '2', port, + 'I', '3', port); + + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + + if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) { + VIR_ERROR(_("Could not apply iptables rules")); + goto cleanup_iptables; + } + + if (sendto(csockfd, cmdline, 1, 0, (struct sockaddr *)serveraddr, + sizeof(*serveraddr)) < 0) { + VIR_ERROR(_("Could not send to UDP socket")); + goto cleanup_iptables; + } + + FD_ZERO(&readfds); + FD_SET(ssockfd, &readfds); + + while (true) { + n = select(ssockfd + 1, &readfds, NULL, NULL, &timeout); + if (n < 0) { + if (errno == EINTR) + continue; + VIR_ERROR(_("Select failed")); + goto cleanup_iptables; + } + if (n == 0) { + VIR_INFO("Ctdir probing received no data -- 'old' netfilter"); + goto cleanup_iptables; + } + VIR_INFO("Ctdir probing received data -- 'fixed' netfilter"); + iptables_ctdir_corrected = true; + break; + } + +cleanup_iptables: + virBufferFreeAndReset(&buf); + + NWFILTER_SET_IPTABLES_SHELLVAR(&buf); + virBufferAsprintf(&buf, cmdline, + 'D', ' ', port, + 'D', ' ', port, + 'D', ' ', port); + ebiptablesExecCLI(&buf, NULL, NULL); + +cleanup: + virBufferFreeAndReset(&buf); + VIR_FORCE_CLOSE(ssockfd); + VIR_FORCE_CLOSE(csockfd); +} + static int ebiptablesDriverInit(bool privileged) { @@ -4341,6 +4459,9 @@ ebiptablesDriverInit(bool privileged) return -ENOTSUP; } + if (iptables_cmd_path) + ebiptablesDriverProbeCtdir(); + ebiptables_driver.flags = TECHDRV_FLAG_INITIALIZED; return 0;
On Fri, Mar 22, 2013 at 08:26:59AM -0400, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected.
I think this is really very hackish. If this test capability goes wrong for any reason, then we're going to silently setting up incorrect rules, which would be a security flaw. I think we need a more robust detection system for this. 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 03/22/2013 01:49 PM, Daniel P. Berrange wrote:
On Fri, Mar 22, 2013 at 08:26:59AM -0400, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected. I think this is really very hackish. If this test capability goes wrong for any reason, then we're going to silently setting up incorrect rules, which would be a security flaw. I think we need a more robust detection system for this.
What method do you suggest? We cannot look in the kernel afaik so we have to probe it. We have to setup some rules to make this work and the tighter the rules are the better, so they don't interfer with anything else. Then of course we need to send 'something' so we can detect whether this is an old or a new system. We could of course send once with the rules with '--ctdir ORIGINAL' and then do a 2nd test with '--ctdir REPLY' to make sure that it works at all. But what would we do if we detected that it doesn't work in either one of the cases? Terminate? The assumption of the code was that we have a working loopback device. Stefan
On Fri, Mar 22, 2013 at 01:58:50PM -0400, Stefan Berger wrote:
On 03/22/2013 01:49 PM, Daniel P. Berrange wrote:
On Fri, Mar 22, 2013 at 08:26:59AM -0400, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected. I think this is really very hackish. If this test capability goes wrong for any reason, then we're going to silently setting up incorrect rules, which would be a security flaw. I think we need a more robust detection system for this.
What method do you suggest? We cannot look in the kernel afaik so we have to probe it. We have to setup some rules to make this work and the tighter the rules are the better, so they don't interfer with anything else. Then of course we need to send 'something' so we can detect whether this is an old or a new system. We could of course send once with the rules with '--ctdir ORIGINAL' and then do a 2nd test with '--ctdir REPLY' to make sure that it works at all. But what would we do if we detected that it doesn't work in either one of the cases? Terminate? The assumption of the code was that we have a working loopback device.
When was the change in semantics made to iptables ? Is this considered a critical bug by iptables maintainers, if so then we could just use the correct syntax unconditionally, and let distro vendors fix their iptables binaries to work correctly, instead of trying to workaround the brokeness. 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 03/22/2013 02:04 PM, Daniel P. Berrange wrote:
On Fri, Mar 22, 2013 at 08:26:59AM -0400, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected. I think this is really very hackish. If this test capability goes wrong for any reason, then we're going to silently setting up incorrect rules, which would be a security flaw. I think we need a more robust detection system for this. What method do you suggest? We cannot look in the kernel afaik so we have to probe it. We have to setup some rules to make this work and
On 03/22/2013 01:49 PM, Daniel P. Berrange wrote: the tighter the rules are the better, so they don't interfer with anything else. Then of course we need to send 'something' so we can detect whether this is an old or a new system. We could of course send once with the rules with '--ctdir ORIGINAL' and then do a 2nd test with '--ctdir REPLY' to make sure that it works at all. But what would we do if we detected that it doesn't work in either one of the cases? Terminate? The assumption of the code was that we have a working loopback device. When was the change in semantics made to iptables ? Is this considered a critical bug by iptables maintainers, if so then we could just use
On Fri, Mar 22, 2013 at 01:58:50PM -0400, Stefan Berger wrote: the correct syntax unconditionally, and let distro vendors fix their iptables binaries to work correctly, instead of trying to workaround the brokeness.
The problem is libvirt can be running on any kernel with any mix of iptables tools on top of that kernel. It was fixed in the kernel a while ago actually and the tools are unaffected by this just that the interpretation of the '--ctdir ORIGINAL' parameter in the kernel now is reverted and before the change it was '--ctdir REPLY' that setup that same rule in the kernel. Admins using this -m conntrack option are going to love it. Install a new kernel, reboot and your previously stored filtering rules aren't working anymore. Laine pointed me to the thread: https://www.redhat.com/archives/libvirt-users/2013-March/msg00109.html A simple '!' causing headaches.
On 03/22/2013 08:26 AM, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instead and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected.
While this is an admirable piece of work :-), I'm concerned that it may 1) be fragile, and 2) assume too much about the system being probed, and end up giving incorrect results in some circumstances. But since we have the check in place, we would be lulled into believing that we always correctly know which version of --ctdir we're working with, and end up with a non-working system and no clear indication why. It's very distressing that so little thought was apparently put into the far-reaching effects of making such an ABI change to netfilter; in my mind it really does render --ctdir more or less unusable except for very controlled cases where the same people are maintaining both netfilter/kernel and libvirt for a particular release of a particular distro. I unfortunately also don't have any alternative to offer, other than "just don't use it" (although this message Pablo from netfilter says that can be done with no reduction in security): https://www.redhat.com/archives/libvirt-users/2013-March/msg00128.html
On 03/22/2013 02:29 PM, Laine Stump wrote:
> On 03/22/2013 08:26 AM, Stefan Berger wrote:
>> Linux netfilter at some point inverted the meaning of the '--ctdir reply'
>> and newer netfilter implementations now expect '--ctdir original'
>> instead and vice-versa.
>> We probe for this netfilter change via a UDP message over loopback and 3
>> filtering rules applied to INPUT. If the sent byte arrives, the newer
>> netfilter implementation has been detected.
> While this is an admirable piece of work :-), I'm concerned that it may
> 1) be fragile, and 2) assume too much about the system being probed, and
> end up giving incorrect results in some circumstances. But since we have
> the check in place, we would be lulled into believing that we always
> correctly know which version of --ctdir we're working with, and end up
> with a non-working system and no clear indication why.
So is the consensus now that it cannot be probed for in all cases by
libvirt? What alternative do you suggest? Removal of --ctdir usage even
if it was there for a reason?
Stefan
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Stefan Berger