On 2005.11.30 10:56:59 -0800, Guy Harris wrote:
| 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?

Ah, it returns EINVAL but still does an out-of-bounds access in
bpf_validate if len < 1...

| >|         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?

Oh, that's really a problem.

| 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.

IMHO this is not needed and the code already differs from the three
BSDs.

		-- Rui Paulo