Test and fix bug #5: A malformed packet can be used to leak old packets

diff --git a/Hazel.UnitTests/MessageReaderTests.cs b/Hazel.UnitTests/MessageReaderTests.cs
index 1d5c218c52a9301670590be07102ccc162ba72f9..04a727de6f6ded7ba1f285d2399e339abfd7af9b 100644 (file)
--- a/Hazel.UnitTests/MessageReaderTests.cs
+++ b/Hazel.UnitTests/MessageReaderTests.cs
@@ -165,6 +165,48 @@ namespace Hazel.UnitTests
             Assert.AreEqual("NO", sub.ReadString());
         }
 
+        [TestMethod]
+        public void ReadStringProtectsAgainstOverrun()
+        {
+            const string TestDataFromAPreviousPacket = "You shouldn't be able to see this data";
+
+            // An extra byte from the length of TestData when written via MessageWriter
+            int DataLength = TestDataFromAPreviousPacket.Length + 1;
+
+            // THE BUG
+            //
+            // No bound checks. When the server wants to read a string from
+            // an offset, it reads the packed int at that offset, treats it
+            // as a length and then proceeds to read the data that comes after
+            // it without any bound checks. This can be chained with something
+            // else to create an infoleak.
+
+            MessageWriter writer = MessageWriter.Get(SendOption.None);
+
+            // This will be our malicious "string length"
+            writer.WritePacked(DataLength);
+
+            // This is data from a "previous packet"
+            writer.Write(TestDataFromAPreviousPacket);
+
+            byte[] testData = writer.ToByteArray(includeHeader: false);
+
+            // One extra byte for the MessageWriter header, one more for the malicious data
+            Assert.AreEqual(DataLength + 1, testData.Length);
+
+            var dut = MessageReader.Get(testData);
+
+            // If Length is short by even a byte, ReadString should obey that.
+            dut.Length--;
+
+            try
+            {
+                dut.ReadString();
+                Assert.Fail("ReadString is expected to throw");
+            }
+            catch (InvalidDataException) { }
+        }
+
         [TestMethod]
         public void GetLittleEndian()
         {

diff --git a/Hazel/MessageReader.cs b/Hazel/MessageReader.cs
index b5f53846fbd3bc92495a96ee9a911cf4e96591d1..e19d90ac33a6600dea757d11b2367271bb10499a 100644 (file)
--- a/Hazel/MessageReader.cs
+++ b/Hazel/MessageReader.cs
@@ -1,4 +1,5 @@
 using System;
+using System.IO;
 using System.Runtime.CompilerServices;
 using System.Text;
 
@@ -14,6 +15,8 @@ namespace Hazel
         public int Length;
         public int Offset;
 
+        public int BytesRemaining => this.Length - this.Position;
+
         public int Position
         {
             get { return this._position; }
@@ -201,6 +204,8 @@ namespace Hazel
         public string ReadString()
         {
             int len = this.ReadPackedInt32();
+            if (this.BytesRemaining < len) throw new InvalidDataException($"Read length is longer than message length: {len} of {this.BytesRemaining}");
+
             string output = UTF8Encoding.UTF8.GetString(this.Buffer, this.readHead, len);
 
             this.Position += len;
@@ -210,11 +215,15 @@ namespace Hazel
         public byte[] ReadBytesAndSize()
         {
             int len = this.ReadPackedInt32();
+            if (this.BytesRemaining < len) throw new InvalidDataException($"Read length is longer than message length: {len} of {this.BytesRemaining}");
+
             return this.ReadBytes(len);
         }
 
         public byte[] ReadBytes(int length)
         {
+            if (this.BytesRemaining < length) throw new InvalidDataException($"Read length is longer than message length: {length} of {this.BytesRemaining}");
+
             byte[] output = new byte[length];
             Array.Copy(this.Buffer, this.readHead, output, 0, output.Length);
             this.Position += output.Length;
@@ -236,6 +245,8 @@ namespace Hazel
 
             while (readMore)
             {
+                if (this.BytesRemaining < 1) throw new InvalidDataException($"Read length is longer than message length.");
+
                 byte b = this.ReadByte();
                 if (b >= 0x80)
                 {