Skip to content

Guard against NULL sub-enum table in upb_MiniTableEnum_CheckValue#28820

Open
Ashutosh0x wants to merge 1 commit into
protocolbuffers:mainfrom
Ashutosh0x:fix/upb-decode-null-subenum-deref
Open

Guard against NULL sub-enum table in upb_MiniTableEnum_CheckValue#28820
Ashutosh0x wants to merge 1 commit into
protocolbuffers:mainfrom
Ashutosh0x:fix/upb-decode-null-subenum-deref

Conversation

@Ashutosh0x

Copy link
Copy Markdown

A closed-enum field whose sub-enum table has not been linked resolves to a NULL upb_MiniTableEnum via upb_MiniTable_GetSubEnumTable(), which is a documented return value for unlinked fields. The varint and packed enum decode paths in upb/wire/decode.c call upb_MiniTableEnum_CheckValue() on that pointer without a NULL check, so decoding untrusted wire data against a MiniTable built from an untrusted mini descriptor dereferences NULL and crashes (reliably reproducible SIGSEGV; remote DoS for such callers).

Add a NULL guard at the single chokepoint in CheckValue that reports every value as unrecognized, routing it to the unknown-field set exactly as an out-of-range closed-enum value already is. UPB_UNLIKELY keeps the hot decode path unaffected. Adds a regression test covering all three internal branches on a NULL table.

Fixes #26857.

A closed-enum field whose sub-enum table has not been linked resolves to
a NULL upb_MiniTableEnum via upb_MiniTable_GetSubEnumTable(), which is a
documented return value for unlinked fields. The varint and packed enum
decode paths in upb/wire/decode.c call upb_MiniTableEnum_CheckValue() on
that pointer without a NULL check, so decoding untrusted wire data against
a MiniTable built from an untrusted mini descriptor dereferences NULL and
crashes (reliably reproducible SIGSEGV; remote DoS for such callers).

Add a NULL guard at the single chokepoint in CheckValue that reports every
value as unrecognized, routing it to the unknown-field set exactly as an
out-of-range closed-enum value already is. UPB_UNLIKELY keeps the hot
decode path unaffected. Adds a regression test covering all three internal
branches on a NULL table.

Fixes protocolbuffers#26857.
@Ashutosh0x

Copy link
Copy Markdown
Author

This is a one-line NULL guard at the single chokepoint. Context for review:

  • upb_MiniTable_GetSubEnumTable() is documented to return NULL for an unlinked closed-enum field. Two decode paths call upb_MiniTableEnum_CheckValue() on that pointer with no NULL check: varint at upb/wire/decode.c:892 and packed at :318/:325.
  • A MiniTable built from an untrusted mini descriptor (upb_MiniTable_Build, sub-enum not linked) therefore lets untrusted wire data reach a NULL dereference in upb_MiniTableEnum_CheckValue — a deterministic SIGSEGV; 5-byte reproducer in [Fuzzing] NULL-pointer dereference: upb_MiniTableEnum_CheckValue crash via unlinked closed-enum field in upb_Decode #26857.
  • The fix returns false for a NULL table, routing the value to the unknown-field set exactly as an out-of-range closed-enum value already is: no behavior change for linked tables, and UPB_UNLIKELY keeps the hot decode path unaffected.
  • The regression test exercises all three internal CheckValue branches on a NULL table.

Happy to move the guard to the decode call sites instead if the team prefers that over the shared chokepoint.

@haberman — this fixes #26857, which is assigned to you. The PR is from an external fork, so the full test matrix is gated behind the safe for tests label; could you take a look when you have a moment? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Fuzzing] NULL-pointer dereference: upb_MiniTableEnum_CheckValue crash via unlinked closed-enum field in upb_Decode

1 participant