[PATCH] netfilter: nf_conntrack_h323: fix NULL deref in decode_seqof()

[PATCH] netfilter: nf_conntrack_h323: fix NULL deref in decode_seqof()

[Guanglei Zhu]

decode_seqof() loads son from f->fields and dereferences son->attr
in the item loop without a NULL check.  A number of SEQOF entries
in nf_conntrack_h323_types.c carry a NULL fields pointer, so a
crafted packet that makes one of these optional fields present with
a nonzero item count dereferences NULL in the loop.

Skip the decoding when fields is NULL.

Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
Verified on a KASAN kernel with a test module calling decode_seqof()
with a fields=NULL SEQOF entry and a nonzero item count: NULL deref
report at son->attr (read of size 2 at address 0x4).

 net/netfilter/nf_conntrack_h323_asn1.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 6830c9da3..0d07d0276 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -691,6 +691,8 @@ static int decode_seqof(struct bitstr *bs, const struct field_t *f,
 	}
 
 	/* Decode nested field */
+	if (!f->fields)
+		return H323_ERROR_NONE;
 	son = f->fields;
 	if (base)
 		base -= son->offset;
-- 
2.43.0

Re: [PATCH] netfilter: nf_conntrack_h323: fix NULL deref in decode_seqof()

[Florian Westphal]

Guanglei Zhu <zhugl3@xiaopeng.com> wrote:
> decode_seqof() loads son from f->fields and dereferences son->attr
> in the item loop without a NULL check.  A number of SEQOF entries
> in nf_conntrack_h323_types.c carry a NULL fields pointer, so a
> crafted packet that makes one of these optional fields present with
> a nonzero item count dereferences NULL in the loop.
> 
> Skip the decoding when fields is NULL.
> 
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
> ---
> Verified on a KASAN kernel with a test module calling decode_seqof()
> with a fields=NULL SEQOF entry and a nonzero item count: NULL deref
> report at son->attr (read of size 2 at address 0x4).

We don't add NULL tests unless there is a sensibe reason.

Will you add NULL tests to other functions too because a 3rd party
module would crash otherwise?

If this is reachable, show how.

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3..0d07d0276 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -691,6 +691,8 @@ static int decode_seqof(struct bitstr *bs, const struct field_t *f,
>  	}
>  
>  	/* Decode nested field */
> +	if (!f->fields)
> +		return H323_ERROR_NONE;
>  	son = f->fields;

This is strange.

son = f->fields;
if (!son)...

would be more natural.

Re: [PATCH] netfilter: nf_conntrack_h323: fix NULL deref in decode_seqof()

[Guanglei Zhu]

You are right, I could not find a reachable path either.

I re-checked all call sites that would invoke decode_seqof() without
a DECODE guard:

- decode_seq() root loop: every SEQOF entry with a NULL fields
  pointer sits outside the root window (index >= f->lb); the ones
  inside the root window carry STOP or OPEN.
- decode_seq() extension loop: entries without DECODE are skipped
  with bs->cur += len, decode_seqof() is never called for them.
- decode_choice() and the decode_seqof() item loop: no NULL-fields
  SEQOF entries among the options/items without STOP or OPEN either.

So f->fields can only be NULL when decode_seqof() is called from
outside the decoder, which is what my test module did.  The KASAN
report only demonstrates the in-function deref, not reachability
from a packet.

Please drop this patch.