Potential DoS attack on PF due to infinite loop

Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Potential DoS attack on PF due to infinite loop

Jingmin Zhou
Recently we discovered a potential bug in pf_lb.c. It is in the latest code that we retrieved from here:


The problem is at line 224. When a LB rule is configured to have 65535 as the high port, and uint16 variable tmp reaches it, ++(tmp) will wrap around and hence potentially enter into an infinite loop. Of course, it only happens when high is configured to 65535.

Thanks,
Jingmin

                    146: int
1.13      henning   147: pf_get_sport(struct pf_pdesc *pd, struct pf_rule *r,
1.1       pyr       148:     struct pf_addr *naddr, u_int16_t *nport, u_int16_t low, u_int16_t high,
1.13      henning   149:     struct pf_src_node **sn)
1.1       pyr       150: {
                    151:        struct pf_state_key_cmp key;
                    152:        struct pf_addr          init_addr;
                    153:        u_int16_t               cut;
1.54      bluhm     154:        int                     dir = (pd->dir == PF_IN) ? PF_OUT : PF_IN;
                    155:        int                     sidx = pd->sidx;
                    156:        int                     didx = pd->didx;
1.1       pyr       157:
                    158:        bzero(&init_addr, sizeof(init_addr));
1.19      claudio   159:        if (pf_map_addr(pd->naf, r, &pd->nsaddr, naddr, &init_addr, sn, &r->nat,
1.9       henning   160:            PF_SN_NAT))
1.1       pyr       161:                return (1);
                    162:
1.35      bluhm     163:        if (pd->proto == IPPROTO_ICMP) {
                    164:                if (pd->ndport == htons(ICMP_ECHO)) {
1.4       mcbride   165:                        low = 1;
                    166:                        high = 65535;
                    167:                } else
                    168:                        return (0);     /* Don't try to modify non-echo ICMP */
1.1       pyr       169:        }
1.35      bluhm     170: #ifdef INET6
                    171:        if (pd->proto == IPPROTO_ICMPV6) {
                    172:                if (pd->ndport == htons(ICMP6_ECHO_REQUEST)) {
                    173:                        low = 1;
                    174:                        high = 65535;
                    175:                } else
                    176:                        return (0);     /* Don't try to modify non-echo ICMP */
                    177:        }
                    178: #endif /* INET6 */
1.1       pyr       179:
                    180:        do {
1.19      claudio   181:                key.af = pd->naf;
1.13      henning   182:                key.proto = pd->proto;
                    183:                key.rdomain = pd->rdomain;
1.54      bluhm     184:                PF_ACPY(&key.addr[didx], &pd->ndaddr, key.af);
                    185:                PF_ACPY(&key.addr[sidx], naddr, key.af);
                    186:                key.port[didx] = pd->ndport;
1.1       pyr       187:
                    188:                /*
                    189:                 * port search; start random, step;
                    190:                 * similar 2 portloop in in_pcbbind
                    191:                 */
1.13      henning   192:                if (!(pd->proto == IPPROTO_TCP || pd->proto == IPPROTO_UDP ||
1.30      mikeb     193:                    pd->proto == IPPROTO_ICMP || pd->proto == IPPROTO_ICMPV6)) {
1.12      sthen     194:                        /* XXX bug: icmp states dont use the id on both
                    195:                         * XXX sides (traceroute -I through nat) */
1.54      bluhm     196:                        key.port[sidx] = pd->nsport;
                    197:                        if (pf_find_state_all(&key, dir, NULL) == NULL) {
1.13      henning   198:                                *nport = pd->nsport;
1.1       pyr       199:                                return (0);
1.12      sthen     200:                        }
1.1       pyr       201:                } else if (low == 0 && high == 0) {
1.54      bluhm     202:                        key.port[sidx] = pd->nsport;
                    203:                        if (pf_find_state_all(&key, dir, NULL) == NULL) {
1.13      henning   204:                                *nport = pd->nsport;
1.1       pyr       205:                                return (0);
1.12      sthen     206:                        }
1.1       pyr       207:                } else if (low == high) {
1.54      bluhm     208:                        key.port[sidx] = htons(low);
                    209:                        if (pf_find_state_all(&key, dir, NULL) == NULL) {
1.1       pyr       210:                                *nport = htons(low);
                    211:                                return (0);
                    212:                        }
                    213:                } else {
                    214:                        u_int16_t tmp;
                    215:
                    216:                        if (low > high) {
                    217:                                tmp = low;
                    218:                                low = high;
                    219:                                high = tmp;
                    220:                        }
                    221:                        /* low < high */
                    222:                        cut = arc4random_uniform(1 + high - low) + low;
                    223:                        /* low <= cut <= high */
                    224:                        for (tmp = cut; tmp <= high; ++(tmp)) {
1.54      bluhm     225:                                key.port[sidx] = htons(tmp);
                    226:                                if (pf_find_state_all(&key, dir, NULL) ==
1.13      henning   227:                                    NULL && !in_baddynamic(tmp, pd->proto)) {
1.1       pyr       228:                                        *nport = htons(tmp);
                    229:                                        return (0);
                    230:                                }
                    231:                        }
                    232:                        for (tmp = cut - 1; tmp >= low; --(tmp)) {
1.54      bluhm     233:                                key.port[sidx] = htons(tmp);
                    234:                                if (pf_find_state_all(&key, dir, NULL) ==
1.13      henning   235:                                    NULL && !in_baddynamic(tmp, pd->proto)) {
1.1       pyr       236:                                        *nport = htons(tmp);
                    237:                                        return (0);
                    238:                                }
                    239:                        }
                    240:                }
                    241:
1.6       henning   242:                switch (r->nat.opts & PF_POOL_TYPEMASK) {
1.1       pyr       243:                case PF_POOL_RANDOM:
                    244:                case PF_POOL_ROUNDROBIN:
1.15      zinke     245:                case PF_POOL_LEASTSTATES:
1.29      mikeb     246:                        /*
                    247:                         * pick a different source address since we're out
                    248:                         * of free port choices for the current one.
                    249:                         */
1.19      claudio   250:                        if (pf_map_addr(pd->naf, r, &pd->nsaddr, naddr,
1.13      henning   251:                            &init_addr, sn, &r->nat, PF_SN_NAT))
1.1       pyr       252:                                return (1);
                    253:                        break;
                    254:                case PF_POOL_NONE:
                    255:                case PF_POOL_SRCHASH:
                    256:                case PF_POOL_BITMASK:
                    257:                default:
                    258:                        return (1);
                    259:                }
1.19      claudio   260:        } while (! PF_AEQ(&init_addr, naddr, pd->naf) );
1.1       pyr       261:        return (1);                                     /* none available */
                    262: }

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Potential DoS attack on PF due to infinite loop

Alexander Bluhm
On Tue, Jul 11, 2017 at 12:29:33PM -0700, Jingmin Zhou wrote:
> The problem is at line 224. When a LB rule is configured to have 65535 as
> the high port, and uint16 variable tmp reaches it, ++(tmp) will wrap around
> and hence potentially enter into an infinite loop. Of course, it only
> happens when high is configured to 65535.

I think this should detect all overflow problems.

ok?

bluhm

Index: net/pf_lb.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_lb.c,v
retrieving revision 1.60
diff -u -p -r1.60 pf_lb.c
--- net/pf_lb.c 23 Apr 2017 11:37:11 -0000 1.60
+++ net/pf_lb.c 12 Jul 2017 12:11:35 -0000
@@ -211,7 +211,7 @@ pf_get_sport(struct pf_pdesc *pd, struct
  return (0);
  }
  } else {
- u_int16_t tmp;
+ u_int32_t tmp;
 
  if (low > high) {
  tmp = low;
@@ -221,7 +221,7 @@ pf_get_sport(struct pf_pdesc *pd, struct
  /* low < high */
  cut = arc4random_uniform(1 + high - low) + low;
  /* low <= cut <= high */
- for (tmp = cut; tmp <= high; ++(tmp)) {
+ for (tmp = cut; tmp <= high && tmp <= 0xffff; ++tmp) {
  key.port[sidx] = htons(tmp);
  if (pf_find_state_all(&key, dir, NULL) ==
     NULL && !in_baddynamic(tmp, pd->proto)) {
@@ -229,7 +229,8 @@ pf_get_sport(struct pf_pdesc *pd, struct
  return (0);
  }
  }
- for (tmp = cut - 1; tmp >= low; --(tmp)) {
+ tmp = cut;
+ for (tmp -= 1; tmp >= low && tmp <= 0xffff; --tmp) {
  key.port[sidx] = htons(tmp);
  if (pf_find_state_all(&key, dir, NULL) ==
     NULL && !in_baddynamic(tmp, pd->proto)) {
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Potential DoS attack on PF due to infinite loop

Alexandr Nedvedicky
On Wed, Jul 12, 2017 at 02:40:58PM +0200, Alexander Bluhm wrote:
> On Tue, Jul 11, 2017 at 12:29:33PM -0700, Jingmin Zhou wrote:
> > The problem is at line 224. When a LB rule is configured to have 65535 as
> > the high port, and uint16 variable tmp reaches it, ++(tmp) will wrap around
> > and hence potentially enter into an infinite loop. Of course, it only
> > happens when high is configured to 65535.
>
> I think this should detect all overflow problems.
>
> ok?

looks good to me.

OK sashan

>
> bluhm
>
> Index: net/pf_lb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_lb.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 pf_lb.c
> --- net/pf_lb.c 23 Apr 2017 11:37:11 -0000 1.60
> +++ net/pf_lb.c 12 Jul 2017 12:11:35 -0000
> @@ -211,7 +211,7 @@ pf_get_sport(struct pf_pdesc *pd, struct
>   return (0);
>   }
>   } else {
> - u_int16_t tmp;
> + u_int32_t tmp;
>  
>   if (low > high) {
>   tmp = low;
> @@ -221,7 +221,7 @@ pf_get_sport(struct pf_pdesc *pd, struct
>   /* low < high */
>   cut = arc4random_uniform(1 + high - low) + low;
>   /* low <= cut <= high */
> - for (tmp = cut; tmp <= high; ++(tmp)) {
> + for (tmp = cut; tmp <= high && tmp <= 0xffff; ++tmp) {
>   key.port[sidx] = htons(tmp);
>   if (pf_find_state_all(&key, dir, NULL) ==
>      NULL && !in_baddynamic(tmp, pd->proto)) {
> @@ -229,7 +229,8 @@ pf_get_sport(struct pf_pdesc *pd, struct
>   return (0);
>   }
>   }
> - for (tmp = cut - 1; tmp >= low; --(tmp)) {
> + tmp = cut;
> + for (tmp -= 1; tmp >= low && tmp <= 0xffff; --tmp) {
>   key.port[sidx] = htons(tmp);
>   if (pf_find_state_all(&key, dir, NULL) ==
>      NULL && !in_baddynamic(tmp, pd->proto)) {
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Potential DoS attack on PF due to infinite loop

Alexander Bluhm
In reply to this post by Jingmin Zhou
On Tue, Jul 11, 2017 at 12:29:33PM -0700, Jingmin Zhou wrote:
> Recently we discovered a potential bug in pf_lb.c.

I have commited a fix.  Thanks for the report and analysis.

bluhm
Loading...