On Thu, May 21, 2015 at 11:07 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> > On Tue, May 19, 2015 at 14:07 +0200, Alexandr Nedvedicky wrote:
> > > Hello Mike,
> > >
> > > I've reworked patch from yesterday. I've done some quick testing
> > > to see if it fixes problem. It looks like it works. I have not
> > > tested NAT-64 yet. Also I'd like to come up with test case, which
> > > will show the state check is still able to block invalid ICMP packet
> > > (invalid with respect to state).
> > >
> > > The idea of fix is to keep icmp_dir in state as well. The icmp_dir
> > > indicates whether state got created by ICMP request or response.
> > > This is useful later in pf_icmp_state_lookup() to check whether
> > > ICMP request/response matches state direction.
> > >
> >
> > This feels slightly convoluted... check my diff out! (:
>
> nice, I like your "XOR Magic!" comment.
(:
> Looks like I was trying to fix the other end...
And you were not wrong.
> your patch is minimalistic and correct as far as I can tell.
>
Well, not entirely (: I did it while exploring the code and sent
out to provoke further discussion. Today I've talked to reyk@ and
we think that it's better to go down a different road: make sure we
don't create states on reply packets in the first place.
Please consider the following: unless sloppy state tracking is used,
TCP states can only be set up by the first SYN packet (ok, ok, it's
because of "flags S/SA", but that is largely irrelevant nowadays
since "S/SA" is now the default and nobody is doing "flags any").
ICMP was made to adhere to a stricter set of rules and recently
I've added sloppy handling there so perhaps we should just prevent
state creation for replies (icmp_dir == PF_OUT)? If that is not
what sysadmin wants he can always specify "keep state (sloopy)" and
move on with his/her life.
I've tested this with ICMP, ICMPv6 and NAT64 (slightly). Any OKs?
Objections?
diff --git sys/net/pf.c sys/net/pf.c
index 39d5cb6..dbc8707 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3201,10 +3201,15 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
PF_TEST_ATTRIB((r->type && r->type != icmptype + 1),
TAILQ_NEXT(r, entries));
/* icmp only. type always 0 in other cases */
PF_TEST_ATTRIB((r->code && r->code != icmpcode + 1),
TAILQ_NEXT(r, entries));
+ /* icmp only. don't create states on replies */
+ PF_TEST_ATTRIB((r->keep_state && !state_icmp &&
+ (r->rule_flag & PFRULE_STATESLOPPY) == 0 &&
+ icmp_dir != PF_IN),
+ TAILQ_NEXT(r, entries));
break;
default:
break;
}
