From c18aa7795a2e02ef700ff8b039d94ecdcc33432f Mon Sep 17 00:00:00 2001
From: Thomas Van Lenten <thomasvl@google.com>
Date: Wed, 29 Jun 2016 09:51:13 -0400
Subject: [PATCH] Validate the tag numbers when parsing. (#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.
---
 objectivec/GPBCodedInputStream.m | 11 ++++++++---
 objectivec/GPBMessage.m          |  5 ++++-
 objectivec/GPBUnknownFieldSet.m  |  1 +
 objectivec/GPBWireFormat.h       |  1 +
 objectivec/GPBWireFormat.m       |  7 +++++++
 5 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/objectivec/GPBCodedInputStream.m b/objectivec/GPBCodedInputStream.m
index acba715681..2b578dd54c 100644
--- a/objectivec/GPBCodedInputStream.m
+++ b/objectivec/GPBCodedInputStream.m
@@ -227,7 +227,13 @@ int32_t GPBCodedInputStreamReadTag(GPBCodedInputStreamState *state) {
   state->lastTag = ReadRawVarint32(state);
   if (state->lastTag == 0) {
     // If we actually read zero, that's not a valid tag.
-    RaiseException(GPBCodedInputStreamErrorInvalidTag, @"Last tag can't be 0");
+    RaiseException(GPBCodedInputStreamErrorInvalidTag,
+                   @"A zero tag on the wire is invalid.");
+  }
+  // Tags have to include a valid wireformat, check that also.
+  if (!GPBWireFormatIsValidTag(state->lastTag)) {
+    RaiseException(GPBCodedInputStreamErrorInvalidTag,
+                   @"Invalid wireformat in tag.");
   }
   return state->lastTag;
 }
@@ -352,6 +358,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state,
 }
 
 - (BOOL)skipField:(int32_t)tag {
+  NSAssert(GPBWireFormatIsValidTag(tag), @"Invalid tag");
   switch (GPBWireFormatGetTagWireType(tag)) {
     case GPBWireFormatVarint:
       GPBCodedInputStreamReadInt32(&state_);
@@ -374,8 +381,6 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state,
       SkipRawData(&state_, sizeof(int32_t));
       return YES;
   }
-  RaiseException(GPBCodedInputStreamErrorInvalidTag, nil);
-  return NO;
 }
 
 - (void)skipMessage {
diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m
index 7ab184c1f1..b9566bdf4c 100644
--- a/objectivec/GPBMessage.m
+++ b/objectivec/GPBMessage.m
@@ -2279,6 +2279,9 @@ static void MergeRepeatedNotPackedFieldFromCodedInputStream(
   while (YES) {
     BOOL merged = NO;
     tag = GPBCodedInputStreamReadTag(state);
+    if (tag == 0) {
+      break;  // Reached end.
+    }
     for (NSUInteger i = 0; i < numFields; ++i) {
       if (startingIndex >= numFields) startingIndex = 0;
       GPBFieldDescriptor *fieldDescriptor = fields[startingIndex];
@@ -2317,7 +2320,7 @@ static void MergeRepeatedNotPackedFieldFromCodedInputStream(
       }
     }  // for(i < numFields)
 
-    if (!merged) {
+    if (!merged && (tag != 0)) {
       // Primitive, repeated types can be packed on unpacked on the wire, and
       // are parsed either way.  The above loop covered tag in the preferred
       // for, so this need to check the alternate form.
diff --git a/objectivec/GPBUnknownFieldSet.m b/objectivec/GPBUnknownFieldSet.m
index af08556e62..9ba1d65c88 100644
--- a/objectivec/GPBUnknownFieldSet.m
+++ b/objectivec/GPBUnknownFieldSet.m
@@ -359,6 +359,7 @@ static void GPBUnknownFieldSetMergeUnknownFields(const void *key,
 }
 
 - (BOOL)mergeFieldFrom:(int32_t)tag input:(GPBCodedInputStream *)input {
+  NSAssert(GPBWireFormatIsValidTag(tag), @"Got passed an invalid tag");
   int32_t number = GPBWireFormatGetTagFieldNumber(tag);
   GPBCodedInputStreamState *state = &input->state_;
   switch (GPBWireFormatGetTagWireType(tag)) {
diff --git a/objectivec/GPBWireFormat.h b/objectivec/GPBWireFormat.h
index 29cf2f0b8d..c5941a3824 100644
--- a/objectivec/GPBWireFormat.h
+++ b/objectivec/GPBWireFormat.h
@@ -53,6 +53,7 @@ uint32_t GPBWireFormatMakeTag(uint32_t fieldNumber, GPBWireFormat wireType)
     __attribute__((const));
 GPBWireFormat GPBWireFormatGetTagWireType(uint32_t tag) __attribute__((const));
 uint32_t GPBWireFormatGetTagFieldNumber(uint32_t tag) __attribute__((const));
+BOOL GPBWireFormatIsValidTag(uint32_t tag) __attribute__((const));
 
 GPBWireFormat GPBWireFormatForType(GPBDataType dataType, BOOL isPacked)
     __attribute__((const));
diff --git a/objectivec/GPBWireFormat.m b/objectivec/GPBWireFormat.m
index 193235d6ce..860a339f9a 100644
--- a/objectivec/GPBWireFormat.m
+++ b/objectivec/GPBWireFormat.m
@@ -49,6 +49,13 @@ uint32_t GPBWireFormatGetTagFieldNumber(uint32_t tag) {
   return GPBLogicalRightShift32(tag, GPBWireFormatTagTypeBits);
 }
 
+BOOL GPBWireFormatIsValidTag(uint32_t tag) {
+  uint32_t formatBits = (tag & GPBWireFormatTagTypeMask);
+  // The valid GPBWireFormat* values are 0-5, anything else is not a valid tag.
+  BOOL result = (formatBits <= 5);
+  return result;
+}
+
 GPBWireFormat GPBWireFormatForType(GPBDataType type, BOOL isPacked) {
   if (isPacked) {
     return GPBWireFormatLengthDelimited;