From ab0c15aa1ba45d083e2652b1dd3a397db51bcd4f Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 1 May 2020 15:40:26 +0100 Subject: [PATCH] address PR feedback --- .../tests/Cbor.Tests/CborReaderTests.SkipValue.cs | 11 ++++++ .../tests/Cbor/CborReader.Map.cs | 25 +++++++------ .../tests/Cbor/CborReader.SkipValue.cs | 6 +-- .../tests/Cbor/CborReader.cs | 43 +++++++++++++++------- .../tests/Cbor/CborWriter.Integer.cs | 22 +++++------ .../tests/Cbor/CborWriter.Map.cs | 12 +++--- 6 files changed, 72 insertions(+), 47 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.SkipValue.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.SkipValue.cs index eaa8fd5..95b980e 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.SkipValue.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborReaderTests.SkipValue.cs @@ -140,6 +140,17 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor }.Select(l => new object[] { l.Level, l.Encoding }); [Fact] + internal static void SkipValue_SkippedValueFollowedByNonConformingValue_ShouldThrowFormatException() + { + byte[] encoding = "827fff7fff".HexToByteArray(); + var reader = new CborReader(encoding, CborConformanceLevel.Ctap2Canonical); + + reader.ReadStartArray(); + reader.SkipValue(); + Assert.Throws(() => reader.ReadTextString()); + } + + [Fact] public static void SkipValue_NestedFormatException_ShouldPreserveOriginalReaderState() { string hexEncoding = "820181bf01ff"; // [1, [ {_ 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 index 7c9bb3e..c8328c1 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.Map.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.Map.cs @@ -56,7 +56,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor throw new InvalidOperationException("Not at end of indefinite-length map."); } - if (!_curentItemIsKey) + if (!_currentItemIsKey) { throw new FormatException("CBOR map key is missing a value."); } @@ -78,28 +78,31 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor private void HandleMapKeyAdded() { - Debug.Assert(_currentKeyOffset != null && _curentItemIsKey); + Debug.Assert(_currentKeyOffset != null && _currentItemIsKey); (int Offset, int Length) currentKeyRange = (_currentKeyOffset.Value, _bytesRead - _currentKeyOffset.Value); - if (_isConformanceLevelCheckEnabled && CborConformanceLevelHelpers.RequiresSortedKeys(ConformanceLevel)) + if (_isConformanceLevelCheckEnabled) { - ValidateSortedKeyEncoding(currentKeyRange); - } - else if (_isConformanceLevelCheckEnabled && CborConformanceLevelHelpers.RequiresUniqueKeys(ConformanceLevel)) - { - ValidateKeyUniqueness(currentKeyRange); + if (CborConformanceLevelHelpers.RequiresSortedKeys(ConformanceLevel)) + { + ValidateSortedKeyEncoding(currentKeyRange); + } + else if (CborConformanceLevelHelpers.RequiresUniqueKeys(ConformanceLevel)) + { + ValidateKeyUniqueness(currentKeyRange); + } } - _curentItemIsKey = false; + _currentItemIsKey = false; } private void HandleMapValueAdded() { - Debug.Assert(_currentKeyOffset != null && !_curentItemIsKey); + Debug.Assert(_currentKeyOffset != null && !_currentItemIsKey); _currentKeyOffset = _bytesRead; - _curentItemIsKey = true; + _currentItemIsKey = true; } private void ValidateSortedKeyEncoding((int Offset, int Length) currentKeyRange) diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.SkipValue.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.SkipValue.cs index bc7c975..f5c5139 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.SkipValue.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.SkipValue.cs @@ -9,10 +9,6 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor { internal partial class CborReader { - // flag used to temporarily disable conformance level checks, - // e.g. during a skip operation over nonconforming encodings. - private bool _isConformanceLevelCheckEnabled = true; - public void SkipValue(bool validateConformance = false) => SkipToAncestor(0, validateConformance); public void SkipToParent(bool validateConformance = false) { @@ -44,7 +40,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor } finally { - _isConformanceLevelCheckEnabled = false; + _isConformanceLevelCheckEnabled = true; } } diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs index 3038c9b..d344f00 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborReader.cs @@ -50,10 +50,14 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor // Map-specific bookkeeping private int? _currentKeyOffset = null; - private bool _curentItemIsKey = false; + private bool _currentItemIsKey = false; private (int Offset, int Length)? _previousKeyRange; private HashSet<(int Offset, int Length)>? _previousKeyRanges; + // flag used to temporarily disable conformance level checks, + // e.g. during a skip operation over nonconforming encodings. + private bool _isConformanceLevelCheckEnabled = true; + // keeps a cached copy of the reader state; 'Unknown' denotes uncomputed state private CborReaderState _cachedState = CborReaderState.Unknown; @@ -137,7 +141,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor case CborMajorType.ByteString: return CborReaderState.EndByteString; case CborMajorType.TextString: return CborReaderState.EndTextString; case CborMajorType.Array: return CborReaderState.EndArray; - case CborMajorType.Map when !_curentItemIsKey: return CborReaderState.FormatError; + case CborMajorType.Map when !_currentItemIsKey: return CborReaderState.FormatError; case CborMajorType.Map: return CborReaderState.EndMap; default: Debug.Fail("CborReader internal error. Invalid CBOR major type pushed to stack."); @@ -298,7 +302,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor frameOffset: _frameOffset, remainingDataItems: _remainingDataItems, currentKeyOffset: _currentKeyOffset, - currentItemIsKey: _curentItemIsKey, + currentItemIsKey: _currentItemIsKey, previousKeyRange: _previousKeyRange, previousKeyRanges: _previousKeyRanges ); @@ -310,7 +314,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor _frameOffset = _bytesRead; _isTagContext = false; _currentKeyOffset = (type == CborMajorType.Map) ? (int?)_bytesRead : null; - _curentItemIsKey = type == CborMajorType.Map; + _currentItemIsKey = type == CborMajorType.Map; _previousKeyRange = null; _previousKeyRanges = null; } @@ -349,7 +353,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor _frameOffset = frame.FrameOffset; _remainingDataItems = frame.RemainingDataItems; _currentKeyOffset = frame.CurrentKeyOffset; - _curentItemIsKey = frame.CurrentItemIsKey; + _currentItemIsKey = frame.CurrentItemIsKey; _previousKeyRange = frame.PreviousKeyRange; _previousKeyRanges = frame.PreviousKeyRanges; // Popping items from the stack can change the reader state @@ -363,7 +367,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor { if (_currentKeyOffset != null) // is map context { - if(_curentItemIsKey) + if(_currentItemIsKey) { HandleMapKeyAdded(); } @@ -411,9 +415,14 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor private readonly struct StackFrame { - public StackFrame(CborMajorType type, int frameOffset, int? remainingDataItems, - int? currentKeyOffset, bool currentItemIsKey, - (int Offset, int Length)? previousKeyRange, HashSet<(int Offset, int Length)>? previousKeyRanges) + public StackFrame( + CborMajorType type, + int frameOffset, + int? remainingDataItems, + int? currentKeyOffset, + bool currentItemIsKey, + (int Offset, int Length)? previousKeyRange, + HashSet<(int Offset, int Length)>? previousKeyRanges) { MajorType = type; FrameOffset = frameOffset; @@ -440,9 +449,15 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor // reader is within the original context in which the checkpoint was created private readonly struct Checkpoint { - public Checkpoint(int bytesRead, int stackDepth, int frameOffset, int? remainingDataItems, - int? currentKeyOffset, bool currentItemIsKey, (int Offset, int Length)? previousKeyRange, - HashSet<(int Offset, int Length)>? previousKeyRanges) + public Checkpoint( + int bytesRead, + int stackDepth, + int frameOffset, + int? remainingDataItems, + int? currentKeyOffset, + bool currentItemIsKey, + (int Offset, int Length)? previousKeyRange, + HashSet<(int Offset, int Length)>? previousKeyRanges) { BytesRead = bytesRead; StackDepth = stackDepth; @@ -474,7 +489,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor frameOffset: _frameOffset, remainingDataItems: _remainingDataItems, currentKeyOffset: _currentKeyOffset, - currentItemIsKey: _curentItemIsKey, + currentItemIsKey: _currentItemIsKey, previousKeyRange: _previousKeyRange, previousKeyRanges: _previousKeyRanges); } @@ -504,7 +519,7 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor _frameOffset = checkpoint.FrameOffset; _remainingDataItems = checkpoint.RemainingDataItems; _currentKeyOffset = checkpoint.CurrentKeyOffset; - _curentItemIsKey = checkpoint.CurrentItemIsKey; + _currentItemIsKey = checkpoint.CurrentItemIsKey; _previousKeyRange = checkpoint.PreviousKeyRange; _previousKeyRanges = checkpoint.PreviousKeyRanges; diff --git a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Integer.cs b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Integer.cs index 0310be5..80bcd5c 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Integer.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Integer.cs @@ -52,30 +52,30 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor } else if (value <= byte.MaxValue) { - EnsureWriteCapacity(2); + EnsureWriteCapacity(1 + sizeof(byte)); WriteInitialByte(new CborInitialByte(type, CborAdditionalInfo.Additional8BitData)); _buffer[_offset++] = (byte)value; } else if (value <= ushort.MaxValue) { - EnsureWriteCapacity(3); + EnsureWriteCapacity(1 + sizeof(ushort)); WriteInitialByte(new CborInitialByte(type, CborAdditionalInfo.Additional16BitData)); BinaryPrimitives.WriteUInt16BigEndian(_buffer.AsSpan(_offset), (ushort)value); - _offset += 2; + _offset += sizeof(ushort); } else if (value <= uint.MaxValue) { - EnsureWriteCapacity(5); + EnsureWriteCapacity(1 + sizeof(uint)); WriteInitialByte(new CborInitialByte(type, CborAdditionalInfo.Additional32BitData)); BinaryPrimitives.WriteUInt32BigEndian(_buffer.AsSpan(_offset), (uint)value); - _offset += 4; + _offset += sizeof(uint); } else { - EnsureWriteCapacity(9); + EnsureWriteCapacity(1 + sizeof(ulong)); WriteInitialByte(new CborInitialByte(type, CborAdditionalInfo.Additional64BitData)); BinaryPrimitives.WriteUInt64BigEndian(_buffer.AsSpan(_offset), value); - _offset += 8; + _offset += sizeof(ulong); } } @@ -87,19 +87,19 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor } else if (value <= byte.MaxValue) { - return 2; + return 1 + sizeof(byte); } else if (value <= ushort.MaxValue) { - return 3; + return 1 + sizeof(ushort); } else if (value <= uint.MaxValue) { - return 5; + return 1 + sizeof(uint); } else { - return 9; + return 1 + sizeof(ulong); } } } 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 index d54c6f2..a65c4df 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Map.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor/CborWriter.Map.cs @@ -60,19 +60,19 @@ namespace System.Security.Cryptography.Encoding.Tests.Cbor if (CborConformanceLevelHelpers.RequiresUniqueKeys(ConformanceLevel)) { - HashSet ranges = GetKeyValueEncodingRanges(); + HashSet keys = GetKeyValueEncodingRanges(); - var currentKeyRange = new KeyValueEncodingRange( + var newKey = new KeyValueEncodingRange( offset: _currentKeyOffset.Value, keyLength: currentValueOffset - _currentKeyOffset.Value, - totalLength: 0 // totalLength not known, but is not significant w.r.t. key equality semantics + totalLength: -1 // totalLength not known, but is not significant w.r.t. key equality semantics ); - if (ranges.Contains(currentKeyRange)) + if (keys.Contains(newKey)) { // reset writer state to what existed before the offending key write - _buffer.AsSpan(currentKeyRange.Offset, _offset).Fill(0); - _offset = currentKeyRange.Offset; + _buffer.AsSpan(newKey.Offset, _offset).Fill(0); + _offset = newKey.Offset; throw new InvalidOperationException("Duplicate key encoding in CBOR map."); } -- 2.7.4