[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3] nwfilter: probe for inverted ctdir

On Thu, Mar 28, 2013 at 10:54:01AM -0400, Stefan Berger wrote:
> On 03/28/2013 10:36 AM, Laine Stump wrote:
> >For reference of people new to this thread, here is the start of the thread:
> >
> >   https://www.redhat.com/archives/libvir-list/2013-March/msg01403.html
> >
> >This concerns changes to libvirt to cope with the newly discovered (by
> >us :-) difference in interpretation of ctdir by different versions of
> >netfilter.
> >
> >On 03/28/2013 07:11 AM, Stefan Berger wrote:
> >>On 03/27/2013 09:09 PM, Stefan Berger wrote:
> >>>On 03/27/2013 02:01 PM, Eric Blake wrote:
> >>>>On 03/27/2013 10:30 AM, Laine Stump wrote:
> >>>>>My opinion is that the patch we should apply should be a simple patch
> >>>>>that just removes use of --ctdir. According to the netfilter developer
> >>>>>who responded to the thread on libvirt-users, it doesn't add any extra
> >>>>>security not already provided by conntrack:
> >>>>>
> >>>>>https://www.redhat.com/archives/libvirt-users/2013-March/msg00121.html
> >>>>>https://www.redhat.com/archives/libvirt-users/2013-March/msg00128.html
> >>>>>
> >>>>>Not being an expert on netfilter internals, I can't dispute his claim.
> >>>>>
> >>>>>Does anyone else have an opinion?
> >>>>What filters specifically caused the use of --ctdir, and are they
> >>>>broken
> >>>>if we omit the use of --ctdir?
> >>>It depends on how you write the filters that the --ctdir is being used.
> >>>
> >>>iirc: The effect of the --ctdir usage is that if one has an incoming
> >>>rule and and outgoing rule with the same IP address on the 'other'
> >>>side the check for an ESTABLISHED state is not enough to ACCEPT the
> >>>traffic, if one was to remove one of the rules while communication in
> >>>both directions was occurring and an immediate cut of the traffic in
> >>>one way was expected. The effect so far was that if the rule for the
> >>>incoming rule was removed it would cut the incoming traffic
> >>>immediately while the traffic in outgoing direction was
> >>>uninterrupted. I think that if we remove this now the traffic in both
> >>>directions will continue. I will verify tomorrow.
> >>Verified. I have a ping running from the VM to destination 'A' and
> >>from 'A' to the VM. The --ctdir enforces the direction of the traffic
> >>and if one of the following rules is removed, the ping is immediately
> >>cut.
> >>
> >>   <rule action='accept' direction='out' priority='500'>
> >>     <icmp/>
> >>   </rule>
> >>   <rule action='accept' direction='in' priority='500'>
> >>     <icmp/>
> >>   </rule>
> >>
> >>The ping is not cut anymore upon removal of one of the above rules if
> >>--ctdir was to be removed entirely.
> >Okay, as I understand from your description, the difference is that when
> >a ping in one direction is already in action, and you remove the rule
> >allowing that ping, that existing ping "session" will continue to be
> >allowed *if* there is still a rule allowing pings in the other
> >direction. Is that correct? I'm guessing that *new* attempts to ping in
> >that direction will no longer be allowed though, is that also correct?
> >
> >For the benefit of Pablo and the other netfilter developers, can you
> >paste the iptables commands that are generated for the two rules above?
> >Possibly they can suggest alternative rules that have the desired effect.
> First off, there are multiple ways one can write the filtering rules
> in nwfilter, either stateless or stateful:
> http://libvirt.org/formatnwfilter.html#nwfwrite
> Thus the filter here is only one example how one can write a
> stateful filter for traffic from/to a VM:
> <filter name='ctdirtest' chain='ipv4' priority='-700'>
> <uuid>582c2fe6-569a-f366-58fb-f995f1a559ce</uuid>
>   <rule action='accept' direction='out' priority='500'>
>     <icmp/>
>   </rule>
>   <rule action='accept' direction='in' priority='500'>
>     <icmp/>
>   </rule>
>   <rule action='drop' direction='inout' priority='500'>
>     <all/>
>   </rule>
> </filter>
> The filter above creates the following types of rules -- some rules
> are omitted that goto into these user-defined rules.
> Chain FI-vnet0 (1 references)
>  pkts bytes target     prot opt in     out source
> destination
>     6   504 RETURN     icmp --  *      *
>            state NEW,ESTABLISHED ctdir ORIGINAL
>     0     0 RETURN     icmp --  *      *
>            state ESTABLISHED ctdir REPLY
>     0     0 DROP       all  --  *      *  

Conntrack is already internally validating that directions are correct
for you, so no need for those --ctdir. Let me explain why:

If conntrack gets an ICMP echo reply entering through the NEW state,
it will consider it invalid since it is not coming as reply to an ICMP
echo request.

[ In case you want to have a look at the internal implementation.
  See linux/net/ipv4/netfilter/nf_conntrack_proto_icmp.c, function
  icmp_new(), the valid ICMP message that can enter through the NEW
  state are:

        static const u_int8_t valid_new[] = {                                    
                [ICMP_ECHO] = 1,                                                 
                [ICMP_TIMESTAMP] = 1,                                            
                [ICMP_INFO_REQUEST] = 1,                                         
                [ICMP_ADDRESS] = 1                                               


So using:            state NEW,ESTABLISHED

is just fine if you drop INVALID traffic in your rule-set:

iptables -A F0-vnet0 -m state INVALID -j LOG --log-prefix "invalid: "
iptables -A F0-vnet0 -m state INVALID -j DROP

Which are the common rules that you add in a sane stateful rule-set
(the log line could be removed if you want to discard traffic

Same thing for TCP traffic. The connection tracking table already
validates in its internal state machine that the correct state
transitions are actually happening. That validation includes that
packets are coming in the good direction.

In sum: The --ctdir is not providing more security. We did not have it
originally in the `state' match, it was a late extension to the
conntrack match.

My advice here: Just rely on conntrack states and drop invalid
traffic, it will do the direction validation that you're trying to
achieve with that rule-set.

Hope that it helps.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]