src/google/protobuf/parse_context.cc : Potential integer overflow

[stasos24]

Since inline void WriteVarint(uint64_t val, std::string* s) accept val as uint64_t at

protobuf/src/google/protobuf/parse_context.cc

Lines 316 to 323 in f587cf5

	inline void WriteVarint(uint64_t val, std::string* s) {
	while (val >= 128) {
	uint8_t c = val | 0x80;
	s->push_back(c);
	val >>= 7;
	}
	s->push_back(val);
	}

But void WriteVarint(uint32_t num, uint64_t val, std::string* s) has num as uint32_t

protobuf/src/google/protobuf/parse_context.cc

Lines 325 to 328 in f587cf5

	void WriteVarint(uint32_t num, uint64_t val, std::string* s) {
	WriteVarint(num << 3, s);
	WriteVarint(val, s);
	}

And there is no cast of "num"
overflow could happen at

protobuf/src/google/protobuf/parse_context.cc

Line 326 in f587cf5

WriteVarint(num << 3, s);

Fix:

inline void WriteVarint(uint32_t num, uint64_t val, std::string* s) {
  // Cast num to uint64_t to prevent overflow during the shift operation
  WriteVarint(static_cast<uint64_t>(num) << 3, s);
  WriteVarint(val, s);
}

[stasos24]

As that function used in:

const char* ExtensionSet::ParseFieldWithExtensionInfo(
    int number, bool was_packed_on_wire, const ExtensionInfo& extension,
    InternalMetadata* metadata, const char* ptr, internal::ParseContext* ctx) 
...
 WriteVarint(number, value, metadata->mutable_unknown_fields<T>());
....

I undestood number is the field number of the field being parsed. Each field in a protobuf message has a unique field number, which is used to identify the field in the wire format.t.

Please, prove me wrong if i'm mistaken

[hartwork]

@stasos24 I would like to second your finding of an unsigned integer overflow. My own play code about it was this:

#include <stdint.h>
#include <stdio.h>

void dummy(const char * label, uint64_t v) {
    printf("[%12s] 0x%9lx\n", label, v);
}

int main(void) {
    uint32_t num = 0xffffffff;
    dummy("without cast", num << 3);
    dummy("with cast", (uint64_t)num << 3);
    return 0;
}

At runtime:

# clang -Wall -Wextra main.c && ./a.out 
[without cast] 0x fffffff8
[   with cast] 0x7fffffff8

I find it surprising that neither Clang 19 nor GCC 14 warn about this despite -Wall -Wextra. UBSan cannot complain because unsigned overflow is not undefined behavior. So a tricky class of bugs to find apparently?

CC @hannob

[stasos24]

Same may occur at

protobuf/src/google/protobuf/parse_context.cc

Lines 514 to 516 in f587cf5

	void AddVarint(uint32_t num, uint64_t value) {
	if (unknown_ == nullptr) return;
	WriteVarint(num * 8, unknown_);

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

[hartwork]

@protocolbuffers I'd like to veto this going inactive. This issue keeps needing more attention, not less. Thank you.

[mkruskal-google]

Thanks for reporting this! I agree that looking narrowly at just this function and its immediate call-sites, it looks like there's a risk of overflow here that would corrupt the parse.

However, the maximum field number is actually 2^29-1, specifically so that field tags always fit in a single byte. So in practice, I'm fairly certain the overflow here is unreachable. It might be worth adding a DCHECK (we would likely accept a PR), but I don't see any way to hit this without breaking an assumption we likely make in quite a few places.

If you actually find a way to exercise this please do reopen this

[hartwork]

@mkruskal-google if you are certain of the 2^29-1 maximum, please add an assertion to both document and regression-proof about it. Without any change in code, this issue will just pop up again and will need to be answered again. Thank you.