Bug 12484 - [PATCH] bpf_filter() broken
Summary: [PATCH] bpf_filter() broken
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 3.2-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: FreeBSD bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 1999-07-02 04:00 UTC by leres
Modified: 2001-07-21 18:30 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description leres 1999-07-02 04:00:01 UTC
	Revision 1.12 of sys/bpf_filter.c breaks certain packet filters.

Fix: The appended context changes are against 1.13 (it looks like
	3.2-RELEASE shipped with 1.12).

RCS file: RCS/bpf_filter.c,v
retrieving revision 1.3


-----BEGIN PGP SIGNATURE-----
Version: 2.6.2

iQCVAwUBN3wpAr2JLbUEFcrxAQFg8wP8Cs1hzHa/s6VjDMQFy2SSbM8oH5lr272A
/MMSl8tlreDcjZUcIhndB8DwVaQlTgMkZG+sAuxvCh4gglFOczxwk/oeikNuUFlt
oCahmi/LjnTPAn/VRKvvw+LPA21mZ940V87sOdqhpE/PtOc5s8zNa0oGP0KWXsb/
3IHfY3c49qI=
=I7G8
-----END PGP SIGNATURE-------C8laoFVYrbqX8AHg2iT27OxbSfSC4NbtIePXAmuJS44UieE0
Content-Type: text/plain; name="file.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="file.diff"

diff -c -r1.3 bpf_filter.c
*** /tmp/,RCSt1z29845	Thu Jul  1 19:43:31 1999
- --- bpf_filter.c	Thu Jul  1 19:42:40 1999
***************
*** 217,223 ****
  
  		case BPF_LD|BPF_W|BPF_ABS:
  			k = pc->k;
! 			if (k > buflen || sizeof(int32_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
- --- 217,223 ----
  
  		case BPF_LD|BPF_W|BPF_ABS:
  			k = pc->k;
! 			if (sizeof(int32_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
***************
*** 241,247 ****
  
  		case BPF_LD|BPF_H|BPF_ABS:
  			k = pc->k;
! 			if (k > buflen || sizeof(int16_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
- --- 241,247 ----
  
  		case BPF_LD|BPF_H|BPF_ABS:
  			k = pc->k;
! 			if (sizeof(int16_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
***************
*** 285,292 ****
  
  		case BPF_LD|BPF_W|BPF_IND:
  			k = X + pc->k;
! 			if (pc->k > buflen || X > buflen - pc->k ||
! 			    sizeof(int32_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
- --- 285,291 ----
  
  		case BPF_LD|BPF_W|BPF_IND:
  			k = X + pc->k;
! 			if (sizeof(int32_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
***************
*** 310,317 ****
  
  		case BPF_LD|BPF_H|BPF_IND:
  			k = X + pc->k;
! 			if (X > buflen || pc->k > buflen - X ||
! 			    sizeof(int16_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
- --- 309,315 ----
  
  		case BPF_LD|BPF_H|BPF_IND:
  			k = X + pc->k;
! 			if (sizeof(int16_t) > buflen - k) {
  #ifdef KERNEL
  				int merr;
  
***************
*** 330,336 ****
  
  		case BPF_LD|BPF_B|BPF_IND:
  			k = X + pc->k;
! 			if (pc->k >= buflen || X >= buflen - k) {
  #ifdef KERNEL
  				register struct mbuf *m;
  
- --- 328,334 ----
  
  		case BPF_LD|BPF_B|BPF_IND:
  			k = X + pc->k;
! 			if (k >= buflen) {
  #ifdef KERNEL
  				register struct mbuf *m;
  
How-To-Repeat: 
	We noticed after upgrading our security system from
	3.0-980414-SNAP to 3.2-RELEASE that the tcpdump filter:
	
	    'tcp[13] & 0x7 != 0'

	would not capture such packets. Debugging turned up a problem
	in this case:

	    case BPF_LD|BPF_B|BPF_IND:
		    k = X + pc->k;
		    if (pc->k >= buflen || X >= buflen - k) {
	
	The conditional can be rewritten with "k" expanded:

		    if (pc->k >= buflen || X >= buflen - (X + pc->k)) {

	which is the same as:

		    if (pc->k >= buflen || X + X + pc->k >= buflen) {
	
	and is clearly wrong.

	Other cases where "k" is initialized to "X + pc->k" appear to
	either be broken or at least contain unnecessary checks.

	We consider this a serious bug because it breaks security
	monitoring software (including our "bro" package).
Comment 1 bde 1999-07-02 08:00:13 UTC
>	would not capture such packets. Debugging turned up a problem
>	in this case:
>
>	    case BPF_LD|BPF_B|BPF_IND:
>		    k = X + pc->k;
>		    if (pc->k >= buflen || X >= buflen - k) {
>	
>	The conditional can be rewritten with "k" expanded:
>
>		    if (pc->k >= buflen || X >= buflen - (X + pc->k)) {
>
>	which is the same as:
>
>		    if (pc->k >= buflen || X + X + pc->k >= buflen) {
>	
>	and is clearly wrong.

Oops.  This should have been:

		if (pc->k >= buflen || X >= buflen - p->k) {

which is the same modulo possible truncation mod 2^32 as each of the
following:

		if (pc->k >= buflen || X + p->k >= buflen) {
		if (pc->k >= buflen || k >= buflen) {
		if (k >= buflen) {

The latter is the conditinal in rev.1.11.

>	Other cases where "k" is initialized to "X + pc->k" appear to
>	either be broken or at least contain unnecessary checks.

The other cases seem to be correct.  The extra checks relative to 1.11
are to detect truncation for weird values of X and/or pc->k, e.g., X =
4, pc->k = (u_int32_t)-5 gives X + pc->k = 3 if u_int32_t is u_int.
I don't know if this much overflow checking is justified.

>	The appended context changes are against 1.13 (it looks like
>	3.2-RELEASE shipped with 1.12).
>
>RCS file: RCS/bpf_filter.c,v
>retrieving revision 1.3
>diff -c -r1.3 bpf_filter.c
>*** /tmp/,RCSt1z29845	Thu Jul  1 19:43:31 1999
>- --- bpf_filter.c	Thu Jul  1 19:42:40 1999
>***************
>*** 217,223 ****
>  
>  		case BPF_LD|BPF_W|BPF_ABS:
>  			k = pc->k;
>! 			if (k > buflen || sizeof(int32_t) > buflen - k) {
>  #ifdef KERNEL
>  				int merr;
>  
>- --- 217,223 ----
>  
>  		case BPF_LD|BPF_W|BPF_ABS:
>  			k = pc->k;
>! 			if (sizeof(int32_t) > buflen - k) {
>  #ifdef KERNEL
>  				int merr;
>  

If you reduce the amount of overflow checking back to 1.11 levels, then
change the checks back to the ones in 1.11. e.g.,

 			if (k + sizeof(int32_t) > buflen) {

The check:

			if (sizeof(int32_t) > buflen - k) {

has fatal overflow problems if k > buflen and u_int32_t is u_int.

Bruce
Comment 2 mike freebsd_committer 2001-07-21 00:11:34 UTC
State Changed
From-To: open->feedback


Does this problem still occur in newer versions of FreeBSD, 
such as 4.3-RELEASE?
Comment 3 mike freebsd_committer 2001-07-21 18:18:45 UTC
Adding this to the Audit-Trail.

On Sat, Jul 21, 2001 at 10:40:39PM +1000, Bruce Evans wrote:
> I kept the following private mail about this:
> 
> > From bde@zeta.org.au Wed Aug 16 03:34:22 2000 +1000
> > Date: Wed, 16 Aug 2000 03:34:20 +1000 (EST)
> > From: Bruce Evans <bde@zeta.org.au>
> > X-Sender: bde@besplex.bde.org
> > To: Johan Karlsson <k@numeri.campus.luth.se>
> > Subject: Re: PR 12484 and your commit rev 1.14 of src/sys/net/bpf_filter.c
> > In-Reply-To: <200008150757.JAA13997@numeri.campus.luth.se>
> > Message-ID: <Pine.BSF.4.21.0008160316500.2104-100000@besplex.bde.org>
> > MIME-Version: 1.0
> > Content-Type: TEXT/PLAIN; charset=US-ASCII
> > Status: O
> > X-Status: 
> > X-Keywords:                  
> > X-UID: 859
> > 
> > On Tue, 15 Aug 2000, Johan Karlsson wrote:
> > 
> > > Hi Bruce
> > > It seems you commited a fix for PR 12484 in rev 1.14 of 
> > > src/sys/net/bpf_filter.c.
> > > 
> > > Can the PR be closed or should the 
> > > "inconsistent and probably unnecessarily pessimal" checks
> > > be fixed first?
> > 
> > I don't know bpf well enough to be sure of more than the formal
> > correctness of the fix.  The people on the Cc line of the PR
> > should know :-).  Craig Leres didn't seem to have any objections.
> > 
> > The efficiency issues were more interesting on old (slow) hardware.
> > 
> > PR 16644 is also about this.  Its author is also at LBL, but seems
> > to be as confused as usual.
> > 
> > Bruce
> 
> Bruce
>
Comment 4 mike freebsd_committer 2001-07-21 18:29:25 UTC
State Changed
From-To: feedback->closed


I'm satisfied the originator's problem has been solved.