ipfw2 buffer overruns
Luigi Rizzo
rizzo at icir.org
Sat May 17 12:41:12 PDT 2003
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,
>
> 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.
>
> 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...
>
> 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 :)
>
> G'luck,
> Peter
>
> --
> Peter Pentchev roam at ringlet.net roam at sbnd.net roam at 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 finished reading.
> add 10 skipto 1000 all from { 139.92.144.0/24 or 139.92.170.0/24 or 139.92.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 193.110.216.0/21 or 193.110.82.0/24 or 193.111.194.0/23 or 193.111.89.0/24 or 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.201.172.0/24 or 193.201.240.0/22 or 193.254.29.0/24 or 193.41.182.0/23 or 193.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.145.63.0/24 or 194.153.145.0/24 or 195.138.128.0/19 or 195.212.63.0/24 or 195.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.56.0/19 or 212.7.192.0/19 or 212.72.192.0/19 or 212.91.160.0/19 or 212.95.160.0/19 or 213.130.64.0/19 or 213.137.32.0/19 or 213.145.96.0/19 or 213.16.32.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.240.0/20 or 217.18.240.0/20 or 217.145.160.0/20 or 217.197.128.0/20 or 217.79.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
> ===================================================================
> 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;
> }
>
> +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 rule.
> * Rules are added into the 'rulebuf' and then copied in the correct order
> @@ -2562,6 +2570,7 @@
> NEED1("missing action");
> i = match_token(rule_actions, *av);
> ac--; av++;
> + check_length(action, 1, actbuf, sizeof(actbuf)); /* superfluous.. */
> action->len = 1; /* default */
> switch(i) {
> case TOK_CHECKSTATE:
> @@ -2602,6 +2611,7 @@
> case TOK_QUEUE:
> case TOK_PIPE:
> action->len = F_INSN_SIZE(ipfw_insn_pipe);
> + check_length(action, action->len, actbuf, sizeof(actbuf));
> case TOK_SKIPTO:
> if (i == TOK_QUEUE)
> action->opcode = O_QUEUE;
> @@ -2639,6 +2649,7 @@
>
> action->opcode = O_FORWARD_IP;
> action->len = F_INSN_SIZE(ipfw_insn_sa);
> + check_length(action, action->len, actbuf, sizeof(actbuf));
>
> p->sa.sin_len = sizeof(struct sockaddr_in);
> p->sa.sin_family = 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 = next_cmd(action);
>
> /*
> @@ -2687,6 +2699,7 @@
> errx(EX_DATAERR, "logamount must be positive");
> ac--; av++;
> }
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> }
>
> @@ -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 = next_cmd(cmd);
> ac -= 2; av += 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 = next_cmd(cmd);
> + }
> ac--; av++; /* any or mac-type */
> goto read_options;
> }
> @@ -2775,6 +2792,7 @@
> else {
> proto = cmd->arg1;
> prev = cmd;
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> }
> } else if (first_cmd != cmd) {
> @@ -2800,6 +2818,7 @@
> ac--; av++;
> if (F_LEN(cmd) != 0) { /* ! any */
> prev = cmd;
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> }
> }
> @@ -2814,7 +2833,10 @@
> add_ports(cmd, *av, proto, O_IP_SRCPORT)) {
> ac--; av++;
> if (F_LEN(cmd) != 0)
> + {
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> + }
> }
> }
>
> @@ -2835,6 +2857,7 @@
> ac--; av++;
> if (F_LEN(cmd) != 0) { /* ! any */
> prev = cmd;
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> }
> }
> @@ -2849,7 +2872,10 @@
> add_ports(cmd, *av, proto, O_IP_DSTPORT)) {
> ac--; av++;
> if (F_LEN(cmd) != 0)
> + {
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> + }
> }
> }
>
> @@ -3167,6 +3193,7 @@
> }
> if (F_LEN(cmd) > 0) { /* prepare to advance */
> prev = cmd;
> + check_length(cmd, F_LEN(cmd), cmdbuf, sizeof(cmdbuf));
> cmd = next_cmd(cmd);
> }
> }
> @@ -3187,6 +3214,7 @@
> if (match_prob != 1) { /* 1 means always match */
> dst->opcode = O_PROB;
> dst->len = 2;
> + check_length(dst, 2, rulebuf, sizeof(rulebuf));
> *((int32_t *)(dst+1)) = (int32_t)(match_prob * 0x7fffffff);
> dst += dst->len;
> }
> @@ -3196,6 +3224,7 @@
> */
> if (have_state && have_state->opcode != O_CHECK_STATE) {
> fill_cmd(dst, O_PROBE_STATE, 0, 0);
> + check_length(dst, F_LEN(dst), rulebuf, sizeof(rulebuf));
> dst = 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 += i;
> }
> @@ -3220,6 +3250,7 @@
> */
> if (have_state && have_state->opcode != O_CHECK_STATE) {
> i = F_LEN(have_state);
> + check_length(dst, i, rulebuf, sizeof(rulebuf));
> bcopy(have_state, dst, i * sizeof(u_int32_t));
> dst += i;
> }
> @@ -3234,6 +3265,7 @@
> src = (ipfw_insn *)cmdbuf;
> if ( src->opcode == O_LOG ) {
> i = F_LEN(src);
> + check_length(dst, i, rulebuf, sizeof(rulebuf));
> bcopy(src, dst, i * sizeof(u_int32_t));
> dst += i;
> }
> @@ -3242,6 +3274,7 @@
> */
> for (src = (ipfw_insn *)actbuf; src != action; src += i) {
> i = F_LEN(src);
> + check_length(dst, i, rulebuf, sizeof(rulebuf));
> bcopy(src, dst, i * sizeof(u_int32_t));
> dst += i;
> }
More information about the freebsd-ipfw
mailing list