Subject: Re: kern/32198: bpf_validate() needs to do more checks
To: None <rpaulo@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org>
From: Guy Harris <guy@alum.mit.edu>
List: netbsd-bugs
Date: 11/30/2005 19:30:04
The following reply was made to PR kern/32198; it has been noted by GNATS.

From: Guy Harris <guy@alum.mit.edu>
To: Rui Paulo <rpaulo@fnop.net>
Cc: gnats-bugs@netbsd.org, kern-bug-people@netbsd.org,
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/32198: bpf_validate() needs to do more checks
Date: Wed, 30 Nov 2005 10:56:59 -0800

 Rui Paulo wrote:
 > On 2005.11.30 11:42:00 +0000, guy@alum.mit.edu wrote:
 > | >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;
 >
 > This is done in bpf_setf();
 >   
 The check for no instructions isn't - is that check necessary?
 > |         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).
 >
 > What's wrong with the current checks in bpf_validate() ?
 >   
 If that question applies to both of those items:
 
 For the first item, to quote a message from Dawson Engler:
 > we're developing a symbolic execution tool to find bugs in code and it
 > seems to have turned up two errors in recent BPF code that appear to
 > let filters read/write arbitrary memory.  Vern recommended that I ask
 > you guys if these were legitimate bugs.
 >
 > If you have a filter with either:
 >
 >         code == BPF_STX;
 >   or
 >         code == BPF_LDX|BPF_MEM;
 >
 > then from what I can tell with testing bpf_validate forgets to check
 > their associated offset stored in field "k" (it does check
 > for BPF_ST and BPF_LD but not the *X versions):
 >
 >                  * 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;
 >
 > then the interpreter lets you read/write arbitrary offsets off of
 > mem:
 >
 >                 case BPF_LDX|BPF_MEM:
 >                         X = mem[pc->k];
 >                         continue;
 >
 >                 case BPF_STX:
 >                         mem[pc->k] = X;
 >                         continue;
 Did he miss something, or is that, indeed, a risk?
 
 For the second item:
 
 The install-time check isn't necessary, but it makes the code more 
 closely resemble the code in OpenBSD, to make keeping the code in sync 
 easier.