Subject: Re: kern/32198: bpf_validate() needs to do more checks
To: Guy Harris <firstname.lastname@example.org>
From: Rui Paulo <email@example.com>
Date: 11/30/2005 20:00:54
On 2005.11.30 10:56:59 -0800, Guy Harris wrote:
| Rui Paulo wrote:
| >On 2005.11.30 11:42:00 +0000, firstname.lastname@example.org 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
| >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
| >| 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
| > 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
IMHO this is not needed and the code already differs from the three
-- Rui Paulo