Re: pf flag PFDESC_IP_REAS
On Mon, Jun 20, 2011 at 02:09:39AM +0200, Alexander Bluhm wrote: Hi, We accept more TCP reset packets in pf, if fragment reassembly is turned off. That does not make sense to me. It came into the tree here: revision 1.443 date: 2004/04/27 18:28:07; author: frantzen; state: Exp; lines: +9 -6 validate the sequence numbers on TCP resets are an exact match. check is only enabled when we're doing full frag reassembly and thus have full seq info ok markus@ Nowadays we have full fragment reassembly on by default, the fragment cache has gone. When the user turns it off, he has to cope with the consequences. Fragmented TCP reset packets too short for the sequence number are likely to be evil. ok to remove PFDESC_IP_REAS? Yes please. OK claudio@ and sorry for the conflict. The codepath with this check is only entered with either not fragmented packets or with packets that are reassembled. So there is no way you could end up there with a fragment. Plus it removes the pd from pf_normalize_ip() which is something I planned to do soon. Index: net/pf.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v retrieving revision 1.747 diff -u -p -r1.747 pf.c --- net/pf.c 2 Jun 2011 22:08:40 - 1.747 +++ net/pf.c 17 Jun 2011 23:59:35 - @@ -3617,8 +3617,7 @@ pf_tcp_track_full(struct pf_state_peer * (ackskew = (MAXACKWINDOW sws)) /* Acking not more than one window forward */ ((th-th_flags TH_RST) == 0 || orig_seq == src-seqlo || - (orig_seq == src-seqlo + 1) || (orig_seq + 1 == src-seqlo) || - (pd-flags PFDESC_IP_REAS) == 0)) { + (orig_seq == src-seqlo + 1) || (orig_seq + 1 == src-seqlo))) { /* Require an exact/+1 sequence match on resets when possible */ if (dst-scrub || src-scrub) { @@ -5810,7 +5809,7 @@ pf_test(int dir, struct ifnet *ifp, stru h = mtod(m, struct ip *); if (pf_status.reass (h-ip_off htons(IP_MF | IP_OFFMASK)) - pf_normalize_ip(m0, dir, kif, reason, pd) != PF_PASS) { + pf_normalize_ip(m0, dir, kif, reason) != PF_PASS) { action = PF_DROP; goto done; } @@ -6090,7 +6089,7 @@ pf_test6(int fwdir, struct ifnet *ifp, s /* packet reassembly */ if (pf_status.reass - pf_normalize_ip6(m0, fwdir, kif, reason, pd) != PF_PASS) { + pf_normalize_ip6(m0, fwdir, kif, reason) != PF_PASS) { action = PF_DROP; goto done; } Index: net/pf_norm.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v retrieving revision 1.133 diff -u -p -r1.133 pf_norm.c --- net/pf_norm.c 24 May 2011 14:01:52 - 1.133 +++ net/pf_norm.c 17 Jun 2011 23:58:35 - @@ -740,7 +740,7 @@ pf_refragment6(struct mbuf **m0, struct int pf_normalize_ip(struct mbuf **m0, int dir, struct pfi_kif *kif, -u_short *reason, struct pf_pdesc *pd) +u_short *reason) { struct mbuf *m = *m0; struct ip *h = mtod(m, struct ip *); @@ -787,7 +787,6 @@ pf_normalize_ip(struct mbuf **m0, int di if (h-ip_off ~htons(IP_DF)) h-ip_off = htons(IP_DF); - pd-flags |= PFDESC_IP_REAS; return (PF_PASS); drop: @@ -798,7 +797,7 @@ pf_normalize_ip(struct mbuf **m0, int di #ifdef INET6 int pf_normalize_ip6(struct mbuf **m0, int dir, struct pfi_kif *kif, -u_short *reason, struct pf_pdesc *pd) +u_short *reason) { struct mbuf *m = *m0; struct ip6_hdr *h = mtod(m, struct ip6_hdr *); @@ -924,7 +923,6 @@ pf_normalize_ip6(struct mbuf **m0, int d if (m == NULL) return (PF_PASS); - pd-flags |= PFDESC_IP_REAS; return (PF_PASS); shortpkt: Index: net/pfvar.h === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v retrieving revision 1.332 diff -u -p -r1.332 pfvar.h --- net/pfvar.h 24 May 2011 14:01:52 - 1.332 +++ net/pfvar.h 17 Jun 2011 23:58:57 - @@ -1221,8 +1221,6 @@ struct pf_pdesc { u_int16_t *proto_sum; u_int16_trdomain; /* original routing domain */ - u_int16_tflags; -#define PFDESC_IP_REAS 0x0002 /* IP frags would've been reassembled */ sa_family_t af; u_int8_t proto; u_int8_t tos; @@ -1777,10 +1775,8 @@ intpf_match_gid(u_int8_t, gid_t, gid_t, int pf_refragment6(struct mbuf **, struct m_tag *mtag, int); void pf_normalize_init(void); -int pf_normalize_ip(struct mbuf **, int, struct pfi_kif *, u_short *, - struct pf_pdesc *); -int pf_normalize_ip6(struct mbuf **, int, struct pfi_kif *, u_short *, -
pf flag PFDESC_IP_REAS
Hi, We accept more TCP reset packets in pf, if fragment reassembly is turned off. That does not make sense to me. It came into the tree here: revision 1.443 date: 2004/04/27 18:28:07; author: frantzen; state: Exp; lines: +9 -6 validate the sequence numbers on TCP resets are an exact match. check is only enabled when we're doing full frag reassembly and thus have full seq info ok markus@ Nowadays we have full fragment reassembly on by default, the fragment cache has gone. When the user turns it off, he has to cope with the consequences. Fragmented TCP reset packets too short for the sequence number are likely to be evil. ok to remove PFDESC_IP_REAS? bluhm Index: net/pf.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v retrieving revision 1.747 diff -u -p -r1.747 pf.c --- net/pf.c2 Jun 2011 22:08:40 - 1.747 +++ net/pf.c17 Jun 2011 23:59:35 - @@ -3617,8 +3617,7 @@ pf_tcp_track_full(struct pf_state_peer * (ackskew = (MAXACKWINDOW sws)) /* Acking not more than one window forward */ ((th-th_flags TH_RST) == 0 || orig_seq == src-seqlo || - (orig_seq == src-seqlo + 1) || (orig_seq + 1 == src-seqlo) || - (pd-flags PFDESC_IP_REAS) == 0)) { + (orig_seq == src-seqlo + 1) || (orig_seq + 1 == src-seqlo))) { /* Require an exact/+1 sequence match on resets when possible */ if (dst-scrub || src-scrub) { @@ -5810,7 +5809,7 @@ pf_test(int dir, struct ifnet *ifp, stru h = mtod(m, struct ip *); if (pf_status.reass (h-ip_off htons(IP_MF | IP_OFFMASK)) - pf_normalize_ip(m0, dir, kif, reason, pd) != PF_PASS) { + pf_normalize_ip(m0, dir, kif, reason) != PF_PASS) { action = PF_DROP; goto done; } @@ -6090,7 +6089,7 @@ pf_test6(int fwdir, struct ifnet *ifp, s /* packet reassembly */ if (pf_status.reass - pf_normalize_ip6(m0, fwdir, kif, reason, pd) != PF_PASS) { + pf_normalize_ip6(m0, fwdir, kif, reason) != PF_PASS) { action = PF_DROP; goto done; } Index: net/pf_norm.c === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v retrieving revision 1.133 diff -u -p -r1.133 pf_norm.c --- net/pf_norm.c 24 May 2011 14:01:52 - 1.133 +++ net/pf_norm.c 17 Jun 2011 23:58:35 - @@ -740,7 +740,7 @@ pf_refragment6(struct mbuf **m0, struct int pf_normalize_ip(struct mbuf **m0, int dir, struct pfi_kif *kif, -u_short *reason, struct pf_pdesc *pd) +u_short *reason) { struct mbuf *m = *m0; struct ip *h = mtod(m, struct ip *); @@ -787,7 +787,6 @@ pf_normalize_ip(struct mbuf **m0, int di if (h-ip_off ~htons(IP_DF)) h-ip_off = htons(IP_DF); - pd-flags |= PFDESC_IP_REAS; return (PF_PASS); drop: @@ -798,7 +797,7 @@ pf_normalize_ip(struct mbuf **m0, int di #ifdef INET6 int pf_normalize_ip6(struct mbuf **m0, int dir, struct pfi_kif *kif, -u_short *reason, struct pf_pdesc *pd) +u_short *reason) { struct mbuf *m = *m0; struct ip6_hdr *h = mtod(m, struct ip6_hdr *); @@ -924,7 +923,6 @@ pf_normalize_ip6(struct mbuf **m0, int d if (m == NULL) return (PF_PASS); - pd-flags |= PFDESC_IP_REAS; return (PF_PASS); shortpkt: Index: net/pfvar.h === RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v retrieving revision 1.332 diff -u -p -r1.332 pfvar.h --- net/pfvar.h 24 May 2011 14:01:52 - 1.332 +++ net/pfvar.h 17 Jun 2011 23:58:57 - @@ -1221,8 +1221,6 @@ struct pf_pdesc { u_int16_t *proto_sum; u_int16_trdomain; /* original routing domain */ - u_int16_tflags; -#define PFDESC_IP_REAS 0x0002 /* IP frags would've been reassembled */ sa_family_t af; u_int8_t proto; u_int8_t tos; @@ -1777,10 +1775,8 @@ int pf_match_gid(u_int8_t, gid_t, gid_t, intpf_refragment6(struct mbuf **, struct m_tag *mtag, int); void pf_normalize_init(void); -intpf_normalize_ip(struct mbuf **, int, struct pfi_kif *, u_short *, - struct pf_pdesc *); -intpf_normalize_ip6(struct mbuf **, int, struct pfi_kif *, u_short *, - struct pf_pdesc *); +intpf_normalize_ip(struct mbuf **, int, struct pfi_kif *, u_short *); +intpf_normalize_ip6(struct mbuf **, int, struct pfi_kif *, u_short *); intpf_normalize_tcp(int, struct pfi_kif *, struct mbuf *, int, int, void *, struct pf_pdesc *); void pf_normalize_tcp_cleanup(struct pf_state *);