With reference to my earlier post about Hefferlump Traps,
and after further reflection, I'd like to propose the
following one-line patch to the file "fil.c":
change:
if (pass & FR_QUICK)
to:
if ((pass & FR_QUICK) && (skip == 0))
this line is about 6 or 7 lines (depending on the version of
IPFilter) before the end if the function "fr_scanlist"; and
applies to all recent versions 3.4 and the alpha version 4.
The net effect should be that:
* stopping because of a "quick" tag in a "skip" rule would
be delayed until the "skip" process had finished skipping
the subsequent rules -- but then such a "quick" tag is
ignored in any case because the rule flags are not copied
to the "pass" variable from the "passt" variable (which in
turn had been ser from the "fr->fr_flags" field in the rule);
the net effect is a small processor overhead (unless the
skip count and rule grour were *very* large)
* stopping because of an inherited "quick" tag on a group head
rule that entered a group that did not reset the "pass"
variable before encountering a "skip" rule would be delayed
while the "skip" was skipping; thus leaving the situation
as if the "skip" rule and those skipped had not been
present in the rule group
* the effect with "log" rules would not be changed
The last point is that this change gives minimum effects;
I personally think that a change that made "log" (only)
rules behave the same way as "skip" rules would probably
also remove another hefferlump trap (not just the one I
fell into). This change would require the insertion of
" && (passt & FR_LOGMASK) != FR_LOG)" into the patched
line just before the last ")". Alternativly, the code
could be modified in two places to note (in a local
variable) that the "pass = passt" statement had been
executed and to only test the "FR_QUICK" flag if it
had (been executed). Yet another variation would be
to test and remember the "FR_QUICK" flag *at the point
that the "pass = passt" statement is written* and to
then test that memory instead of "(pass & FR_QUICK)"
at the end of the for-loop. These variations are all
equivalent in effect and mainly a matter of style;
personally I prefer the last because it most clearly
expresses the idea that the "quick" tag only has an
effect on a rule that both matches (of course!) *and*
*changes the saved flags*. This would come down to,
in the "fr_scanlist" function in "fil.c", making the
following changes:
* at the top changing:
int rulen, portcmp, off, logged, skip;
to:
int rulen, portcmp, off, logged, skip, quick;
* in the middle changing:
pass = passt;
to:
pass = passt; quick = pass & FR_QUICK;
* near the end changing:
if (pass & FR_QUICK)
to:
if (quick)
Comments, anyone, Darren?
If there are no objections that convince me it's a bad
idea I'll test it and let the list know what happens.
If it works as I expect do we think this would be a
good patch to include in the master sources?
--
David Pick
