incorrect fallthrough in pf

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

incorrect fallthrough in pf

Mike Belopuhov-4
hi, in pf_translate, when we're changing addresses for the icmp messages
there's an unjustified fallthrough in the IPPROTO_ICMPV6 case.  in fact
this doesn't seem to harm anything because default case performs the
same operation.  note that pd->ip_sum is null in ipv6 case so pf_change_a6
just punches a translation address to the packet with PF_ACPY.

henning@ agrees that this fallthrough was introduced by mistake, but we
won't mind if somebody with pf knowledge will glance through the code.

cvs diff -r1.657 -r1.658 pf.c  might be handy.

ok?

Index: pf.c
===================================================================
RCS file: /home/cvs/src/sys/net/pf.c,v
retrieving revision 1.722
diff -u -p -U10 -r1.722 pf.c
--- pf.c 22 Jan 2011 11:43:57 -0000 1.722
+++ pf.c 2 Feb 2011 15:01:38 -0000
@@ -3342,21 +3342,21 @@ pf_translate(struct pf_pdesc *pd, struct
     &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;
  }
- /* FALLTHROUGH */
+ break;
 #endif /* INET6 */
 
  default:
  switch (pd->af) {
 #ifdef INET
  case 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);
  rewrite = 1;

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Alexander Bluhm
On Wed, Feb 02, 2011 at 04:14:01PM +0100, Mike Belopuhov wrote:
> hi, in pf_translate, when we're changing addresses for the icmp messages
> there's an unjustified fallthrough in the IPPROTO_ICMPV6 case.  in fact
> this doesn't seem to harm anything because default case performs the
> same operation.  note that pd->ip_sum is null in ipv6 case so pf_change_a6
> just punches a translation address to the packet with PF_ACPY.
>
> henning@ agrees that this fallthrough was introduced by mistake, but we
> won't mind if somebody with pf knowledge will glance through the code.

I think there is a missing check and fallthrough in the icmp case.

The logic should be

if (proto == tcp) {
} else if (proto == udp) {
} else if (proto == icmp && af == inet) {
} else if (proto == icmp6 && af == inet6) {
} else {
}

The current code would do icmp processing for an ipv6 packet with
protocol 1.  Such a packet is strange but it should not get special
translation treatment.

bluhm

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Mike Belopuhov
On Sat, Feb 5, 2011 at 1:50 PM, Alexander Bluhm <[hidden email]>
wrote:

> I think there is a missing check and fallthrough in the icmp case.
>
> The logic should be
>
> if (proto == tcp) {
> } else if (proto == udp) {
> } else if (proto == icmp && af == inet) {
> } else if (proto == icmp6 && af == inet6) {
> } else {
> }
>
> The current code would do icmp processing for an ipv6 packet with
> protocol 1.  Such a packet is strange but it should not get special
> translation treatment.
>
> bluhm
>
>

we can check if "af == inet" in icmp case obviously, but how and why
can we end up with af == inet6 and an icmp payload (or af == inet
and icmp6 payload for that matter)?

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Alexander Bluhm
On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote:
> we can check if "af == inet" in icmp case obviously, but how and why
> can we end up with af == inet6 and an icmp payload (or af == inet
> and icmp6 payload for that matter)?

Somebody could send us such a packet.

It is strange but not illegal.  If we decide, pf should drop such
packets that is a complete different thing.

Anyway we should make sure that pf_translate() can handle these
packets by itself.  Protocol 1 for IPv6 packets means no special
treatment so it should end in the else case.

I suggest that diff (untested):


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:

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Henning Brauer-5
* Alexander Bluhm <[hidden email]> [2011-02-05 14:56]:
> On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote:
> > we can check if "af == inet" in icmp case obviously, but how and why
> > can we end up with af == inet6 and an icmp payload (or af == inet
> > and icmp6 payload for that matter)?
>
> Somebody could send us such a packet.

I'm pretty damn sure we catch that way earlier.

--
Henning Brauer, [hidden email], [hidden email]
BS Web Services, http://bsws.de
Full-Service ISP - Secure Hosting, Mail and DNS Services
Dedicated Servers, Rootservers, Application Hosting

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Mike Belopuhov
In reply to this post by Alexander Bluhm
On Sat, Feb 5, 2011 at 2:54 PM, Alexander Bluhm <[hidden email]>
wrote:

> On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote:
>> we can check if "af == inet" in icmp case obviously, but how and why
>> can we end up with af == inet6 and an icmp payload (or af == inet
>> and icmp6 payload for that matter)?
>
> Somebody could send us such a packet.
>
> It is strange but not illegal.  If we decide, pf should drop such
> packets that is a complete different thing.
>
> Anyway we should make sure that pf_translate() can handle these
> packets by itself.  Protocol 1 for IPv6 packets means no special
> treatment so it should end in the else case.
>
> I suggest that diff (untested):
>

in this case i'm okay with your diff.

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Alexander Bluhm
In reply to this post by Henning Brauer-5
On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> * Alexander Bluhm <[hidden email]> [2011-02-05 14:56]:
> > Somebody could send us such a packet.
>
> I'm pretty damn sure we catch that way earlier.

Yeah, it panics right away if nat/rdr is used with unusual protocol.

panic: m_clget: request for 1926905556 byte cluster
Stopped at      Debugger+0x4:   popl    %ebp
RUN AT LEAST 'trace' AND 'ps' AND INCLUDE OUTPUT WHEN REPORTING THIS PANIC!
DO NOT EVEN BOTHER REPORTING THIS WITHOUT INCLUDING THAT INFORMATION!
ddb> trace
Debugger(d08dcebc,d45b4bd0,d08bc8c4,d45b4bd0,d0202f0d) at Debugger+0x4
panic(d08bc8c4,72da3ed4,d45b4bf4,d0400453,40) at panic+0x5d
m_clget(d30ea700,2,0,72da3ed4,d30591a8) at m_clget+0x138
m_copyback(d30ea500,14,72da3ed4,d45b4d94,2) at m_copyback+0x1d2
pf_test_rule(d45b4dc0,d45b4dbc,1,d0e04d00,d30ea500) at pf_test_rule+0xd91
pf_test(1,d0f06038,d45b4eac,0,d0f06000) at pf_test+0xcd3
ipv4_input(d30ea500,6,d45b4ec4,d0441185,d0202f0d) at ipv4_input+0x204
ipintr(d0202f0d,d0eefbe0,d45b4ee4,d057ac2f,0) at ipintr+0x49
netintr(0,0,d0ef0a80,0,d0201fc6) at netintr+0xd5
softintr_dispatch(1) at softintr_dispatch+0x4f
Xsoftnet() at Xsoftnet+0x12

hrdlen can be uinitialized.  Let's fix that first.

ok?


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 16:16:57 -0000
@@ -3047,7 +3047,7 @@ pf_test_rule(struct pf_rule **rm, struct
  }
 
  /* copy back packet headers if we performed NAT operations */
- if (rewrite)
+ if (rewrite && hdrlen)
  m_copyback(m, off, hdrlen, pd->hdr.any, M_NOWAIT);
 
 #if NPFSYNC > 0
@@ -5517,6 +5517,7 @@ pf_setup_pdesc(sa_family_t af, int dir,
  if (pd->hdr.any == NULL)
  panic("pf_setup_pdesc: no storage for headers provided");
 
+ *hdrlen = 0;
  switch (af) {
 #ifdef INET
  case AF_INET: {

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Alexander Bluhm
In reply to this post by Henning Brauer-5
On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> I'm pretty damn sure we catch that way earlier.

You are right.

pf_test():
        case IPPROTO_ICMPV6: {
                action = PF_DROP;
                DPFPRINTF(LOG_NOTICE,
                    "dropping IPv4 packet with ICMPv6 payload");
                goto done;
        }

pf_test6():
        case IPPROTO_ICMP: {
                action = PF_DROP;
                DPFPRINTF(LOG_NOTICE,
                    "dropping IPv6 packet with ICMPv4 payload");
                goto done;
        }

But then some more checks in pf_test_rule() are dead code and should
be removed.  Either we rely on the checks in pf_test[46]() or we
don't.  But we should do it consistently.

ok?

bluhm


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,
@@ -3336,20 +3332,17 @@ pf_translate(struct pf_pdesc *pd, struct
 
 #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;
+ 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 */
+ break;
 #endif /* INET6 */
 
  default:

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Henning Brauer-5
* Alexander Bluhm <[hidden email]> [2011-02-05 19:02]:

> On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> > I'm pretty damn sure we catch that way earlier.
>
> You are right.
>
> pf_test():
>         case IPPROTO_ICMPV6: {
>                 action = PF_DROP;
>                 DPFPRINTF(LOG_NOTICE,
>                     "dropping IPv4 packet with ICMPv6 payload");
>                 goto done;
>         }
>
> pf_test6():
>         case IPPROTO_ICMP: {
>                 action = PF_DROP;
>                 DPFPRINTF(LOG_NOTICE,
>                     "dropping IPv6 packet with ICMPv4 payload");
>                 goto done;
>         }
>
> But then some more checks in pf_test_rule() are dead code and should
> be removed.  Either we rely on the checks in pf_test[46]() or we
> don't.  But we should do it consistently.

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:

> ok?

yes.

--
Henning Brauer, [hidden email], [hidden email]
BS Web Services, http://bsws.de
Full-Service ISP - Secure Hosting, Mail and DNS Services
Dedicated Servers, Rootservers, Application Hosting

Reply | Threaded
Open this post in threaded view
|

Re: incorrect fallthrough in pf

Alexander Bluhm
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: