Add some exceptions for MessageParser.ParseFrom (#5588)

* Fix #5513 * Added tests for invalid lengths when reading strings and bytes. Added test for reading tags with invalid wire types in unknown field set. Changed invalid length check in ReadString to match the one in ReadBytes
protocolbuffers · Feb 26, 2019 · a4c3472 · a4c3472
1 parent 3a538fc
commit a4c3472
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 2 deletions.
diff --git a/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs b/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs
@@ -373,6 +373,42 @@ public void ReadInvalidUtf8()
             Assert.AreEqual('\ufffd', text[0]);
         }
 
+        [Test]
+        public void ReadNegativeSizedStringThrowsInvalidProtocolBufferException()
+        {
+            MemoryStream ms = new MemoryStream();
+            CodedOutputStream output = new CodedOutputStream(ms);
+
+            uint tag = WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited);
+            output.WriteRawVarint32(tag);
+            output.WriteLength(-1);
+            output.Flush();
+            ms.Position = 0;
+
+            CodedInputStream input = new CodedInputStream(ms);
+
+            Assert.AreEqual(tag, input.ReadTag());
+            Assert.Throws<InvalidProtocolBufferException>(() => input.ReadString());
+        }
+
+        [Test]
+        public void ReadNegativeSizedBytesThrowsInvalidProtocolBufferException()
+        {
+            MemoryStream ms = new MemoryStream();
+            CodedOutputStream output = new CodedOutputStream(ms);
+
+            uint tag = WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited);
+            output.WriteRawVarint32(tag);
+            output.WriteLength(-1);
+            output.Flush();
+            ms.Position = 0;
+
+            CodedInputStream input = new CodedInputStream(ms);
+
+            Assert.AreEqual(tag, input.ReadTag());
+            Assert.Throws<InvalidProtocolBufferException>(() => input.ReadBytes());
+        }
+
         /// <summary>
         /// A stream which limits the number of bytes it reads at a time.
         /// We use this to make sure that CodedInputStream doesn't screw up when

diff --git a/csharp/src/Google.Protobuf.Test/UnknownFieldSetTest.cs b/csharp/src/Google.Protobuf.Test/UnknownFieldSetTest.cs
@@ -172,5 +172,23 @@ public void TestDiscardUnknownFields()
             assertEmpty(discardingParser1.ParseFrom(new MemoryStream(data)));
             assertEmpty(discardingParser2.ParseFrom(new MemoryStream(data)));
         }
+
+        [Test]
+        public void TestReadInvalidWireTypeThrowsInvalidProtocolBufferException()
+        {
+            MemoryStream ms = new MemoryStream();
+            CodedOutputStream output = new CodedOutputStream(ms);
+
+            uint tag = WireFormat.MakeTag(1, (WireFormat.WireType)6);
+            output.WriteRawVarint32(tag);
+            output.WriteLength(-1);
+            output.Flush();
+            ms.Position = 0;
+
+            CodedInputStream input = new CodedInputStream(ms);
+            Assert.AreEqual(tag, input.ReadTag());
+
+            Assert.Throws<InvalidProtocolBufferException>(() => UnknownFieldSet.MergeFieldFrom(null, input));
+        }
     }
 }
diff --git a/csharp/src/Google.Protobuf/CodedInputStream.cs b/csharp/src/Google.Protobuf/CodedInputStream.cs
@@ -560,7 +560,7 @@ public string ReadString()
             {
                 return "";
             }
-            if (length <= bufferSize - bufferPos)
+            if (length <= bufferSize - bufferPos && length > 0)
             {
                 // Fast path:  We already have the bytes in a contiguous buffer, so
                 //   just copy directly from it.

diff --git a/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs b/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs
@@ -88,6 +88,12 @@ internal static InvalidProtocolBufferException InvalidTag()
                 "Protocol message contained an invalid tag (zero).");
         }
 
+        internal static InvalidProtocolBufferException InvalidWireType()
+        {
+            return new InvalidProtocolBufferException(
+                "Protocol message contained a tag with an invalid wire type.");
+        }
+
         internal static InvalidProtocolBufferException InvalidBase64(Exception innerException)
         {
             return new InvalidProtocolBufferException("Invalid base64 data", innerException);

diff --git a/csharp/src/Google.Protobuf/UnknownFieldSet.cs b/csharp/src/Google.Protobuf/UnknownFieldSet.cs
@@ -229,7 +229,7 @@ private bool MergeFieldFrom(CodedInputStream input)
                         return false;
                     }
                 default:
-                    throw new InvalidOperationException("Wire Type is invalid.");
+                    throw InvalidProtocolBufferException.InvalidWireType();
             }
         }