Date: Wed, 30 Nov 2005 10:11:39 GMT From: Guy Harris <guy@alum.mit.edu> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/89752: bpf_validate() needs to do more checks Message-ID: <200511301011.jAUABd8F079721@www.freebsd.org> Resent-Message-ID: <200511301020.jAUAK2ji030786@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 89752 >Category: kern >Synopsis: bpf_validate() needs to do more checks >Confidential: no >Severity: serious >Priority: medium >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Nov 30 10:20:02 GMT 2005 >Closed-Date: >Last-Modified: >Originator: Guy Harris >Release: >Organization: >Environment: >Description: OpenBSD's bpf_validate() in sys/net/bpf_filter.c does some additional checks to catch: BPF programs with no instructions or with more than BPF_MAXINSNS instructions; BPF_STX and BPF_LDX|BPF_MEM instructions that have out-of-range offsets (which could be made to fetch or store into arbitrary memory locations); BPF_DIV instructions with a constant 0 divisor (that's a check also done at run time). Here's a patch to make the FreeBSD bpf_validate() match it (with some changes to comments; I've submitted the changes to comments to the OpenBSD bug database). >How-To-Repeat: >Fix: Index: bpf_filter.c =================================================================== RCS file: /home/ncvs/src/sys/net/bpf_filter.c,v retrieving revision 1.23 diff -c -r1.23 bpf_filter.c *** bpf_filter.c 7 Jan 2005 01:45:34 -0000 1.23 --- bpf_filter.c 30 Nov 2005 09:57:08 -0000 *************** *** 507,513 **** /* * Return true if the 'fcode' is a valid filter program. * The constraints are that each jump be forward and to a valid ! * code. The code must terminate with either an accept or reject. * * The kernel needs to be able to verify an application's filter code. * Otherwise, a bogus program could easily crash the system. --- 507,516 ---- /* * Return true if the 'fcode' is a valid filter program. * The constraints are that each jump be forward and to a valid ! * code, that memory accesses are within valid ranges (to the ! * extent that this can be checked statically; loads of packet ! * data have to be, and are, also checked at run time), and that ! * the code terminates with either an accept or reject. * * The kernel needs to be able to verify an application's filter code. * Otherwise, a bogus program could easily crash the system. *************** *** 517,555 **** const struct bpf_insn *f; int len; { ! register int i; register const struct bpf_insn *p; for (i = 0; i < len; ++i) { /* ! * Check that that jumps are forward, and within ! * the code block. */ ! p = &f[i]; ! if (BPF_CLASS(p->code) == BPF_JMP) { ! register int from = i + 1; ! ! if (BPF_OP(p->code) == BPF_JA) { ! if (from >= len || p->k >= len - from) return 0; } ! else if (from >= len || p->jt >= len - from || ! p->jf >= len - from) return 0; ! } ! /* ! * Check that memory operations use valid addresses. ! */ ! if ((BPF_CLASS(p->code) == BPF_ST || ! (BPF_CLASS(p->code) == BPF_LD && ! (p->code & 0xe0) == BPF_MEM)) && ! p->k >= BPF_MEMWORDS) ! return 0; ! /* ! * Check for constant division by 0. ! */ ! if (p->code == (BPF_ALU|BPF_DIV|BPF_K) && p->k == 0) return 0; } return BPF_CLASS(f[len - 1].code) == BPF_RET; } --- 520,628 ---- const struct bpf_insn *f; int len; { ! u_int i, from; register const struct bpf_insn *p; + if (len < 1 || len > BPF_MAXINSNS) + return 0; + for (i = 0; i < len; ++i) { + p = &f[i]; + switch (BPF_CLASS(p->code)) { /* ! * Check that memory operations use valid addresses. */ ! case BPF_LD: ! case BPF_LDX: ! switch (BPF_MODE(p->code)) { ! case BPF_IMM: ! break; ! case BPF_ABS: ! case BPF_IND: ! case BPF_MSH: ! /* ! * More strict check with actual packet length ! * is done runtime. ! */ ! if (p->k >= bpf_maxbufsize) ! return 0; ! break; ! case BPF_MEM: ! if (p->k >= BPF_MEMWORDS) return 0; + break; + case BPF_LEN: + break; + default: + return 0; } ! break; ! case BPF_ST: ! case BPF_STX: ! if (p->k >= BPF_MEMWORDS) return 0; ! break; ! case BPF_ALU: ! switch (BPF_OP(p->code)) { ! case BPF_ADD: ! case BPF_SUB: ! case BPF_OR: ! case BPF_AND: ! case BPF_LSH: ! case BPF_RSH: ! case BPF_NEG: ! break; ! case BPF_DIV: ! /* ! * Check for constant division by 0. ! */ ! if (BPF_RVAL(p->code) == BPF_K && p->k == 0) ! return 0; ! default: ! return 0; ! } ! break; ! case BPF_JMP: ! /* ! * Check that jumps are within the code block, ! * and that unconditional branches don't go ! * backwards as a result of an overflow. ! * Unconditional branches have a 32-bit offset, ! * so they could overflow; we check to make ! * sure they don't. Conditional branches have ! * an 8-bit offset, and the from address is <= ! * BPF_MAXINSNS, and we assume that BPF_MAXINSNS ! * is sufficiently small that adding 255 to it ! * won't overflow. ! * ! * We know that len is <= BPF_MAXINSNS, and we ! * assume that BPF_MAXINSNS is < the maximum size ! * of a u_int, so that i + 1 doesn't overflow. ! */ ! from = i + 1; ! switch (BPF_OP(p->code)) { ! case BPF_JA: ! if (from + p->k < from || from + p->k >= len) ! return 0; ! break; ! case BPF_JEQ: ! case BPF_JGT: ! case BPF_JGE: ! case BPF_JSET: ! if (from + p->jt >= len || from + p->jf >= len) ! return 0; ! break; ! default: ! return 0; ! } ! break; ! case BPF_RET: ! break; ! case BPF_MISC: ! break; ! default: return 0; + } } return BPF_CLASS(f[len - 1].code) == BPF_RET; } >Release-Note: >Audit-Trail: >Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200511301011.jAUABd8F079721>