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 *);

Reply via email to