On Sat, Feb 05, 2011 at 07:51:27PM +0100, Henning Brauer wrote:
> indeed. and as much as i'm all for defensive programming, pf_test_rule
> will never be called from anything but pf_test[6] - at least without
> heavy heavy major super duper changes, besides there not being a reson
> to. thus:
I checked the call graph again and found this:
pf_test() -> pflog_packet() -> bpf_mtap_pflog() -> bpf_catchpacket()
-> pflog_bpfcopy() -> pf_translate()
It is not obvious anymore that IPv4-ICMP6 and IPv6-ICMP cannot occur
in pf_translate(). I recommend defensive programming in that case.
So I'd like to commit a combination of my previous two diffs.
ok?
bluhm
pf_test() and pf_test6() drop IPv4-ICMP6 and IPv6-ICMP packets. Do
not do the same check in pf_test_rule() again.
Index: net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.723
diff -u -p -r1.723 pf.c
--- net/pf.c 5 Feb 2011 17:29:05 -0000 1.723
+++ net/pf.c 5 Feb 2011 17:50:32 -0000
@@ -2776,8 +2776,6 @@ pf_test_rule(struct pf_rule **rm, struct
break;
#ifdef INET
case IPPROTO_ICMP:
- if (pd->af != AF_INET)
- break;
icmptype = pd->hdr.icmp->icmp_type;
icmpcode = pd->hdr.icmp->icmp_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
@@ -2793,8 +2791,6 @@ pf_test_rule(struct pf_rule **rm, struct
#endif /* INET */
#ifdef INET6
case IPPROTO_ICMPV6:
- if (af != AF_INET6)
- break;
icmptype = pd->hdr.icmp6->icmp6_type;
icmpcode = pd->hdr.icmp6->icmp6_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
pf_translate() may be called from pflog_packet(). Make sure that
IPv4-ICMP6 and IPv6-ICMP packets are handled correctly in case they
are dropped and logged.
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.722
diff -u -p -r1.722 pf.c
--- net/pf.c 22 Jan 2011 11:43:57 -0000 1.722
+++ net/pf.c 5 Feb 2011 13:43:58 -0000
@@ -3281,8 +3281,7 @@ pf_translate(struct pf_pdesc *pd, struct
if (PF_ANEQ(daddr, pd->dst, pd->af))
pd->destchg = 1;
- switch (pd->proto) {
- case IPPROTO_TCP:
+ if (pd->proto == IPPROTO_TCP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.tcp->th_sum, saddr, sport, 0, pd->af);
@@ -3293,9 +3292,7 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.tcp->th_sum, daddr, dport, 0, pd->af);
rewrite = 1;
}
- break;
-
- case IPPROTO_UDP:
+ } else if (pd->proto == IPPROTO_UDP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.udp->uh_sum, saddr, sport, 1, pd->af);
@@ -3306,10 +3303,8 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.udp->uh_sum, daddr, dport, 1, pd->af);
rewrite = 1;
}
- break;
-
#ifdef INET
- case IPPROTO_ICMP:
+ } else if (pd->proto == IPPROTO_ICMP && pd->af == AF_INET) {
if (PF_ANEQ(saddr, pd->src, pd->af)) {
pf_change_a(&pd->src->v4.s_addr, pd->ip_sum,
saddr->v4.s_addr, 0);
@@ -3331,28 +3326,21 @@ pf_translate(struct pf_pdesc *pd, struct
rewrite = 1;
}
}
- break;
#endif /* INET */
-
#ifdef INET6
- case IPPROTO_ICMPV6:
- if (pd->af == AF_INET6) {
- if (PF_ANEQ(saddr, pd->src, pd->af)) {
- pf_change_a6(pd->src,
- &pd->hdr.icmp6->icmp6_cksum, saddr, 0);
- rewrite = 1;
- }
- if (PF_ANEQ(daddr, pd->dst, pd->af)) {
- pf_change_a6(pd->dst,
- &pd->hdr.icmp6->icmp6_cksum, daddr, 0);
- rewrite = 1;
- }
- break;
+ } else if (pd->proto == IPPROTO_ICMPV6 && pd->af == AF_INET6) {
+ if (PF_ANEQ(saddr, pd->src, pd->af)) {
+ pf_change_a6(pd->src, &pd->hdr.icmp6->icmp6_cksum,
+ saddr, 0);
+ rewrite = 1;
+ }
+ if (PF_ANEQ(daddr, pd->dst, pd->af)) {
+ pf_change_a6(pd->dst, &pd->hdr.icmp6->icmp6_cksum,
+ daddr, 0);
+ rewrite = 1;
}
- /* FALLTHROUGH */
#endif /* INET6 */
-
- default:
+ } else {
switch (pd->af) {
#ifdef INET
case AF_INET:
