Re: mismatch for ICMP state created by inound response
Hello, 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: absolutely not, 0 is O.K. thank you very much. regards sasha 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_tvirtual_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; }
Re: mismatch for ICMP state created by inound response
* Alexandr Nedvedicky alexandr.nedvedi...@oracle.com [2015-05-21 21:29]: 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... agreed. 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... I don't see any harm done by this on our side, so yeah, why not. having a default case there is better style anyway. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS. Virtual Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: mismatch for ICMP state created by inound response
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_tvirtual_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; }
Re: mismatch for ICMP state created by inound response
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... your patch is minimalistic and correct as far as I can tell. P.S. I took discussion off-line not to create extra noise on tech@openbsd.org feel free go get the alias back to loop. Nah, that's what tech@ is for! O.K. I won't do it again... regards sasha
Re: mismatch for ICMP state created by inound response
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'. thanks a lot regards sasha Index: pf.c === RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.913 diff -u -r1.913 pf.c --- pf.c11 May 2015 12:22:14 - 1.913 +++ pf.c21 May 2015 18:59:29 - @@ -3124,6 +3124,8 @@ } break; #endif /* INET6 */ + default: + icmp_dir = -1; } ruleset = pf_main_ruleset; @@ -3206,6 +3208,11 @@ 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;
Re: mismatch for ICMP state created by inound response
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; }
Re: mismatch for ICMP state created by inound response
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! (:
The attached patch keeps ICMP state match for both cases:
pass in on dst-nic from any to any
and
pass out on dst-nic from any to any
the dst-nic is NIC towards ICMP destination network.
regards
sasha
P.S. I took discussion off-line not to create extra noise on tech@openbsd.org
feel free go get the alias back to loop.
Nah, that's what tech@ is for!
diff --git sys/net/pf.c sys/net/pf.c
index 39d5cb6..81e23de 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -177,11 +177,11 @@ intpf_test_rule(struct pf_pdesc
*, struct pf_rule **,
static __inline int pf_create_state(struct pf_pdesc *, struct pf_rule *,
struct pf_rule *, struct pf_rule *,
struct pf_state_key **, struct pf_state_key **,
int *, struct pf_state **, int,
struct pf_rule_slist *, struct pf_rule_actions *,
- struct pf_src_node *[]);
+ struct pf_src_node *[], int);
static __inline int pf_state_key_addr_setup(struct pf_pdesc *, void *,
int, struct pf_addr *, int, struct pf_addr *,
int, int);
int pf_state_key_setup(struct pf_pdesc *, struct
pf_state_key **, struct pf_state_key **, int);
@@ -3365,11 +3365,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
REASON_SET(reason, PFRES_MAXSTATES);
goto cleanup;
}
action = pf_create_state(pd, r, a, nr, skw, sks, rewrite,
- sm, tag, rules, act, sns);
+ sm, tag, rules, act, sns, icmp_dir);
if (action != PF_PASS)
return (action);
if (sks != skw) {
struct pf_state_key *sk;
@@ -3433,11 +3433,12 @@ cleanup:
static __inline int
pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
struct pf_rule *nr, struct pf_state_key **skw, struct pf_state_key **sks,
int *rewrite, struct pf_state **sm, int tag, struct pf_rule_slist *rules,
-struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX])
+struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX],
+int icmp_dir)
{
struct pf_state *s = NULL;
struct tcphdr *th = pd-hdr.tcp;
u_int16_tmss = tcp_mssdflt;
u_short reason;
@@ -3446,10 +3447,11 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
s = pool_get(pf_state_pl, PR_NOWAIT | PR_ZERO);
if (s == NULL) {
REASON_SET(reason, PFRES_MEMORY);
goto csfailed;
}
+ s-direction = pd-dir;
s-rule.ptr = r;
s-anchor.ptr = a;
s-natrule.ptr = nr;
memcpy(s-match_rules, rules, sizeof(s-match_rules));
STATE_INC_COUNTERS(s);
@@ -3517,10 +3519,14 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
break;
case IPPROTO_ICMP:
#ifdef INET6
case IPPROTO_ICMPV6:
#endif
+ /* XOR Magic! */
+ s-direction = s-direction == icmp_dir ?
+ PF_IN : PF_OUT;
+
s-timeout = PFTM_ICMP_FIRST_PACKET;
break;
default:
s-src.state = PFOTHERS_SINGLE;
s-dst.state = PFOTHERS_NO_TRAFFIC;
@@ -3543,11 +3549,10 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
DPFPRINTF(LOG_ERR,
pf_normalize_tcp_stateful failed on first pkt);
goto csfailed;
}
}
- s-direction = pd-dir;
if (pf_state_key_setup(pd, skw, sks, act-rtableid)) {
REASON_SET(reason, PFRES_MEMORY);
goto csfailed;
}
mismatch for ICMP state created by inound response
Hello,
during our testing we've discovered small glitch in ICMP state handling.
we use simple rule as follows:
# pfctl -sr
pass in on vnet2 all flags S/SA
next we create a local outbound traffic using ping to arbitrary destination
over vnet2 interface. This is what we get:
# ping 172.16.1.2
PING 172.16.1.2 (172.16.1.2): 56 data bytes
64 bytes from 172.16.1.2: icmp_seq=0 ttl=255 time=0.718 ms
ping: sendto: No route to host
ping: wrote 172.16.1.2 64 chars, ret=-1
ping: sendto: No route to host
ping: wrote 172.16.1.2 64 chars, ret=-1
/snip
64 bytes from 172.16.1.2: icmp_seq=20 ttl=255 time=0.587 ms
ping: sendto: No route to host
ping: wrote 172.16.1.2 64 chars, ret=-1
it looks like state created by icmp_seq=0 response must expire first before
firewall is able to put next packet to wire.
It took me a while to figure out what's going on here. I think the problem is
PF keeps packet direction in pf_state::direction, when state gets created, while
the pf_icmp_state_lookup() uses icmp_dir to verify whether packet is valid or
invalid for given state.
The idea of the fix is straightforward:
remember ICMP direction in pf_pdesc, so it can be passed to newly created
state for ICMP packet.
the straightforward fix is bit cluttered by change, which switches icmp_dir to
u_int8_t.
as soon as fix get applied the ping command works, all ICMP probes leave
firewall host.
patch is attached.
regards
sasha
? icmp-state.patch
? pf_table.c.diff
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c18 May 2015 17:07:14 -
@@ -148,8 +148,8 @@
struct pf_state_peer *);
voidpf_change_a6(struct pf_pdesc *, struct pf_addr *a,
struct pf_addr *an);
-int pf_icmp_mapping(struct pf_pdesc *, u_int8_t, int *,
- int *, u_int16_t *, u_int16_t *);
+int pf_icmp_mapping(struct pf_pdesc *, u_int8_t,
+ u_int8_t *, int *, u_int16_t *, u_int16_t *);
voidpf_change_icmp(struct pf_pdesc *, struct pf_addr *,
u_int16_t *, struct pf_addr *, struct pf_addr *,
u_int16_t, sa_family_t);
@@ -197,7 +197,7 @@
u_short *);
int pf_icmp_state_lookup(struct pf_pdesc *,
struct pf_state_key_cmp *, struct pf_state **,
- u_int16_t, u_int16_t, int, int *, int, int);
+ u_int16_t, u_int16_t, u_int8_t, int *, int, int);
int pf_test_state_icmp(struct pf_pdesc *,
struct pf_state **, u_short *);
u_int8_tpf_get_wscale(struct pf_pdesc *);
@@ -1689,8 +1689,8 @@
#endif /* INET6 */
int
-pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, int *icmp_dir, int *multi,
-u_int16_t *virtual_id, u_int16_t *virtual_type)
+pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, u_int8_t *icmp_dir,
+int *multi, u_int16_t *virtual_id, u_int16_t *virtual_type)
{
/*
* ICMP types marked with PF_OUT are typically responses to
@@ -3081,9 +3081,10 @@
int tag = -1;
int asd = 0;
int match = 0;
- int state_icmp = 0, icmp_dir, multi;
+ int state_icmp = 0, multi;
u_int16_tvirtual_type, virtual_id;
u_int8_t icmptype = 0, icmpcode = 0;
+ u_int8_t icmp_dir = (u_int8_t)-1;
bzero(act, sizeof(act));
bzero(sns, sizeof(sns));
@@ -3124,7 +3125,10 @@
}
break;
#endif /* INET6 */
+ default:
+ icmp_dir = (u_int8_t)-1;
}
+ pd-icmp_dir = icmp_dir;
ruleset = pf_main_ruleset;
r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr);
@@ -3549,7 +3553,7 @@
goto csfailed;
}
}
- s-direction = pd-dir;
+ s-direction = (pd-icmp_dir == (u_int8_t)-1) ? pd-dir : pd-icmp_dir;
if (pf_state_key_setup(pd, skw, sks, act-rtableid)) {
REASON_SET(reason, PFRES_MEMORY);
@@ -3627,7 +3631,7 @@
int
pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport,
struct pf_addr *daddr, u_int16_t dport, u_int16_t virtual_type,
-int icmp_dir)
+u_int8_t icmp_dir)
{
/*
* when called from bpf_mtap_pflog, there are extra constraints:
@@ -4416,7 +4420,7 @@
int
pf_icmp_state_lookup(struct pf_pdesc *pd, struct pf_state_key_cmp *key,
struct pf_state **state, u_int16_t icmpid, u_int16_t type,
-int icmp_dir, int
Re: mismatch for ICMP state created by inound response
On Mon, May 18, 2015 at 19:24 +0200, Alexandr Nedvedicky wrote: Hello, during our testing we've discovered small glitch in ICMP state handling. we use simple rule as follows: Hi, # pfctl -sr pass in on vnet2 all flags S/SA If that is the only rule there is, then you need to consider the default stateless pass. So effectively you've got: pass all no state pass in on vnet2 all flags S/SA keep state next we create a local outbound traffic using ping to arbitrary destination over vnet2 interface. This is what we get: Which is a bad combo really. You're supposed to use a pass out keep state for that, however the behaviour you're describing is rather peculiar (and reproducible on OpenBSD). # ping 172.16.1.2 PING 172.16.1.2 (172.16.1.2): 56 data bytes 64 bytes from 172.16.1.2: icmp_seq=0 ttl=255 time=0.718 ms ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 /snip 64 bytes from 172.16.1.2: icmp_seq=20 ttl=255 time=0.587 ms ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 it looks like state created by icmp_seq=0 response must expire first before firewall is able to put next packet to wire. It took me a while to figure out what's going on here. I think the problem is PF keeps packet direction in pf_state::direction, when state gets created, while the pf_icmp_state_lookup() uses icmp_dir to verify whether packet is valid or invalid for given state. The idea of the fix is straightforward: remember ICMP direction in pf_pdesc, so it can be passed to newly created state for ICMP packet. the straightforward fix is bit cluttered by change, which switches icmp_dir to u_int8_t. This needs a much closer look, but it might be a result of a bad interaction between stateless and stateful rules. This code is rather fragile and needs special care. If you enable debugging (pfctl -x debug) you'll see that the state is being created for the reply packet (10.10.1.2 is pinging 10.10.1.1): May 18 20:26:54 pf: key search, if=vmx1: ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988 May 18 20:26:54 pf: key search, if=vmx1: ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988 May 18 20:26:54 pf: key setup: ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988 stack: (0) - And then the next packet I send matches the state but fails the direction check that is there to prevent reply spoofing/ replaying: May 18 20:26:55 pf: key search, if=vmx1: ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988 May 18 20:26:55 pf: icmp type 8 in wrong direction (1): ICMP in wire: (0) 10.10.1.1:8 10.10.1.2:15988 stack: (0) - 0:0 @0 Perhaps we should deny state creation on reply in the first place, but this might have repercussions of its own. Generally speaking the advice is to have a proper outgoing state and not relying on the default pass all no state rule. One recommended way of achieving it is to have a block all as your first rule. as soon as fix get applied the ping command works, all ICMP probes leave firewall host. Thanks for the patch, we'll be investigating this further. patch is attached. regards sasha
Re: mismatch for ICMP state created by inound response
Hello, Thanks for the patch, we'll be investigating this further. my deep apologize, I was too fast on send trigger. the patch is toxic. It breaks the opposite case: pass out on vnet2 all flags S/SA once rule above is used with patch applied we drop the first ICMP reply, so ping stops to work completely. as you've said: This needs a much closer look, but it might be a result of a bad I need to study PF source code more... sorry for extra noise. regards sasha
