On 01/06/2013 07:10 AM, Mr Dash Four wrote: > > 4. Wrong "rules" optimisation with regards to "--cstate RELATED" (the > "local" zone in the example given below relates to the "lo" loopback > interface): > > 4.1 > > rules > ~~~~~ > SECTION RELATED > [...] > # local > ACCEPT:info $FW local > ACCEPT:info local $FW > [...] > # all > ACCEPT all all icmp destination-unreachable > > shorewall.conf > ~~~~~~~~~~~~~~ > RELATED_DISPOSITION=A_DROP > > Produces the following set of rules after optimisation (set at 31): > > firewall > ~~~~~~~~ > :~log0 - [0:0] > :~log1 - [0:0] > [...] > -A fw2local -m conntrack --ctstate RELATED -g ~log0 > ==>-A fw2local -p 1 --icmp-type 3 -m conntrack --ctstate RELATED -j ACCEPT > ==>-A fw2local -m conntrack --ctstate RELATED -g A_DROP > [...] > -A local2fw -m conntrack --ctstate RELATED -g ~log1 > ==>-A local2fw -p 1 --icmp-type 3 -m conntrack --ctstate RELATED -j ACCEPT > ==>-A local2fw -m conntrack --ctstate RELATED -g A_DROP > [...] > -A ~log0 -j LOG --log-level 6 --log-prefix "Shorewall:fw2local:ACCEPT:" > -A ~log0 -j ACCEPT > -A ~log1 -j LOG --log-level 6 --log-prefix "Shorewall:local2fw:ACCEPT:" > -A ~log1 -j ACCEPT > > The 4 statements marked with '==>' above will NEVER get executed.
This isn't wrong optimization -- it is just that the optimizer isn't smart enough to realize that the following statements cannot be executed. Added to my wish list. > Besides, I don't know why "-g" has been used instead of "-j" (a simple > jump). There is no need for Netfilter to push the return information onto the jump stack since the ~log chain will never return. So '-g' is used if supported. > > 4.2 > > rules > ~~~~~ > SECTION RELATED > > # local > ACCEPT $FW local icmp destination-unreachable > LOG:info(uid,tcp_options,ip_options,macdecode,tcp_sequence) $FW local > ACCEPT:NFLOG(1,0,1) $FW local > ACCEPT local $FW icmp destination-unreachable > LOG:info(uid,tcp_options,ip_options,macdecode,tcp_sequence) local $FW > ACCEPT:NFLOG(1,0,1) local $FW > > translates to: > > -A fw2local -p 1 --icmp-type 3 -m conntrack --ctstate RELATED -j ACCEPT > -A fw2local -m conntrack --ctstate RELATED -j LOG --log-uid > --log-tcp-options --log-ip-options --log-macdecode --log-tcp-sequence > --log-level 6 --log-prefix "Shorewall:fw2local:LOG:" > -A fw2local -m conntrack --ctstate RELATED -g ~log0 > -A fw2local -m conntrack --ctstate RELATED -g A_DROP > [...] > -A local2fw -p 1 --icmp-type 3 -m conntrack --ctstate RELATED -j ACCEPT > -A local2fw -m conntrack --ctstate RELATED -j LOG --log-uid > --log-tcp-options --log-ip-options --log-macdecode --log-tcp-sequence > --log-level 6 --log-prefix "Shorewall:local2fw:LOG:" > -A local2fw -m conntrack --ctstate RELATED -g ~log1 > -A local2fw -m conntrack --ctstate RELATED -g A_DROP > [...] > -A ~log0 -j NFLOG --nflog-group 1 --nflog-range 0 --nflog-threshold 1 > --nflog-prefix "Shorewall:fw2local:ACCEPT:" > -A ~log0 -j ACCEPT > -A ~log1 -j NFLOG --nflog-group 1 --nflog-range 0 --nflog-threshold 1 > --nflog-prefix "Shorewall:local2fw:ACCEPT:" > -A ~log1 -j ACCEPT > > As evident, "--cstate RELATED" optimisation is non-existent! You're right. > All of the above statements for each group (fw2local and local2fw) could be > combined into a chain with a single "--cstate RELATED" match. The right thing to do here is to generate a separate chain as was done in the now-extinct BLACKLIST section. That results in only one STATE test. Also added to the wish list. > Not to mention that the "-g A_DROP" statements above will NEVER execute. > Same issue as noted earlier. > One other point with regards to optimisation, as far as "--cstate > RELATED" is concerned: > > 4.3 > > rules > ~~~~~ > SECTION RELATED > > ACCEPT net:+mickey-mouse $FW icmp destination-unreachable > LOG:info(uid,tcp_options,ip_options,macdecode,tcp_sequence) > net:+mickey-mouse $FW > ACCEPT:NFLOG(1,0,1) net:+mickey-mouse $FW > LOG:info(uid,tcp_options,ip_options,macdecode,tcp_sequence) net:+mm2 $FW > ACCEPT:NFLOG(1,0,1) net:+mm2 $FW > > The above produces the following: > > -A net2fw -p 1 --icmp-type 3 -m conntrack --ctstate RELATED -m set > --match-set mickey-mouse src -j ACCEPT > -A net2fw -m conntrack --ctstate RELATED -m set --match-set mickey-mouse > src -j LOG --log-uid --log-tcp-options --log-ip-options --log-macdecode > --log-tcp-sequence --log-level 6 --log-prefix "Shorewall:net2fw:LOG:" > -A net2fw -m conntrack --ctstate RELATED -m set --match-set mickey-mouse > src -g ~log10 > -A net2fw -m conntrack --ctstate RELATED -m set --match-set mm2 src -j > LOG --log-uid --log-tcp-options --log-ip-options --log-macdecode > --log-tcp-sequence --log-level 6 --log-prefix "Shorewall:net2fw:LOG:" > -A net2fw -m conntrack --ctstate RELATED -m set --match-set mm2 src -g > ~log10 > -A net2fw -m conntrack --ctstate RELATED -g A_DROP > [...] > -A ~log10 -j NFLOG --nflog-group 1 --nflog-range 0 --nflog-threshold 1 > --nflog-prefix "Shorewall:net2fw:ACCEPT:" > -A ~log10 -j ACCEPT > > That's 5 "--cstate RELATED" comparison matches too many. I guess the > situation would be the same if I have 10 such statements in my "RELATED" > section - I'll end up with 10 "--cstate RELATED" matches, which is > clearly undesirable. Same as above. > > 5. ?IF expression isn't documented properly - I had to guess what should > be included/not included (I am NOT a perl guru!). The expression "! ($1 > eq 'log')" was acceptable to shorewall, "! $1 eq 'log'", "! $1 == 'log'" > or "! ($1 == 'log')", however, wasn't. These things should be properly > explained. 'man perlintro' gives a nice explanation of Perl expressions. > > 6. Ambiguity using the alternative syntax: > > Apparently, "ACCEPT $FW net ; user:root,switch:fw2net_accept=0" is not > the equivalent of "ACCEPT $FW net ; user:root, switch:fw2net_accept=0" > (note the extra space after that comma) - the first statement gives an > error: "invalid user (switch)", the second one passes without problems > and generates the appropriate iptables statement. It would be nice if > this behavious is documented somewhere. The following isn't clear enough? (from http://www.shorewall.net/configuration_file_basics.htm#Pairs) The pairs *must be separated by white space*, but you can add a comma adjacent to the values for readability as in: ; proto=>udp, port=1024 > > 7. Allow @{XXXX} shorewall variables to be modified in custom actions - > why having control of the @{logtag} (and other such shorewall variables) > is important: > > policy > ~~~~~~ > $FW net DROP:IELOG(-,1,2) > > produces: > > -A fw2net -j NFLOG --nflog-group 1 --nflog-range 0 --nflog-threshold 1 > --nflog-prefix "Shorewall:fw2net:LOG:" -m comment --comment "IELOG" > -A fw2net -j NFLOG --nflog-group 2 --nflog-range 0 --nflog-threshold 1 > --nflog-prefix "Shorewall:fw2net:LOG:" -m comment --comment "IELOG" > -A fw2net -j DROP > > Since I cannot change the --nflog-prefix (which I presume is supplied to > my IELOG custom action via @{logtag}), shorewall, in its infinite > wisdom, determines that the tag should be "Shorewall:fw2net:LOG:", which > is clearly wrong, given that the operation in question is DROP. > > If I had access to this variable *and* was able to change it, I would > have corrected this in the action itself (provided I could also pass > @{logtag} to the NFLOG action - something I still doubt), not to mention > that I would have done a lot of other nice things with this action too > if I knew how the tests in ?IF operate. What you are asking for is a way to set the 'disposition' field in the log prefix. I'll work on that. > 8. For some reason after upgrading shorewall I started getting all > broadcast packets (on 2 of my internal nets) logged and then dropped, > which filled my logs with all sorts of rubbish pretty quickly. I then > looked in more details and saw that dropBcast action, even though > defined, it now has 0 references (in other words, it just sits there > unused - not optimised by shorewall properly). > > In addition, my logs also started filling up at an alarming rate with, > what I later discovered, INVALID packets (caused, in part, by accessing > misconfigured web servers). > > All this, as it turned out, seems to have been caused by a change in the > "policy" file I've made before the upgrade - when I have, say, "$FW net > DROP info" for example, shorewall "automatically" includes the "Drop" > action (which in itself includes dropBcast as well as dropInvalid to > drop INVALID packets, among other things), but when I have, say, "$FW > net DROP:IELOG(...)" that does not happen. > > Maybe a note somewhere to tell me of this behaviour would be nice, since > I was not aware that "Drop" is automatically included by "default" and > then "silently" excluded by shorewall when I use a custom-defined action > in my policy file instead. Default actions are discussed at http://ipv6.shorewall.net/Actions.html#Default. > 9. dropInvalid built-in action does not function properly: > > blrules > ~~~~~~~ > dropInvalid all all > > generates the following statement for all zones for which I do *not* > have explicit blrules defined: > > -A X2Y -m conntrack --ctstate NEW,INVALID -m conntrack --ctstate INVALID > -j DROP > > where X and Y are the zones in question. Patch dropInvalid.patch attached. > > The above, quite clearly, won't work as intended. Perhaps instead of > doing this "dropInvalid" malarkey, it would be much better to have > something like INVALID_DISPOSITION in shorewall[6].conf (coupled perhaps > with INVALID_LOG_LEVEL as well) and be done with it, particularly if I > wish to use disposition other than "DROP" to examine those packets > (A_DROP for example, not to mention a custom-defined action - see > suggestion No 2 below). > > One final thought with regards to Drop/dropBcast/dropInvalid: most of > the time, dropping broadcast packets makes sense, but sometimes, > particularly where DHCP negotiation is invlolved, that isn't > appropriate. The Drop action does a lot of good things, but since it > includes dropBcast, it can't be used everywhere "as standard". I think > it would be better if dropping (or not) of broadcast traffic is handled > separately (maybe by an option in "interfaces") and the original Drop is > modified to exclude dropBcast. > > As far as INVALID packets are concerned, if admins rely on "Drop" for > these to be handled, this action is normally added to the zone2zone > chain right at the end. That means an INVALID packet has to traverse the > whole chain until it is eventually dropped by "Drop", which isn't very > efficient, not to mention wrong if there is a LOG/NFLOG/ULOG target > along the way, which will log this packet, causing unneccessary > confusion to whoever cares to look at these logs (i.e. myself for example). > > If INVALID_DISPOSITION is introduced, it would make perfect sense for > INVALID packets to be dropped as early as possible - maybe the first > thing in INPUT, OUTPUT and FORWARD chains before any other processing > takes place (this is exactly what I ended up doing at the end - I > slapped a "--cstate INVALID -j DROP" rule right at the beginning of my > INPUT, OUTPUT and FORWARD chains and it is all peace and quiet now). > > What I also did was to insert rules "manually" for each zone2zone > separately for another - "Drop2" action (a modified "Drop" to exclude > dropBcast and dropInvalid) - accounting for zones where DHCP negotiation > takes place, hence dropBcast isn't needed and dropInvalid is also no > longer required since this is dealth with in INPUT, OUTPUT and FORWARD > in the way I mentioned above. The standard configuration samples released with Shorewall all contain this as their initial rule: # Don't allow connection pickup from the net # Invalid(DROP) net all tcp That rule *does* work correctly, even without the attached patch. The reason that the rule is net->all rather than all->all is that it allows for connection pickup from the local and firewall zones. So that approach is more flexible than INVALID_DISPOSITION. > > 10. "A_REJECT" is not optimised properly by shorewall - even though > A_REJECT is not used in my firewall (it has 0 references), it is still > created. "A_ACCEPT", for example, is optimised (it is not created if not > used) - same goes for "A_DROP". I'll probably need a sample configuration that shows that problem since all three are handled by the same code. > > 11. Specifying multiple protocols is ignored in "secmarks": > > secmarks > ~~~~~~~~ > system_u:object_r:www_server_packet_t:s0 I:N > eth0:+mickey-mouse-net,+lucky-stars-net +www-ports udp,tcp > > produces: > > -A INPUT -p 17 -i eth0 -m conntrack --ctstate NEW -m set --match-set > mickey-mouse-net src -m set --match-set www-ports dst -j SECMARK > --selctx system_u:object_r:www_server_packet_t:s0 > -A INPUT -p 17 -i eth0 -m conntrack --ctstate NEW -m set --match-set > lucky-stars-net src -m set --match-set www-ports dst -j SECMARK --selctx > system_u:object_r:www_server_packet_t:s0 > > Note that the "tcp" part of "secmarks" statement above is completely > ignored. Similar rule anywhere else is treated properly (separate > iptables statements for "udp" and "tcp"). Actually, the rules and blrules files are the only ones that support multiple protocols. So it is actually a bug in the secmark rules processor that it doesn't catch the incorrect usage. I suspect that the same bug would show up on more contexts than just the secmarks file. > > > Suggestions: > > 1. Any progress on allowing custom actions to be used in "conntrack"? Do > you need any help with this? I haven't done anything yet. > > 2. Currently, when I try to use my own customised action in various > *_DISPOSITION fields in shorewall[6].conf instead of, say, "DROP" or > "A_DROP", I get a message similar to "ERROR: SMURF_DISPOSITION must be > 'DROP' or 'A_DROP'" or "WARNING: Duplicate Action Name (A_DROP) Ignored" > if I try to "substitute" the built-in default A_DROP action. Why? > > What is the reason behind introducing such restrictions? More > importantly, can I have the ability to use my own custom-defined action > in places like RELATED_DISPOSITION, RPFILTER_DISPOSITION, > SMURF_DISPOSITION and so on? No reason other than it takes time and effort to code, test and document this type of feature. > > 3. It would be nice if I could specify "named" action parameters, like > so: IELOG(audit=>drop,switch=>mamas,action=>DROP) (note that the 'log' > parameter in this case is not specified!) and then refer to them in the > action itself with something like @audit, @log, @switch, @action for > example. > > These "named" parameters could be defined in the action itself (may be > something like "?DEFINE <varname> $n" statements) and then used in the > manner I just described. > > Specifying action parameters in such a way will add clarity, which is > particularly useful in custom-defined actions where there are quite a > few parameters used, e.g. > "IEEELOG(log=>true,nflog_local=>1,nflog_ext=>2,audit=>drop,action=>DROP,a_switch=>mamas)" > > is much more clearer than having "IEEELOG(drop,1,-,2,true,DROP,mamas)" > (also note the different position of the various parameters specified in > each action!). That would be nice, indeed. > > One final thought on this as far as specifying parameters go: to avoid > any possible collisions and "syntax errors", could the parameters of an > action be specified with quotes/double quotes? This will eliminate any > possibility of wrong error messages and allow more freedom in specifying > action parameters containing "forbidden" characters (like comma, colon, > semicolon etc). Given the techniques that the rules compiler uses to do its parsing, handling of quoted strings is in that context would be prohibitively expensive. > > 4. It would be nice if I could perform more "complex" expressions in > ?IF, matching wildcards or regex for example. As I can't find where > "?IF" is explained properly, I am only guessing what to include/not > include in the match statement and I am also having to guess what is > allowed/not allowed. Documenting Perl syntax is the poorest use of my Shorewall time that I can imagine. You could fill a good sized library with Perl books. The way that Shorewall evaluates expressions is: - Performs variable expansion - Passes the resulting expression to Perl's eval() function - Raises a fatal error if the expression failed to compile or blows up at run-time. - The result is then treated as a boolean; the undefined value, 0 and the empty string are treated as false; all other values are treated as true (normal Perl convention). In Perl, arithmetic expressions are the same as in C. When comparing strings, use 'eq' for testing equality and 'ne' for testing non-equality. > > 5. As already mentioned in 7 above, it would be nice to have the ability > to change the @{logtag} and @{loglevel} shorewall variables (with > "?SET"?), so that they could be used in LOG/NFLOG/ULOG actions, > otherwise I can see very little use of these variables even if they are > made available to custom actions. SET tag=@{logtag} SET level=@{loglevel} Now set and use $tag and $level to your heart's content. But that still doesn't solve the problem that you describe above regarding 'DROP' and 'LOG'. > > 6. Add DESTROY_IPSETS=Yes|No option (or similar) to shorewall[6].conf to > destroy all ipsets when the firewall is restarted (at the point when the > firewall doesn't contain any rules so that the destruction of ipsets > succeeds!). Shorewall does have "SAVE_IPSETS", but that is the opposite > of what would be DESTROY_IPSETS. > > Currently, if I have ipsets used in stoppedrules (or anywhere else when > the firewall is in stopped state), (re)-starting the firewall would > bring a couple of failures simply because these ipsets exist and are > currently in use. Including "ipset flush|destroy" in init or > "forcefully" trying to destroy these sets, as I am currently doing, > isn't helping because the current rules in existence prevent these > ipsets from being flushed and destroyed. > > If shorewall wipes out all ipsets at the precise point when the firewall > has no rules (i.e. iptables chains are all empty), that would erradicate > the problem. There is no such point if you use ipsets in both the stoppedrules and in any other context. > > 7. It would be nice to have the equivalent of "stoppedrules" but for > arp. In other words, the arp rules which should be in effect when > shorewall is in stopped state. Currently, I do this manually and prevent > any sort of arp access outside the internal network if shorewall is > stopped (and clear the arp cache, bar the permanent entries there!). > That gives me extra layer of security in case shorewall is shutdown/does > not start for whatever reason. Noted. -Tom -- Tom Eastep \ When I die, I want to go like my Grandfather who Shoreline, \ died peacefully in his sleep. Not screaming like Washington, USA \ all of the passengers in his car http://shorewall.net \________________________________________________ -- Tom Eastep \ When I die, I want to go like my Grandfather who Shoreline, \ died peacefully in his sleep. Not screaming like Washington, USA \ all of the passengers in his car http://shorewall.net \________________________________________________
diff --git a/Shorewall/Perl/Shorewall/Rules.pm b/Shorewall/Perl/Shorewall/Rules.pm
index 4ac427e..4d54410 100644
--- a/Shorewall/Perl/Shorewall/Rules.pm
+++ b/Shorewall/Perl/Shorewall/Rules.pm
@@ -2322,7 +2322,7 @@ sub process_rule1 ( $$$$$$$$$$$$$$$$$$ ) {
);
}
- unless ( $section =~ /^NEW|DEFAULTACTION$/ || $inaction ) {
+ unless ( $section =~ /^NEW|DEFAULTACTION$/ || $inaction || $basictarget eq 'dropInvalid' ) {
if ( $config{FASTACCEPT} ) {
fatal_error "Entries in the $section SECTION of the rules file not permitted with FASTACCEPT=Yes" unless
$section eq 'BLACKLIST' ||
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. ON SALE this month only -- learn more at: http://p.sf.net/sfu/learnmore_123012
_______________________________________________ Shorewall-devel mailing list Shorewall-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/shorewall-devel
