implement appropriate rollback semantics for key conformance validation

Chryssie · Apr 29, 2020 · 1c0d13d · 1c0d13d
1 parent 236aa65
commit 1c0d13d
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 7 deletions.
diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.Map.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.Map.cs
@@ -191,7 +191,17 @@ internal static void ReadMap_DuplicateKeys_StrictConformance_ShouldThrowFormatEx
             reader.ReadStartMap();
             Helpers.VerifyValue(reader, dupeKey);
             reader.ReadInt32();
+
+            int bytesRead = reader.BytesRead;
+            int bytesRemaining = reader.BytesRemaining;
+            CborReaderState state = reader.PeekState();
+
             Assert.Throws<FormatException>(() => Helpers.VerifyValue(reader, dupeKey));
+
+            // ensure reader state is preserved
+            Assert.Equal(bytesRead, reader.BytesRead);
+            Assert.Equal(bytesRemaining, reader.BytesRemaining);
+            Assert.Equal(state, reader.PeekState());
         }
 
         [Theory]
@@ -227,8 +237,17 @@ internal static void ReadMap_UnsortedKeys_ConformanceRequiringSortedKeys_ShouldT
                 reader.ReadInt32(); // value is always an integer
             }
 
+            int bytesRead = reader.BytesRead;
+            int bytesRemaining = reader.BytesRemaining;
+            CborReaderState state = reader.PeekState();
+
             // the final element violates sorting invariant
             Assert.Throws<FormatException>(() => Helpers.VerifyValue(reader, keySequence.Last()));
+
+            // ensure reader state is preserved
+            Assert.Equal(bytesRead, reader.BytesRead);
+            Assert.Equal(bytesRemaining, reader.BytesRemaining);
+            Assert.Equal(state, reader.PeekState());
         }
 
         [Theory]

diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborWriterTests.Map.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborWriterTests.Map.cs
@@ -159,6 +159,43 @@ internal static void WriteMap_DuplicateKeys_StrictConformance_ShouldFail(CborCon
             Assert.Throws<InvalidOperationException>(() => Helpers.WriteValue(writer, dupeKey));
         }
 
+        [Theory]
+        [InlineData(CborConformanceLevel.Strict, 42)]
+        [InlineData(CborConformanceLevel.Rfc7049Canonical, 42)]
+        [InlineData(CborConformanceLevel.Ctap2Canonical, 42)]
+        [InlineData(CborConformanceLevel.Strict, "foobar")]
+        [InlineData(CborConformanceLevel.Rfc7049Canonical, "foobar")]
+        [InlineData(CborConformanceLevel.Ctap2Canonical, "foobar")]
+        [InlineData(CborConformanceLevel.Strict, new object[] { new object[] { "x", "y" } })]
+        [InlineData(CborConformanceLevel.Rfc7049Canonical, new object[] { new object[] { "x", "y" } })]
+        [InlineData(CborConformanceLevel.Ctap2Canonical, new object[] { new object[] { "x", "y" } })]
+        internal static void WriteMap_DuplicateKeys_StrictConformance_ShouldBeRecoverableError(CborConformanceLevel level, object dupeKey)
+        {
+            byte[] expected = PerformWrite(attemptDuplicateWrite: false);
+            byte[] actual = PerformWrite(attemptDuplicateWrite: true);
+            Assert.Equal(expected.ByteArrayToHex(), actual.ByteArrayToHex());
+
+            byte[] PerformWrite(bool attemptDuplicateWrite)
+            {
+                using var writer = new CborWriter(level);
+                writer.WriteStartMap(2);
+                Helpers.WriteValue(writer, dupeKey);
+                writer.WriteInt32(0);
+
+                if (attemptDuplicateWrite)
+                {
+                    Assert.Throws<InvalidOperationException>(() => Helpers.WriteValue(writer, dupeKey));
+                }
+
+                // wrap dupe key in an array to satisfy key sorting & uniqueness constraints
+                Helpers.WriteValue(writer, new object[] { dupeKey }); 
+                writer.WriteInt32(0);
+                writer.WriteEndMap();
+
+                return writer.ToArray();
+            }
+        }
+
         [Theory]
         [InlineData(0)]
         [InlineData(1)]

diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.Map.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.Map.cs
@@ -115,10 +115,12 @@ private void ValidateSortedKeyEncoding((int Offset, int Length) currentKeyRange)
                 int cmp = CborConformanceLevelHelpers.CompareEncodings(previousKeyEncoding, currentKeyEncoding, ConformanceLevel);
                 if (cmp > 0)
                 {
+                    ResetBuffer(currentKeyRange.Offset);
                     throw new FormatException("CBOR map keys are not in sorted encoding order.");
                 }
                 else if (cmp == 0 && CborConformanceLevelHelpers.RequiresUniqueKeys(ConformanceLevel))
                 {
+                    ResetBuffer(currentKeyRange.Offset);
                     throw new FormatException("CBOR map contains duplicate keys.");
                 }
             }
@@ -134,6 +136,7 @@ private void ValidateKeyUniqueness((int Offset, int Length) currentKeyRange)
 
             if (!previousKeys.Add(currentKeyRange))
             {
+                ResetBuffer(currentKeyRange.Offset);
                 throw new FormatException("CBOR map contains duplicate keys.");
             }
         }

diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs
@@ -361,6 +361,14 @@ private void AdvanceBuffer(int length)
             _cachedState = CborReaderState.Unknown;
         }
 
+        private void ResetBuffer(int position)
+        {
+            _buffer = _originalBuffer.Slice(position);
+            _bytesRead = position;
+            // invalidate the state cache
+            _cachedState = CborReaderState.Unknown;
+        }
+
         private void EnsureBuffer(int length)
         {
             if (_buffer.Length < length)

diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Map.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Map.cs
@@ -67,23 +67,28 @@ private void HandleKeyWritten()
         {
             Debug.Assert(_currentKeyOffset != null && _currentValueOffset == null);
 
-            _currentValueOffset = _offset;
+            int currentValueOffset = _offset;
 
             if (CborConformanceLevelHelpers.RequiresUniqueKeys(ConformanceLevel))
             {
                 SortedSet<(int Offset, int KeyLength, int TotalLength)> ranges = GetKeyValueEncodingRanges();
 
                 (int Offset, int KeyLength, int TotalLength) currentKeyRange =
                     (_currentKeyOffset.Value,
-                     _currentValueOffset.Value - _currentKeyOffset.Value,
-                     0);
+                     currentValueOffset - _currentKeyOffset.Value,
+                     0); 
 
                 if (ranges.Contains(currentKeyRange))
                 {
-                    // TODO: check if rollback is necessary here
+                    // reset writer state to before the offending key write
+                    _buffer.AsSpan(currentKeyRange.Offset, _offset).Fill(0);
+                    _offset = currentKeyRange.Offset;
+
                     throw new InvalidOperationException("Duplicate key encoding in CBOR map.");
                 }
             }
+
+            _currentValueOffset = currentValueOffset;
         }
 
         private void HandleValueWritten()

diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.cs
@@ -161,9 +161,6 @@ private void PopDataItem(CborMajorType expectedType)
 
         private void AdvanceDataItemCounters()
         {
-            _remainingDataItems--;
-            _isTagContext = false;
-
             if (_currentKeyOffset != null) // this is a map context
             {
                 if (_currentValueOffset == null)
@@ -175,6 +172,9 @@ private void AdvanceDataItemCounters()
                     HandleValueWritten();
                 }
             }
+
+            _remainingDataItems--;
+            _isTagContext = false;
         }
 
         private void WriteInitialByte(CborInitialByte initialByte)