Date: Sat, 17 May 2003 12:41:08 -0700 From: Luigi Rizzo <rizzo@icir.org> To: Peter Pentchev <roam@ringlet.net> Cc: ipfw@freebsd.org Subject: Re: ipfw2 buffer overruns Message-ID: <20030517124107.A36097@xorpc.icir.org> In-Reply-To: <20030516135052.GA13482@straylight.oblivion.bg>; from roam@ringlet.net on Fri, May 16, 2003 at 04:50:53PM %2B0300 References: <20030516135052.GA13482@straylight.oblivion.bg>
next in thread | previous in thread | raw e-mail | index | archive | help
Peter, i agree that it is useful to have the ipfw2 compiler check for overflows, but i believe the checks can be greatly simplified by putting them in two places only: right after the 'target:' label in OR_START(), and at the beginning of the while() loop after 'read_options:'. These are the only places where you can have a loop, in all other places you only insert single rules of bounded size so as long as you check that there is a 'large enough' buffer (in the order of 60 bytes or so) at the beginning and before each iteration of the loop, you should be safe. cheers luigi On Fri, May 16, 2003 at 04:50:53PM +0300, Peter Pentchev wrote: > Hi, >=20 > A friend of mine, Kiril Todorov (CC'd), recently came across some quite > strange ipfw2 behavior on -STABLE: when given a specific rule to add, > ipfw would hang. A bit of digging into src/sbin/ipfw/ipfw2.c revealed a > couple of internal buffers - actbuf[], cmdbuf[], rulebuf[] - with a set > length of 255, and a couple of pointers traversing those buffers which > were never actually checked for running over the end. Thus, it was > trivial to construct a long enough 'ipfw add' command that would > eventually overrun the buffer, with much confusion ensuing. >=20 > Attached is a sample rule that causes this, and a patch which performs a > couple of length checks and refuses to add the rule if a buffer overrun > is detected. This is not the most elegant solution, and in a couple of > the checks the damage is already done, but still... >=20 > The patch is against -STABLE; it applies to -CURRENT with just a couple > of offset lines, and the resulting source compiles; I do not currently > have a functional -CURRENT machine to test it on, though. It works on > -STABLE, correctly diagnosing the oversized rule, and some other tests I > threw at it in a hurry. Still, this is the first time I'm touching the > ipfw code, so there is a very high probability that this is not the > right way, or not even close to the right direction; feel free to point > it out :) >=20 > G'luck, > Peter >=20 > --=20 > Peter Pentchev roam@ringlet.net roam@sbnd.net roam@FreeBSD.org > PGP key: http://people.FreeBSD.org/~roam/roam.key.asc > Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 > You have, of course, just begun reading the sentence that you have just f= inished reading. > add 10 skipto 1000 all from { 139.92.144.0/24 or 139.92.170.0/24 or 139.9= 2.51.0/24 or 152.158.113.0/24 or 157.125.223.0/24 or 192.92.129.0/24 or 193= .108.24.0/24 or 193.108.32.0/23 or 193.109.54.0/23 or 193.110.159.0/24 or 1= 93.110.216.0/21 or 193.110.82.0/24 or 193.111.194.0/23 or 193.111.89.0/24 o= r 193.178.152.0/23 or 193.178.166.0/24 or 193.178.222.0/24 or 193.193.162.0= /22 or 193.193.164.0/24 or 193.193.182.0/24 or 193.194.140.0/23 or 193.194.= 141.0/24 or 193.194.156.0/24 or 193.200.0.0/16 or 193.201.114.0/24 or 193.2= 01.172.0/24 or 193.201.240.0/22 or 193.254.29.0/24 or 193.41.182.0/23 or 19= 3.41.188.0/22 or 193.41.206.0/24 or 193.41.64.0/22 or 193.68.0.0/19 or 193.= 68.128.0/17 or 193.68.96.0/19 or 194.12.224.0/19 or 194.141.0.0/16 or 194.1= 45.63.0/24 or 194.153.145.0/24 or 195.138.128.0/19 or 195.212.63.0/24 or 19= 5.230.0.0/19 or 195.234.236.0/22 or 195.24.32.0/19 or 195.34.96.0/19 or 195= .72.112.0/24 or 195.75.71.0/24 or 195.96.224.0/19 or 209.239.78.0/23 or 212= .116.128.0/19 or 212.122.160.0/19 or 212.124.64.0/19 or 212.21.128.0/19 or = 212.36.0.0/19 or 212.39.64.0/19 or 212.50.0.0/19 or 212.5.128.0/19 or 212.5= 6.0/19 or 212.7.192.0/19 or 212.72.192.0/19 or 212.91.160.0/19 or 212.95.16= 0.0/19 or 213.130.64.0/19 or 213.137.32.0/19 or 213.145.96.0/19 or 213.16.3= 2.0/19 or 213.169.32.0/19 or 213.174.0.0/19 or 213.191.192.0/19 or 213.208.= 10.0/23 or 213.222.32.0/19 or 213.226.0.0/18 or 213.91.128.0/17 or 217.10.2= 40.0/20 or 217.18.240.0/20 or 217.145.160.0/20 or 217.197.128.0/20 or 217.7= 9.32.0/19 or 217.79.64.0/20 or 217.9.224.0/20 or 217.75.128.0/20 or 10.0.0.= 0/8 or 172.16.0.0/12 } to any > Index: src/sbin/ipfw/ipfw2.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > RCS file: /home/ncvs/src/sbin/ipfw/ipfw2.c,v > retrieving revision 1.4.2.12 > diff -u -r1.4.2.12 ipfw2.c > --- src/sbin/ipfw/ipfw2.c 14 Apr 2003 12:41:37 -0000 1.4.2.12 > +++ src/sbin/ipfw/ipfw2.c 16 May 2003 13:40:25 -0000 > @@ -2481,6 +2481,14 @@ > return NULL; > } > =20 > +static void > +check_length(ipfw_insn *cmd, size_t add, void *ptr, size_t size) > +{ > + > + if ((uintptr_t)(cmd + add) > (uintptr_t)((char *)ptr + size)) > + errx(EX_DATAERR, "Rule too long!"); > +} > + > /* > * Parse arguments and assemble the microinstructions which make up a ru= le. > * Rules are added into the 'rulebuf' and then copied in the correct ord= er > @@ -2562,6 +2570,7 @@ > NEED1("missing action"); > i =3D match_token(rule_actions, *av); > ac--; av++; > + check_length(action, 1, actbuf, sizeof(actbuf)); /* superfluous.. */ > action->len =3D 1; /* default */ > switch(i) { > case TOK_CHECKSTATE: > @@ -2602,6 +2611,7 @@ > case TOK_QUEUE: > case TOK_PIPE: > action->len =3D F_INSN_SIZE(ipfw_insn_pipe); > + check_length(action, action->len, actbuf, sizeof(actbuf)); > case TOK_SKIPTO: > if (i =3D=3D TOK_QUEUE) > action->opcode =3D O_QUEUE; > @@ -2639,6 +2649,7 @@ > =20 > action->opcode =3D O_FORWARD_IP; > action->len =3D F_INSN_SIZE(ipfw_insn_sa); > + check_length(action, action->len, actbuf, sizeof(actbuf)); > =20 > p->sa.sin_len =3D sizeof(struct sockaddr_in); > p->sa.sin_family =3D AF_INET; > @@ -2665,6 +2676,7 @@ > default: > errx(EX_DATAERR, "invalid action %s\n", av[-1]); > } > + check_length(action, F_LEN(action), actbuf, sizeof(actbuf)); > action =3D next_cmd(action); > =20 > /* > @@ -2687,6 +2699,7 @@ > errx(EX_DATAERR, "logamount must be positive"); > ac--; av++; > } > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > } > =20 > @@ -2751,12 +2764,16 @@ > !strncmp(*av, "mac", strlen(*av))) { > ac--; av++; /* the "MAC" keyword */ > add_mac(cmd, ac, av); /* exits in case of errors */ > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > ac -=3D 2; av +=3D 2; /* dst-mac and src-mac */ > NOT_BLOCK; > NEED1("missing mac type"); > if (add_mactype(cmd, ac, av[0])) > + { > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > + } > ac--; av++; /* any or mac-type */ > goto read_options; > } > @@ -2775,6 +2792,7 @@ > else { > proto =3D cmd->arg1; > prev =3D cmd; > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > } > } else if (first_cmd !=3D cmd) { > @@ -2800,6 +2818,7 @@ > ac--; av++; > if (F_LEN(cmd) !=3D 0) { /* ! any */ > prev =3D cmd; > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > } > } > @@ -2814,7 +2833,10 @@ > add_ports(cmd, *av, proto, O_IP_SRCPORT)) { > ac--; av++; > if (F_LEN(cmd) !=3D 0) > + { > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > + } > } > } > =20 > @@ -2835,6 +2857,7 @@ > ac--; av++; > if (F_LEN(cmd) !=3D 0) { /* ! any */ > prev =3D cmd; > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > } > } > @@ -2849,7 +2872,10 @@ > add_ports(cmd, *av, proto, O_IP_DSTPORT)) { > ac--; av++; > if (F_LEN(cmd) !=3D 0) > + { > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > + } > } > } > =20 > @@ -3167,6 +3193,7 @@ > } > if (F_LEN(cmd) > 0) { /* prepare to advance */ > prev =3D cmd; > + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf)); > cmd =3D next_cmd(cmd); > } > } > @@ -3187,6 +3214,7 @@ > if (match_prob !=3D 1) { /* 1 means always match */ > dst->opcode =3D O_PROB; > dst->len =3D 2; > + check_length(dst, 2, rulebuf, sizeof(rulebuf)); > *((int32_t *)(dst+1)) =3D (int32_t)(match_prob * 0x7fffffff); > dst +=3D dst->len; > } > @@ -3196,6 +3224,7 @@ > */ > if (have_state && have_state->opcode !=3D O_CHECK_STATE) { > fill_cmd(dst, O_PROBE_STATE, 0, 0); > + check_length(dst, F_LEN(dst), rulebuf, sizeof(rulebuf)); > dst =3D next_cmd(dst); > } > /* > @@ -3210,6 +3239,7 @@ > case O_LIMIT: > break; > default: > + check_length(dst, i, rulebuf, sizeof(rulebuf)); > bcopy(src, dst, i * sizeof(u_int32_t)); > dst +=3D i; > } > @@ -3220,6 +3250,7 @@ > */ > if (have_state && have_state->opcode !=3D O_CHECK_STATE) { > i =3D F_LEN(have_state); > + check_length(dst, i, rulebuf, sizeof(rulebuf)); > bcopy(have_state, dst, i * sizeof(u_int32_t)); > dst +=3D i; > } > @@ -3234,6 +3265,7 @@ > src =3D (ipfw_insn *)cmdbuf; > if ( src->opcode =3D=3D O_LOG ) { > i =3D F_LEN(src); > + check_length(dst, i, rulebuf, sizeof(rulebuf)); > bcopy(src, dst, i * sizeof(u_int32_t)); > dst +=3D i; > } > @@ -3242,6 +3274,7 @@ > */ > for (src =3D (ipfw_insn *)actbuf; src !=3D action; src +=3D i) { > i =3D F_LEN(src); > + check_length(dst, i, rulebuf, sizeof(rulebuf)); > bcopy(src, dst, i * sizeof(u_int32_t)); > dst +=3D i; > }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030517124107.A36097>