Re: [PATCH] Bitwise Out Of Bound Read in Netfilter Conntrack

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Eric,

Is there any change with regards to previous patch?

I have two in patchwork:

http://patchwork.ozlabs.org/patch/823074/
http://patchwork.ozlabs.org/patch/825682/

Which one should I consider for submission?

Thanks!

On Fri, Oct 13, 2017 at 08:29:08PM +0200, Eric Sesterhenn wrote:
> 
> From 66a5a55b6e7e45c51691583cf5833b4fe9365dc1 Mon Sep 17 00:00:00 2001
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> Date: Fri, 13 Oct 2017 20:26:32 +0200
> Subject: [PATCH] Prevent bitwise out of bound memory reads
> 
> The CHECK_BOUND function is not always uses, especially not
> when reading bitwise.
> 
> Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> ---
>  net/netfilter/nf_conntrack_h323_asn1.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index 89b2e46925c4..e298878edc86 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -104,6 +104,7 @@ typedef struct {
>  #define INC_BITS(bs,b)
> if(((bs)->bit+=(b))>7){(bs)->cur+=(bs)->bit>>3;(bs)->bit&=7;}
>  #define BYTE_ALIGN(bs) if((bs)->bit){(bs)->cur++;(bs)->bit=0;}
>  #define CHECK_BOUND(bs,n)
> if((bs)->cur+(n)>(bs)->end)return(H323_ERROR_BOUND)
> +#define CHECK_BIT_BOUND(bs,n) ({ size_t __tmp = n/8;
> if((bs)->bit+(n%8)>7) { CHECK_BOUND(bs, __tmp + 2); } else {
> CHECK_BOUND(bs, __tmp + 1); } })
>  static unsigned int get_len(bitstr_t *bs);
>  static unsigned int get_bit(bitstr_t *bs);
>  static unsigned int get_bits(bitstr_t *bs, unsigned int b);
> @@ -319,9 +320,11 @@ static int decode_int(bitstr_t *bs, const struct
> field_t *f,
>  		bs->cur += 2;
>  		break;
>  	case CONS:		/* 64K < Range < 4G */
> +		CHECK_BIT_BOUND(bs, 2)
>  		len = get_bits(bs, 2) + 1;
>  		BYTE_ALIGN(bs);
>  		if (base && (f->attr & DECODE)) {	/* timeToLive */
> +			CHECK_BOUND(bs, len);
>  			unsigned int v = get_uint(bs, len) + f->lb;
>  			PRINT(" = %u", v);
>  			*((unsigned int *)(base + f->offset)) = v;
> @@ -404,6 +407,7 @@ static int decode_numstr(bitstr_t *bs, const struct
> field_t *f,
>  	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
> 
>  	/* 2 <= Range <= 255 */
> +	CHECK_BIT_BOUND(bs, f->sz);
>  	len = get_bits(bs, f->sz) + f->lb;
> 
>  	BYTE_ALIGN(bs);
> @@ -449,6 +453,7 @@ static int decode_octstr(bitstr_t *bs, const struct
> field_t *f,
>  		len = get_len(bs) + f->lb;
>  		break;
>  	default:		/* 2 <= Range <= 255 */
> +		CHECK_BIT_BOUND(bs, f->sz);
>  		len = get_bits(bs, f->sz) + f->lb;
>  		BYTE_ALIGN(bs);
>  		break;
> @@ -477,6 +482,7 @@ static int decode_bmpstr(bitstr_t *bs, const struct
> field_t *f,
>  		len = (*bs->cur++) + f->lb;
>  		break;
>  	default:		/* 2 <= Range <= 255 */
> +		CHECK_BIT_BOUND(bs, f->sz);
>  		len = get_bits(bs, f->sz) + f->lb;
>  		BYTE_ALIGN(bs);
>  		break;
> @@ -503,9 +509,11 @@ static int decode_seq(bitstr_t *bs, const struct
> field_t *f,
>  	base = (base && (f->attr & DECODE)) ? base + f->offset : NULL;
> 
>  	/* Extensible? */
> +	CHECK_BIT_BOUND(bs, 1);
>  	ext = (f->attr & EXT) ? get_bit(bs) : 0;
> 
>  	/* Get fields bitmap */
> +	CHECK_BIT_BOUND(bs, f->sz);
>  	bmp = get_bitmap(bs, f->sz);
>  	if (base)
>  		*(unsigned int *)base = bmp;
> @@ -555,8 +563,9 @@ static int decode_seq(bitstr_t *bs, const struct
> field_t *f,
>  		return H323_ERROR_NONE;
> 
>  	/* Get the extension bitmap */
> +	CHECK_BIT_BOUND(bs, 7);
>  	bmp2_len = get_bits(bs, 7) + 1;
> -	CHECK_BOUND(bs, (bmp2_len + 7) >> 3);
> +	CHECK_BIT_BOUND(bs, bmp2_len);
>  	bmp2 = get_bitmap(bs, bmp2_len);
>  	bmp |= bmp2 >> f->sz;
>  	if (base)
> @@ -639,6 +648,7 @@ static int decode_seqof(bitstr_t *bs, const struct
> field_t *f,
>  		count = get_len(bs);
>  		break;
>  	default:
> +		CHECK_BIT_BOUND(bs, f->sz);
>  		count = get_bits(bs, f->sz);
>  		break;
>  	}
> @@ -658,6 +668,7 @@ static int decode_seqof(bitstr_t *bs, const struct
> field_t *f,
>  	for (i = 0; i < count; i++) {
>  		if (son->attr & OPEN) {
>  			BYTE_ALIGN(bs);
> +			CHECK_BOUND(bs, 2);
>  			len = get_len(bs);
>  			CHECK_BOUND(bs, len);
>  			if (!base || !(son->attr & DECODE)) {
> @@ -710,11 +721,14 @@ static int decode_choice(bitstr_t *bs, const
> struct field_t *f,
>  	base = (base && (f->attr & DECODE)) ? base + f->offset : NULL;
> 
>  	/* Decode the choice index number */
> +	CHECK_BIT_BOUND(bs, 1);
>  	if ((f->attr & EXT) && get_bit(bs)) {
>  		ext = 1;
> +		CHECK_BIT_BOUND(bs, 7);
>  		type = get_bits(bs, 7) + f->lb;
>  	} else {
>  		ext = 0;
> +		CHECK_BIT_BOUND(bs, f->sz);
>  		type = get_bits(bs, f->sz);
>  		if (type >= f->lb)
>  			return H323_ERROR_RANGE;
> @@ -727,6 +741,7 @@ static int decode_choice(bitstr_t *bs, const struct
> field_t *f,
>  	/* Check Range */
>  	if (type >= f->ub) {	/* Newer version? */
>  		BYTE_ALIGN(bs);
> +		CHECK_BOUND(bs, 2);
>  		len = get_len(bs);
>  		CHECK_BOUND(bs, len);
>  		bs->cur += len;
> @@ -742,6 +757,7 @@ static int decode_choice(bitstr_t *bs, const struct
> field_t *f,
> 
>  	if (ext || (son->attr & OPEN)) {
>  		BYTE_ALIGN(bs);
> +		CHECK_BOUND(bs, 2);
>  		len = get_len(bs);
>  		CHECK_BOUND(bs, len);
>  		if (!base || !(son->attr & DECODE)) {
> 
> 
> -- 
> Eric Sesterhenn (Principal Security Consultant)
> X41 D-SEC GmbH, Dennewartstr. 25-27, D-52068 Aachen
> T: +49 241 9809418-0, Fax: -9
> Unternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989
> Geschäftsführer: Markus Vervier
>