[PATCH] Out Of Bound Read in Netfilter Conntrack

[PATCH] Out Of Bound Read in Netfilter Conntrack

[Eric Sesterhenn]

Add missing counter decrement to prevent out of bounds memory read.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
b/net/netfilter/nf_conntrack_h323_asn1.c
index 89b2e46925c4..2a9d1acd0cbd 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -877,6 +877,7 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931
*q931)
 		if (sz < 1)
 			break;
 		len = *p++;
+		sz--;
 		if (sz < len)
 			break;
 		p += len;

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

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

[Florian Westphal]

Eric Sesterhenn <eric.sesterhenn@x41-dsec.de> wrote:
> Add missing counter decrement to prevent out of bounds memory read.
> 
> Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> 
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index 89b2e46925c4..2a9d1acd0cbd 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -877,6 +877,7 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931
> *q931)
>  		if (sz < 1)
>  			break;
>  		len = *p++;
> +		sz--;
>  		if (sz < len)
>  			break;
>  		p += len;

LGTM.
Acked-by: Florian Westphal <fw@strlen.de>

[PATCH] Bitwise Out Of Bound Read in Netfilter Conntrack

[Eric Sesterhenn]

[-- Attachment #1.1: Type: text/plain, Size: 5009 bytes --]


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pablo Neira Ayuso]

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
> 

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

[Eric Sesterhenn]

[-- Attachment #1.1: Type: text/plain, Size: 6194 bytes --]

Hello Pablo,

they fix different issues, therefore both should
be applied.

Regards,
Eric

On 17/10/17 15:09, Pablo Neira Ayuso wrote:
> 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
>>
> 
> 
> 
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pablo Neira Ayuso]

On Tue, Oct 17, 2017 at 03:48:13PM +0200, Eric Sesterhenn wrote:
> Hello Pablo,
> 
> they fix different issues, therefore both should
> be applied.

OK, thanks for explaining. Let me get back to this asap.

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

[Pablo Neira Ayuso]

Hi,

On Mon, Oct 09, 2017 at 07:01:14AM +0200, Eric Sesterhenn wrote:
> Add missing counter decrement to prevent out of bounds memory read.

I have manually applied this. However, your patches are mangled most
likely by MUA and patchword doesn't get it right.

If you could please revamp and make sure patches apply cleanly via:

        git am patch

I would really appreciate, thanks for your patience!

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

[Pablo Neira Ayuso]

On Tue, Oct 24, 2017 at 06:29:03PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Mon, Oct 09, 2017 at 07:01:14AM +0200, Eric Sesterhenn wrote:
> > Add missing counter decrement to prevent out of bounds memory read.
> 
> I have manually applied this. However, your patches are mangled most
> likely by MUA and patchword doesn't get it right.
> 
> If you could please revamp and make sure patches apply cleanly via:
> 
>         git am patch
> 
> I would really appreciate, thanks for your patience!

While at it, please prepend to your patches:

        netfilter: nf_ct_h323: ...

So we all know what subsystems this goes to.

Thanks a lot!

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

[Eric Sesterhenn]

[-- Attachment #1.1.1: Type: text/plain, Size: 1147 bytes --]

Hi Pablo,

On 24/10/17 18:36, Pablo Neira Ayuso wrote:
> On Tue, Oct 24, 2017 at 06:29:03PM +0200, Pablo Neira Ayuso wrote:
>> Hi,
>>
>> On Mon, Oct 09, 2017 at 07:01:14AM +0200, Eric Sesterhenn wrote:
>>> Add missing counter decrement to prevent out of bounds memory read.
>>
>> I have manually applied this. However, your patches are mangled most
>> likely by MUA and patchword doesn't get it right.
>>
>> If you could please revamp and make sure patches apply cleanly via:
>>
>>         git am patch
>>
>> I would really appreciate, thanks for your patience!
>
I tried to attach the patches in order to keep thunderbird
from mangling them. Guess its time to set up a second mua
for mailing patches.

> While at it, please prepend to your patches:
> 
>         netfilter: nf_ct_h323: ...
> 
> So we all know what subsystems this goes to.

As in the patches attached?

Regards,
Eric

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-Out-Of-Bound-Read-in-Netfilter-Conntrack.patch --]
[-- Type: text/x-patch; name="0001-Out-Of-Bound-Read-in-Netfilter-Conntrack.patch", Size: 893 bytes --]

From b8ed8753ca82f6f07fce2901418aab531d98ee39 Mon Sep 17 00:00:00 2001
From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
Date: Wed, 25 Oct 2017 08:32:57 +0200
Subject: [PATCH netfilter: nf_ct_h323: 1/2] Out Of Bound Read in Netfilter
 Conntrack

Add missing counter decrement to prevent out of bounds memory read.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 89b2e46925c4..2a9d1acd0cbd 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -877,6 +877,7 @@ int DecodeQ931(unsigned char *buf, size_t sz, Q931 *q931)
 		if (sz < 1)
 			break;
 		len = *p++;
+		sz--;
 		if (sz < len)
 			break;
 		p += len;
-- 
2.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.3: 0002-Prevent-multiple-out-of-bounds-memory-reads.patch --]
[-- Type: text/x-patch; name="0002-Prevent-multiple-out-of-bounds-memory-reads.patch", Size: 4664 bytes --]

From c1b7044749e534207ecd3b04281ae024b01887d3 Mon Sep 17 00:00:00 2001
From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
Date: Wed, 25 Oct 2017 08:39:38 +0200
Subject: [PATCH netfilter: nf_ct_h323: 2/2] Prevent multiple out of bounds
 memory reads.

Multiple accesses are not guarded by out of bound
checks. This patch introduces them.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 2a9d1acd0cbd..78a218cdf04e 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,6 +320,7 @@ 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 */
@@ -404,6 +406,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 +452,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 +481,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 +508,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 +562,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 +647,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 +667,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 +720,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 +740,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 +756,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)) {
-- 
2.11.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pablo Neira Ayuso]

Hi Eric,

On Wed, Oct 25, 2017 at 09:05:05AM +0200, Eric Sesterhenn wrote:
[...]
> From b8ed8753ca82f6f07fce2901418aab531d98ee39 Mon Sep 17 00:00:00 2001
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> Date: Wed, 25 Oct 2017 08:32:57 +0200
> Subject: [PATCH netfilter: nf_ct_h323: 1/2] Out Of Bound Read in Netfilter
>  Conntrack
> 
> Add missing counter decrement to prevent out of bounds memory read.

This one, I already applied it, see below comment on 2/2.

> From c1b7044749e534207ecd3b04281ae024b01887d3 Mon Sep 17 00:00:00 2001
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> Date: Wed, 25 Oct 2017 08:39:38 +0200
> Subject: [PATCH netfilter: nf_ct_h323: 2/2] Prevent multiple out of bounds
>  memory reads.
> 
> Multiple accesses are not guarded by out of bound
> checks. This patch introduces them.
> 
> Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> ---
>  net/netfilter/nf_conntrack_h323_asn1.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 2a9d1acd0cbd..78a218cdf04e 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); } })

CHECK_BOUND() and your new CHECK_BIT_BOUND() are returning a something
inside a macro, which is a bad practise.

Would you first send me a patch to replace CHECK_BOUND() by a
function, then add place your fix on top of it?

I'd suggest something like:

        static inline int nf_h323_error_boundary(...)
        {
                return bs->cur + (n > bs->end);
        }

Then, use it:

        if (nf_h323_error_boundary(...))
                return H323_ERROR_BOUND;

Please, I'd appreciate if you can send me patches via git-send-mail
too.

Thanks!

[PATCH 1/2] Convert CHECK_BOUND macro to function

[eric.sesterhenn]

From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>

It is bad practive to return in a macro, this patch
moves the check into a function.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 94 +++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 29 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 89b2e46925c4..f358222b1e5e 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -103,7 +103,6 @@ typedef struct {
 #define INC_BIT(bs) if((++(bs)->bit)>7){(bs)->cur++;(bs)->bit=0;}
 #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)
 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);
@@ -166,6 +165,14 @@ static unsigned int get_len(bitstr_t *bs)
 }
 
 /****************************************************************************/
+static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)
+{
+	if(*bs->cur + bytes > *bs->end)
+		return 1;
+	return 0;
+}
+
+/****************************************************************************/
 static unsigned int get_bit(bitstr_t *bs)
 {
 	unsigned int b = (*bs->cur) & (0x80 >> bs->bit);
@@ -280,7 +287,8 @@ static int decode_bool(bitstr_t *bs, const struct field_t *f,
 
 	INC_BIT(bs);
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -293,11 +301,14 @@ static int decode_oid(bitstr_t *bs, const struct field_t *f,
 	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
 
 	BYTE_ALIGN(bs);
-	CHECK_BOUND(bs, 1);
+	if (nf_h323_error_boundary(bs, 1))
+		return H323_ERROR_BOUND;
+
 	len = *bs->cur++;
 	bs->cur += len;
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 
-	CHECK_BOUND(bs, 0);
 	return H323_ERROR_NONE;
 }
 
@@ -330,7 +341,8 @@ static int decode_int(bitstr_t *bs, const struct field_t *f,
 		break;
 	case UNCO:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		bs->cur += len;
 		break;
@@ -341,7 +353,8 @@ static int decode_int(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -357,7 +370,8 @@ static int decode_enum(bitstr_t *bs, const struct field_t *f,
 		INC_BITS(bs, f->sz);
 	}
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -375,12 +389,14 @@ static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 		len = f->lb;
 		break;
 	case WORD:		/* 2-byte length */
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) << 8;
 		len += (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		break;
 	default:
@@ -391,7 +407,8 @@ static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 	bs->cur += len >> 3;
 	bs->bit = len & 7;
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -409,7 +426,8 @@ static int decode_numstr(bitstr_t *bs, const struct field_t *f,
 	BYTE_ALIGN(bs);
 	INC_BITS(bs, (len << 2));
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -440,12 +458,14 @@ static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 		break;
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
@@ -458,7 +478,8 @@ static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -473,7 +494,8 @@ static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
@@ -484,7 +506,8 @@ static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 
 	bs->cur += len << 1;
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -525,9 +548,11 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 
 		/* Decode */
 		if (son->attr & OPEN) {	/* Open field */
-			CHECK_BOUND(bs, 2);
+			if (nf_h323_error_boundary(bs, 2))
+				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
 				      " ", son->name);
@@ -556,7 +581,8 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 
 	/* Get the extension bitmap */
 	bmp2_len = get_bits(bs, 7) + 1;
-	CHECK_BOUND(bs, (bmp2_len + 7) >> 3);
+	if (nf_h323_error_boundary(bs, (bmp2_len + 7) >> 3))
+		return H323_ERROR_BOUND;
 	bmp2 = get_bitmap(bs, bmp2_len);
 	bmp |= bmp2 >> f->sz;
 	if (base)
@@ -567,9 +593,11 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 	for (opt = 0; opt < bmp2_len; opt++, i++, son++) {
 		/* Check Range */
 		if (i >= f->ub) {	/* Newer Version? */
-			CHECK_BOUND(bs, 2);
+			if (nf_h323_error_boundary(bs, 2))
+				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			bs->cur += len;
 			continue;
 		}
@@ -583,9 +611,11 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 		if (!((0x80000000 >> opt) & bmp2))	/* Not present */
 			continue;
 
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
 			      son->name);
@@ -623,19 +653,22 @@ static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		break;
 	case WORD:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		count <<= 8;
 		count += *bs->cur++;
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		count = get_len(bs);
 		break;
 	default:
@@ -659,7 +692,8 @@ static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 		if (son->attr & OPEN) {
 			BYTE_ALIGN(bs);
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
 				      " ", son->name);
@@ -728,7 +762,8 @@ static int decode_choice(bitstr_t *bs, const struct field_t *f,
 	if (type >= f->ub) {	/* Newer version? */
 		BYTE_ALIGN(bs);
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		bs->cur += len;
 		return H323_ERROR_NONE;
 	}
@@ -743,7 +778,8 @@ static int decode_choice(bitstr_t *bs, const struct field_t *f,
 	if (ext || (son->attr & OPEN)) {
 		BYTE_ALIGN(bs);
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
 			      son->name);
-- 
2.11.0

[PATCH 2/2] Extend nf_h323_error_boundary to work on bits as well

[eric.sesterhenn]

From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>

This patches several out of bounds memory reads by extending
the nf_h323_error_boundary() function to work on bits as well
an check the affected parts.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 92 +++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 30 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index f358222b1e5e..b8b4fecaa016 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -165,8 +165,13 @@ static unsigned int get_len(bitstr_t *bs)
 }
 
 /****************************************************************************/
-static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)
+static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes, size_t bits)
 {
+	bits += bs->bit;
+	bytes += bits / 8;
+	if (bits % 8 > 0)
+		bytes++;
+
 	if(*bs->cur + bytes > *bs->end)
 		return 1;
 	return 0;
@@ -286,8 +291,7 @@ static int decode_bool(bitstr_t *bs, const struct field_t *f,
 	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
 
 	INC_BIT(bs);
-
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -301,12 +305,12 @@ static int decode_oid(bitstr_t *bs, const struct field_t *f,
 	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
 
 	BYTE_ALIGN(bs);
-	if (nf_h323_error_boundary(bs, 1))
+	if (nf_h323_error_boundary(bs, 1, 0))
 		return H323_ERROR_BOUND;
 
 	len = *bs->cur++;
 	bs->cur += len;
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 
 	return H323_ERROR_NONE;
@@ -330,6 +334,8 @@ static int decode_int(bitstr_t *bs, const struct field_t *f,
 		bs->cur += 2;
 		break;
 	case CONS:		/* 64K < Range < 4G */
+		if (nf_h323_error_boundary(bs, 0, 2))
+			return H323_ERROR_BOUND;
 		len = get_bits(bs, 2) + 1;
 		BYTE_ALIGN(bs);
 		if (base && (f->attr & DECODE)) {	/* timeToLive */
@@ -341,7 +347,7 @@ static int decode_int(bitstr_t *bs, const struct field_t *f,
 		break;
 	case UNCO:
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		bs->cur += len;
@@ -353,7 +359,7 @@ static int decode_int(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -370,7 +376,7 @@ static int decode_enum(bitstr_t *bs, const struct field_t *f,
 		INC_BITS(bs, f->sz);
 	}
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -389,13 +395,13 @@ static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 		len = f->lb;
 		break;
 	case WORD:		/* 2-byte length */
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		len = (*bs->cur++) << 8;
 		len += (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		break;
@@ -407,7 +413,7 @@ static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 	bs->cur += len >> 3;
 	bs->bit = len & 7;
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -421,12 +427,14 @@ static int decode_numstr(bitstr_t *bs, const struct field_t *f,
 	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
 
 	/* 2 <= Range <= 255 */
+	if (nf_h323_error_boundary(bs, 0, f->sz))
+		return H323_ERROR_BOUND;
 	len = get_bits(bs, f->sz) + f->lb;
 
 	BYTE_ALIGN(bs);
 	INC_BITS(bs, (len << 2));
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -458,17 +466,19 @@ static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 		break;
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 1))
+		if (nf_h323_error_boundary(bs, 1, 0))
 			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		len = get_len(bs) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
+		if (nf_h323_error_boundary(bs, 0, f->sz))
+			return H323_ERROR_BOUND;
 		len = get_bits(bs, f->sz) + f->lb;
 		BYTE_ALIGN(bs);
 		break;
@@ -478,7 +488,7 @@ static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -494,11 +504,13 @@ static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 1))
+		if (nf_h323_error_boundary(bs, 1, 0))
 			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
+		if (nf_h323_error_boundary(bs, 0, f->sz))
+			return H323_ERROR_BOUND;
 		len = get_bits(bs, f->sz) + f->lb;
 		BYTE_ALIGN(bs);
 		break;
@@ -506,7 +518,7 @@ static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 
 	bs->cur += len << 1;
 
-	if (nf_h323_error_boundary(bs, 0))
+	if (nf_h323_error_boundary(bs, 0, 0))
 		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
@@ -526,9 +538,13 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 	base = (base && (f->attr & DECODE)) ? base + f->offset : NULL;
 
 	/* Extensible? */
+	if (nf_h323_error_boundary(bs, 0, 1))
+		return H323_ERROR_BOUND;
 	ext = (f->attr & EXT) ? get_bit(bs) : 0;
 
 	/* Get fields bitmap */
+	if (nf_h323_error_boundary(bs, 0, f->sz))
+		return H323_ERROR_BOUND;
 	bmp = get_bitmap(bs, f->sz);
 	if (base)
 		*(unsigned int *)base = bmp;
@@ -548,10 +564,10 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 
 		/* Decode */
 		if (son->attr & OPEN) {	/* Open field */
-			if (nf_h323_error_boundary(bs, 2))
+			if (nf_h323_error_boundary(bs, 2, 0))
 				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			if (nf_h323_error_boundary(bs, len))
+			if (nf_h323_error_boundary(bs, len, 0))
 				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
@@ -580,8 +596,10 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 		return H323_ERROR_NONE;
 
 	/* Get the extension bitmap */
+	if (nf_h323_error_boundary(bs, 0, 7))
+		return H323_ERROR_BOUND;
 	bmp2_len = get_bits(bs, 7) + 1;
-	if (nf_h323_error_boundary(bs, (bmp2_len + 7) >> 3))
+	if (nf_h323_error_boundary(bs, 0, bmp2_len))
 		return H323_ERROR_BOUND;
 	bmp2 = get_bitmap(bs, bmp2_len);
 	bmp |= bmp2 >> f->sz;
@@ -593,10 +611,10 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 	for (opt = 0; opt < bmp2_len; opt++, i++, son++) {
 		/* Check Range */
 		if (i >= f->ub) {	/* Newer Version? */
-			if (nf_h323_error_boundary(bs, 2))
+			if (nf_h323_error_boundary(bs, 2, 0))
 				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			if (nf_h323_error_boundary(bs, len))
+			if (nf_h323_error_boundary(bs, len, 0))
 				return H323_ERROR_BOUND;
 			bs->cur += len;
 			continue;
@@ -611,10 +629,10 @@ static int decode_seq(bitstr_t *bs, const struct field_t *f,
 		if (!((0x80000000 >> opt) & bmp2))	/* Not present */
 			continue;
 
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		len = get_len(bs);
-		if (nf_h323_error_boundary(bs, len))
+		if (nf_h323_error_boundary(bs, len, 0))
 			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
@@ -653,13 +671,13 @@ static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 1))
+		if (nf_h323_error_boundary(bs, 1, 0))
 			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		break;
 	case WORD:
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		count <<= 8;
@@ -667,11 +685,13 @@ static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		if (nf_h323_error_boundary(bs, 2))
+		if (nf_h323_error_boundary(bs, 2, 0))
 			return H323_ERROR_BOUND;
 		count = get_len(bs);
 		break;
 	default:
+		if (nf_h323_error_boundary(bs, 0, f->sz))
+			return H323_ERROR_BOUND;
 		count = get_bits(bs, f->sz);
 		break;
 	}
@@ -691,8 +711,10 @@ 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);
+			if (nf_h323_error_boundary(bs, 2, 0))
+				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			if (nf_h323_error_boundary(bs, len))
+			if (nf_h323_error_boundary(bs, len, 0))
 				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
@@ -744,11 +766,17 @@ 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 */
+	if (nf_h323_error_boundary(bs, 0, 1))
+		return H323_ERROR_BOUND;
 	if ((f->attr & EXT) && get_bit(bs)) {
 		ext = 1;
+		if (nf_h323_error_boundary(bs, 0, 7))
+			return H323_ERROR_BOUND;
 		type = get_bits(bs, 7) + f->lb;
 	} else {
 		ext = 0;
+		if (nf_h323_error_boundary(bs, 0, f->sz))
+			return H323_ERROR_BOUND;
 		type = get_bits(bs, f->sz);
 		if (type >= f->lb)
 			return H323_ERROR_RANGE;
@@ -761,8 +789,10 @@ static int decode_choice(bitstr_t *bs, const struct field_t *f,
 	/* Check Range */
 	if (type >= f->ub) {	/* Newer version? */
 		BYTE_ALIGN(bs);
+		if (nf_h323_error_boundary(bs, 2, 0))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
-		if (nf_h323_error_boundary(bs, len))
+		if (nf_h323_error_boundary(bs, len, 0))
 			return H323_ERROR_BOUND;
 		bs->cur += len;
 		return H323_ERROR_NONE;
@@ -777,8 +807,10 @@ static int decode_choice(bitstr_t *bs, const struct field_t *f,
 
 	if (ext || (son->attr & OPEN)) {
 		BYTE_ALIGN(bs);
+		if (nf_h323_error_boundary(bs, len, 0))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
-		if (nf_h323_error_boundary(bs, len))
+		if (nf_h323_error_boundary(bs, len, 0))
 			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
-- 
2.11.0

Re: [PATCH 1/2] Convert CHECK_BOUND macro to function

[Pablo Neira Ayuso]

On Mon, Nov 13, 2017 at 09:09:40AM +0100, eric.sesterhenn@x41-dsec.de wrote:
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> 
> It is bad practive to return in a macro, this patch
> moves the check into a function.

Applied with minor changes, see below.

[...]
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 89b2e46925c4..f358222b1e5e 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -103,7 +103,6 @@ typedef struct {
>  #define INC_BIT(bs) if((++(bs)->bit)>7){(bs)->cur++;(bs)->bit=0;}
>  #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)
>  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);
> @@ -166,6 +165,14 @@ static unsigned int get_len(bitstr_t *bs)
>  }
>  
>  /****************************************************************************/
> +static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)

Make sure you make you patches on top of nf-next.git:

https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git/

Look, bitstr_t is gone there already, we got a patch to remove
typedefs.

Anyway, I have mangled this here and it's now applied.

It would be great if your follow up patch subject is prefixes like
this:

        netfilter: nf_ct_h323: blah

So we know what subsystem this is targeting to, just for the next
time.

Thanks for following up on this!

Re: [PATCH 2/2] Extend nf_h323_error_boundary to work on bits as well

[Pablo Neira Ayuso]

On Mon, Nov 13, 2017 at 09:09:41AM +0100, eric.sesterhenn@x41-dsec.de wrote:
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> 
> This patches several out of bounds memory reads by extending
> the nf_h323_error_boundary() function to work on bits as well
> an check the affected parts.

Also applied with changes, see below.

[...]
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index f358222b1e5e..b8b4fecaa016 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -165,8 +165,13 @@ static unsigned int get_len(bitstr_t *bs)
>  }
>  
>  /****************************************************************************/
> -static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)
> +static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes, size_t bits)
>  {
> +	bits += bs->bit;
> +	bytes += bits / 8;

I changed this to use BITS_PER_BYTE instead of hardcoded 8, just a
minor comestic cleanup.

Please, review I'm going to push to nf-next.git, given I have to
mangled your patches slightly, just to make sure I didn't slip through
any mistake.

Thanks.