Re: [net,06/11] netfilter: nf_conntrack_h323: fix OOB read in decode_int() CONS case
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-03-14 16:12:56
Also in:
netfilter-devel
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- netfilter: nf_conntrack_h323: fix OOB read in decode_int() CONS case This patch adds a boundary check in the CONS case of decode_int() to prevent reading beyond the buffer after get_bits() returns a length value. The fix validates that len bytes remain before calling get_uint().
quoted hunk ↗ jump to hunk
diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c index 62aa22a07876..c972e9488e16 100644 --- a/net/netfilter/nf_conntrack_h323_asn1.c +++ b/net/netfilter/nf_conntrack_h323_asn1.c@@ -331,6 +331,8 @@ static int decode_int(struct bitstr *bs, const struct field_t *f, if (nf_h323_error_boundary(bs, 0, 2)) return H323_ERROR_BOUND; len = get_bits(bs, 2) + 1; + if (nf_h323_error_boundary(bs, len, 0)) + return H323_ERROR_BOUND; BYTE_ALIGN(bs); if (base && (f->attr & DECODE)) { /* timeToLive */ unsigned int v = get_uint(bs, len) + f->lb;
This patch fixes the CONS case, but the UNCO case in the same function has the same vulnerability. In the UNCO case, get_len(bs) can return up to 0x3FFF (16383 bytes), and the code advances bs->cur by that amount without checking that len bytes remain: case UNCO: BYTE_ALIGN(bs); if (nf_h323_error_boundary(bs, 2, 0)) return H323_ERROR_BOUND; len = get_len(bs); bs->cur += len; break; The check only validates 2 bytes for get_len() itself, not the subsequent len bytes that will be skipped. Does this need the same fix pattern? The decode_bitstr() function has a similar issue. In the SEMI case, it calls get_len(bs) which can return up to 0x3FFF bits, then advances the buffer by len >> 3 bytes plus partial bits without checking that sufficient buffer space exists: case SEMI: if (nf_h323_error_boundary(bs, 2, 0)) return H323_ERROR_BOUND; len = get_len(bs); break; } bs->cur += len >> 3; bs->bit = len & 7; The check at the top only validates 2 bytes for the get_len() call. The actual advance at the bottom can be up to 2047 bytes. Should this be checked as well? The decode_octstr() function has the same pattern in both the SEMI case and the default case: case SEMI: BYTE_ALIGN(bs); 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; } bs->cur += len; In the SEMI case, get_len() can return up to 0x3FFF and f->lb is added. In the default case, get_bits(bs, f->sz) returns an attacker-controlled value based on field size and f->lb is added. Both cases then advance by len bytes without checking. The existing checks only validate the bytes needed to READ the length value, not the len bytes that will be consumed. Should boundary checks be added after both break statements? Similarly, decode_bmpstr() (which handles 2-byte character strings) has the same issue in both the BYTE case and default case: case BYTE: /* Range == 256 */ BYTE_ALIGN(bs); 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; } bs->cur += len << 1; After computing len from packet data plus f->lb, the function advances by len << 1 bytes (doubling for 2-byte characters) without verifying that len << 1 bytes are available. Should this also be checked?