On Thu, May 21, 2015 at 21:08 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> >
> > 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.
> >
> that's actually very wise approach as replies can be spoofed...
>
> > I've tested this with ICMP, ICMPv6 and NAT64 (slightly). Any OKs?
> > Objections?
>
> I have no objections, just a small wish, can you set icmp_dir to -1,
> if we are not dealing with ICMP? there is a tool we use in Solaris,
> which yells on us because of uninitialized variable. I know it's
> false positive, but I've gave up on explaining...
>
> patch below combines your fix with my 'wish'.
>
if you don't mind, i'd rather set it to 0 (PF_IN is 1) right
where it's declared than add an additional case, like so:
diff --git sys/net/pf.c sys/net/pf.c
index 39d5cb6..5d44f43 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3075,11 +3075,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
u_short reason;
int rewrite = 0;
int tag = -1;
int asd = 0;
int match = 0;
- int state_icmp = 0, icmp_dir;
+ int state_icmp = 0, icmp_dir = 0;
u_int16_t virtual_type, virtual_id;
u_int8_t icmptype = 0, icmpcode = 0;
bzero(&act, sizeof(act));
bzero(sns, sizeof(sns));
@@ -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;
}
