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.c 11 May 2015 12:22:14 -0000 1.913
+++ pf.c 18 May 2015 17:07:14 -0000
@@ -148,8 +148,8 @@
struct pf_state_peer *);
void pf_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 *);
void pf_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_t pf_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_t virtual_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 *iidx, int multi, int inner)
+ u_int8_t icmp_dir, int *iidx, int multi, int inner)
{
int direction;
@@ -4469,8 +4473,8 @@
{
struct pf_addr *saddr = pd->src, *daddr = pd->dst;
u_int16_t virtual_id, virtual_type;
- u_int8_t icmptype;
- int icmp_dir, iidx, ret, multi, copyback = 0;
+ u_int8_t icmptype, icmp_dir;
+ int iidx, ret, multi, copyback = 0;
struct pf_state_key_cmp key;
@@ -6041,6 +6045,7 @@
pd->didx = (dir == PF_IN) ? 1 : 0;
pd->af = pd->naf = af;
pd->rdomain = rtable_l2(pd->m->m_pkthdr.ph_rtableid);
+ pd->icmp_dir = -1;
switch (pd->af) {
case AF_INET: {
Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.414
diff -u -r1.414 pfvar.h
--- pfvar.h 11 Apr 2015 13:00:12 -0000 1.414
+++ pfvar.h 18 May 2015 17:07:14 -0000
@@ -1284,6 +1284,7 @@
u_int8_t tos;
u_int8_t ttl;
u_int8_t dir; /* direction */
+ u_int8_t icmp_dir;
u_int8_t sidx; /* key index for source */
u_int8_t didx; /* key index for destination */
u_int8_t destchg; /* flag set when destination changed */
@@ -1850,7 +1851,7 @@
void pf_pkt_addr_changed(struct mbuf *);
int pf_state_key_attach(struct pf_state_key *, struct pf_state *, int);
int pf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
- struct pf_addr *, u_int16_t, u_int16_t, int);
+ struct pf_addr *, u_int16_t, u_int16_t, u_int8_t);
int pf_translate_af(struct pf_pdesc *);
void pf_route(struct mbuf **, struct pf_rule *, int,
struct ifnet *, struct pf_state *);
