Skip to content

avoid undefined shift in hpack DecodeInteger on crafted integer#3379

Open
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:hpack-decode-integer-shift
Open

avoid undefined shift in hpack DecodeInteger on crafted integer#3379
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:hpack-decode-integer-shift

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number:

Problem Summary:
DecodeInteger keeps consuming HPACK continuation octets while tmp < MAX_HPACK_INTEGER. A run of 0x80 octets carries a zero payload, so tmp never grows and that guard never trips, while the shift amount m keeps climbing; once m reaches 64 the << m on a uint64_t is undefined behavior. UBSan reports shift exponent 70 is too large for 64-bit type. It is reachable from any HTTP/2 or gRPC peer via a crafted HEADERS frame whose index is encoded as 0xFF followed by continuation octets.

What is changed and the side effects?

Changed:
Reject the over-long encoding once m reaches 32, before the shift can leave the type width. A valid integer below MAX_HPACK_INTEGER needs only a handful of continuation octets, so no well-formed input is turned away. A regression test in brpc_hpack_unittest.cpp decodes 0xFF plus a long run of 0x80 and checks it is rejected.

Side effects:

  • Performance effects: one extra comparison per continuation octet, no measurable change.

  • Breaking backward compatibility: none.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens HPACK integer decoding in src/brpc/details/hpack.cpp to avoid undefined behavior from shifting a uint64_t by 64+ bits when parsing crafted, over-long continuation sequences, and adds a regression test to ensure such inputs are rejected.

Changes:

  • Add a guard in DecodeInteger to reject over-long continuation encodings before the shift amount can grow to an undefined range.
  • Add a unit test that feeds 0xFF followed by many 0x80 continuation octets and asserts decode failure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/brpc/details/hpack.cpp Adds a continuation-length/shift guard in HPACK integer decoding to prevent undefined shifts on malformed inputs.
test/brpc_hpack_unittest.cpp Adds a regression test covering a crafted over-long HPACK integer continuation sequence.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +556 to +559
if (m >= 32) {
LOG(ERROR) << "Source stream is likely malformed";
return -1;
}
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.

2 participants