Re: [CFR] unified rc.firewall

On Monday 23 November 2009 10:13:54 am Hajimu UMEMOTO wrote:
> Hi,
> 
> >>>>> On Sun, 22 Nov 2009 11:12:33 -0800
> >>>>> Doug Barton <dougb_at_FreeBSD.org> said:
> 
> dougb> In rc.firewall you seem to have copied afexists() from network.subr.
> dougb> Is there a reason that you did not simply source that file? That 
would
> dougb> be the preferred method. Also in that file you call "if afexists
> dougb> inet6" quite a few times. My preference from a performance standpoint
> dougb> would be to call it once, perhaps in a start_precmd then cache the 
value.
> 
> Thank you for the comments.
> Ah, yes, afexists() is only in 9-CURRENT, and is not MFC'ed into 8,
> yet.  So, I thought the patch should be able to work on both 9 and 8,
> for review.  I've changed to source network.subr for afexists().
> Calling afexists() several times was not good idea.  So, I've changed
> to call afexists() just once.
> The new patch is attached.
> 
> dougb> And of course, you have regression tested this thoroughly, yes? :)
> dougb> Please include scenarios where there is no INET6 in the kernel as 
well.
> 
> Okay, I've tested it on INET6-less kernel, as well.

Some comments I have:

_at__at_ -178,6 +212,16 _at__at_
        # Allow any traffic to or from my own net.
        ${fwcmd} add pass all from me to ${net}
        ${fwcmd} add pass all from ${net} to me
+       if [ -n "$net6" ]; then
+               ${fwcmd} add pass ip6 from me6 to ${net6}
+               ${fwcmd} add pass ip6 from ${net6} to me6
+       fi
+
+       if [ -n "$net6" ]; then
+               # Allow any link-local multicast traffic
+               ${fwcmd} add pass ip6 from fe80::/10 to ff02::/16
+               ${fwcmd} add pass ip6 from ${net6} to ff02::/16
+       fi

Any reason to not use 'all' here rather than 'ip6' to match the earlier IPv4
rules?

_at__at_ -273,6 +329,55 _at__at_
        ${fwcmd} add deny all from 224.0.0.0/4 to any via ${oif}
        ${fwcmd} add deny all from 240.0.0.0/4 to any via ${oif}
 
+       if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
+               # Stop unique local unicast address on the outside interface
+               ${fwcmd} add deny ip6 from fc00::/7 to any via ${oif6}
+               ${fwcmd} add deny ip6 from any to fc00::/7 via ${oif6}
+
....

Similarly here, why not use 'all' instead of 'ip6'?

_at__at_ -291,7 +396,11 _at__at_
        ${fwcmd} add pass tcp from any to me 80 setup
 
        # Reject&Log all setup of incoming connections from the outside
-       ${fwcmd} add deny log tcp from any to any in via ${oif} setup
+       ${fwcmd} add deny log ip4 from any to any in via ${oif} setup proto 
tcp
+       if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
+               ${fwcmd} add deny log ip6 from any to any in via ${oif6} \
+                   setup proto tcp
+       fi

I would actually not use separate v6 interfaces for the 'simple' firewall
but just have 'oif', 'onet', and 'onet_ipv6' variables.  Then you don't need
this diff at all as the existing rule will work fine.

        # For services permitted below.
        ${fwcmd} add pass tcp  from me to any established
+       if [ $ipv6_available -eq 0 ]; then
+               ${fwcmd} add pass ip6 from any to any proto tcp established
+       fi

I think this extra rule here isn't needed at all as the first rule should
already match all of those packets.


        # Allow any connection out, adding state for each.
        ${fwcmd} add pass tcp  from me to any setup keep-state
        ${fwcmd} add pass udp  from me to any       keep-state
        ${fwcmd} add pass icmp from me to any       keep-state
+       if [ $ipv6_available -eq 0 ]; then
+               ${fwcmd} add pass ip6 from me6 to any proto tcp setup
+               ${fwcmd} add pass ip6 from me6 to any proto udp keep-state
+               ${fwcmd} add pass ip6 from me6 to any proto ipv6-icmp \
+                   keep-state
+       fi

I think it is more consistent to use 'pass tcp from me6 to any' similar to
the IPv4 rules here.  It is also shorter and easier to read that way IMO.

-- 
John Baldwin