Skip to content

Commit c18aa77

Browse files
authored
Validate the tag numbers when parsing. (protocolbuffers#1725)
There was a twist code path (that some times showed up due to what happened to be in memory in failure cases), that would cast a bogus wire type into the enum, and then fall through switch statements. Resolve this by validating all wire types when parsing tags and throwing the error at that point so it can't enter the system. As added safety, stick in a few asserts for apis that get passed tags to ensure they also are only seeing valid data. Bonus: Tweak the parsing loop to skip some work when we get the end marker (zero tag) instead of still looping through all the fields.
1 parent e0016c5 commit c18aa77

5 files changed

+21
-4
lines changed

objectivec/GPBCodedInputStream.m

+8-3
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,13 @@ int32_t GPBCodedInputStreamReadTag(GPBCodedInputStreamState *state) {
227227
state->lastTag = ReadRawVarint32(state);
228228
if (state->lastTag == 0) {
229229
// If we actually read zero, that's not a valid tag.
230-
RaiseException(GPBCodedInputStreamErrorInvalidTag, @"Last tag can't be 0");
230+
RaiseException(GPBCodedInputStreamErrorInvalidTag,
231+
@"A zero tag on the wire is invalid.");
232+
}
233+
// Tags have to include a valid wireformat, check that also.
234+
if (!GPBWireFormatIsValidTag(state->lastTag)) {
235+
RaiseException(GPBCodedInputStreamErrorInvalidTag,
236+
@"Invalid wireformat in tag.");
231237
}
232238
return state->lastTag;
233239
}
@@ -352,6 +358,7 @@ - (void)checkLastTagWas:(int32_t)value {
352358
}
353359

354360
- (BOOL)skipField:(int32_t)tag {
361+
NSAssert(GPBWireFormatIsValidTag(tag), @"Invalid tag");
355362
switch (GPBWireFormatGetTagWireType(tag)) {
356363
case GPBWireFormatVarint:
357364
GPBCodedInputStreamReadInt32(&state_);
@@ -374,8 +381,6 @@ - (BOOL)skipField:(int32_t)tag {
374381
SkipRawData(&state_, sizeof(int32_t));
375382
return YES;
376383
}
377-
RaiseException(GPBCodedInputStreamErrorInvalidTag, nil);
378-
return NO;
379384
}
380385

381386
- (void)skipMessage {

objectivec/GPBMessage.m

+4-1
Original file line numberDiff line numberDiff line change
@@ -2279,6 +2279,9 @@ - (void)mergeFromCodedInputStream:(GPBCodedInputStream *)input
22792279
while (YES) {
22802280
BOOL merged = NO;
22812281
tag = GPBCodedInputStreamReadTag(state);
2282+
if (tag == 0) {
2283+
break; // Reached end.
2284+
}
22822285
for (NSUInteger i = 0; i < numFields; ++i) {
22832286
if (startingIndex >= numFields) startingIndex = 0;
22842287
GPBFieldDescriptor *fieldDescriptor = fields[startingIndex];
@@ -2317,7 +2320,7 @@ - (void)mergeFromCodedInputStream:(GPBCodedInputStream *)input
23172320
}
23182321
} // for(i < numFields)
23192322

2320-
if (!merged) {
2323+
if (!merged && (tag != 0)) {
23212324
// Primitive, repeated types can be packed on unpacked on the wire, and
23222325
// are parsed either way. The above loop covered tag in the preferred
23232326
// for, so this need to check the alternate form.

objectivec/GPBUnknownFieldSet.m

+1
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,7 @@ - (void)mergeVarintField:(int32_t)number value:(int32_t)value {
359359
}
360360

361361
- (BOOL)mergeFieldFrom:(int32_t)tag input:(GPBCodedInputStream *)input {
362+
NSAssert(GPBWireFormatIsValidTag(tag), @"Got passed an invalid tag");
362363
int32_t number = GPBWireFormatGetTagFieldNumber(tag);
363364
GPBCodedInputStreamState *state = &input->state_;
364365
switch (GPBWireFormatGetTagWireType(tag)) {

objectivec/GPBWireFormat.h

+1
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ uint32_t GPBWireFormatMakeTag(uint32_t fieldNumber, GPBWireFormat wireType)
5353
__attribute__((const));
5454
GPBWireFormat GPBWireFormatGetTagWireType(uint32_t tag) __attribute__((const));
5555
uint32_t GPBWireFormatGetTagFieldNumber(uint32_t tag) __attribute__((const));
56+
BOOL GPBWireFormatIsValidTag(uint32_t tag) __attribute__((const));
5657

5758
GPBWireFormat GPBWireFormatForType(GPBDataType dataType, BOOL isPacked)
5859
__attribute__((const));

objectivec/GPBWireFormat.m

+7
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,13 @@ uint32_t GPBWireFormatGetTagFieldNumber(uint32_t tag) {
4949
return GPBLogicalRightShift32(tag, GPBWireFormatTagTypeBits);
5050
}
5151

52+
BOOL GPBWireFormatIsValidTag(uint32_t tag) {
53+
uint32_t formatBits = (tag & GPBWireFormatTagTypeMask);
54+
// The valid GPBWireFormat* values are 0-5, anything else is not a valid tag.
55+
BOOL result = (formatBits <= 5);
56+
return result;
57+
}
58+
5259
GPBWireFormat GPBWireFormatForType(GPBDataType type, BOOL isPacked) {
5360
if (isPacked) {
5461
return GPBWireFormatLengthDelimited;

0 commit comments

Comments
 (0)