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>

 		p += len;

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>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html