Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for indefinite length arrays #74215

Merged
merged 5 commits into from
Aug 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@
<value>Critical Headers must be a definite-length CBOR array of at least one element.</value>
</data>
<data name="DecodeCoseSignatureMustBeArrayOfThree" xml:space="preserve">
<value>COSE Signature must be a definite-length array of 3 elements.</value>
<value>COSE Signature must be an array of 3 elements.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<value>COSE Signature must be an array of 3 elements.</value>
<value>COSE Signature must be an array of three elements.</value>

Since DecodeSign1ArrayLengthMustBeFour wrote out the word, and I think there's some sort of guideline somewhere that numbers less than 10 should use words instead of digits.

</data>
<data name="DecodeErrorWhileDecoding" xml:space="preserve">
<value>Error while decoding COSE message. {0}</value>
Expand All @@ -168,11 +168,14 @@
<data name="DecodeMessageContainedTrailingData" xml:space="preserve">
<value>CBOR payload contained trailing data after message was complete.</value>
</data>
<data name="DecodeMultiSignArrayLengthMustBeFour" xml:space="preserve">
<value>COSE_Sign must be an array of four elements.</value>
</data>
<data name="DecodeMultiSignIncorrectTag" xml:space="preserve">
<value>Incorrect tag. Expected Sign(98) or Untagged, Actual '{0}'.</value>
</data>
<data name="DecodeSign1ArrayLengthMustBeFour" xml:space="preserve">
<value>Array length for COSE_Sign1 must be four.</value>
<value>COSE_Sign1 must be an array of four elements.</value>
</data>
<data name="DecodeSign1EncodedProtectedMapIncorrect" xml:space="preserve">
<value>Protected map was incorrect.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,18 @@ private static void ValidateInsertion(CoseHeaderLabel label, CoseHeaderValue val
reader.SkipValue();
break;
case KnownHeaders.Crit:
int length = reader.ReadStartArray().GetValueOrDefault();
if (length < 1)
{
throw new ArgumentException(SR.CriticalHeadersMustBeArrayOfAtLeastOne, nameof(value));
}
int? length = reader.ReadStartArray();
jozkee marked this conversation as resolved.
Show resolved Hide resolved
bool isEmpty = true;

for (int i = 0; i < length; i++)
while (true)
{
CborReaderState state = reader.PeekState();
if (state == CborReaderState.UnsignedInteger || state == CborReaderState.NegativeInteger)
if (state == CborReaderState.EndArray)
{
reader.ReadEndArray();
break;
}
else if (state == CborReaderState.UnsignedInteger || state == CborReaderState.NegativeInteger)
{
reader.ReadInt32();
}
Expand All @@ -287,8 +289,13 @@ private static void ValidateInsertion(CoseHeaderLabel label, CoseHeaderValue val
{
throw new ArgumentException(SR.Format(SR.CoseHeaderMapHeaderDoesNotAcceptSpecifiedValue, label.LabelName), nameof(value));
}
isEmpty = false;
}

if (isEmpty)
{
throw new ArgumentException(SR.CriticalHeadersMustBeArrayOfAtLeastOne, nameof(value));
}
reader.SkipToParent();
break;
case KnownHeaders.ContentType:
if (initialState != CborReaderState.TextString &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ private static CoseSign1Message DecodeCoseSign1Core(CborReader reader)
}

int? arrayLength = reader.ReadStartArray();
if (arrayLength != 4)
if (arrayLength != null && arrayLength != CoseSign1Message.Sign1ArrayLength)
{
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.DecodeSign1ArrayLengthMustBeFour));
}
Expand Down Expand Up @@ -208,9 +208,9 @@ private static CoseMultiSignMessage DecodeCoseMultiSignCore(CborReader reader)
}

int? arrayLength = reader.ReadStartArray();
if (arrayLength != 4)
if (arrayLength != null && arrayLength != CoseMultiSignMessage.MultiSignArrayLength)
{
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.DecodeSign1ArrayLengthMustBeFour));
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.DecodeMultiSignArrayLengthMustBeFour));
}

var protectedHeaders = new CoseHeaderMap();
Expand Down Expand Up @@ -322,19 +322,12 @@ private static byte[] DecodeSignature(CborReader reader)
private static List<CoseSignature> DecodeCoseSignaturesArray(CborReader reader, byte[] bodyProtected)
{
int? signaturesLength = reader.ReadStartArray();
List<CoseSignature> signatures = new List<CoseSignature>(signaturesLength ?? 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 1 the best fallback value here? Given that it's a multi, at least one is expected, and two or three doesn't seem unheard of.

I think I'd expect one of

  • 0: Let List decide its own default (4)
  • 4: List's default, bigger than the number we expect.
  • 10: Much bigger than we expect.
  • Doing some pre-counting to just get the right answer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 1 the best fallback value here?

Yes, this is reading a [+COSE_Signature] i.e: the array of signatures, which is required to have at least one. If we have more, the list will grow, if we have less (zero), we will throw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, at least one item is expected, but why are we putting the List into its worst possible state of growth?

If you make a List(1), then it has 3 entries, it goes

  • new List(1)
    • storage = new T[1];
  • Add 1
  • Add 2
    • Too small! Double in size. new T[2]
  • Add 3
    • Too small! Double in size. new T[4]

new List<T>(0) says "choose your own default size" (which is 4); and any number other than 1 skips the 1->2 growth phase.

I agree that 1 is the most likely value for Count; but I think we should use literally any other number for initial capacity.

Copy link
Member Author

@jozkee jozkee Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new List(0) says "choose your own default size" (which is 4); and any number other than 1 skips the 1->2 growth phase.

Ah, I didn't know that was how List's growth worked. In that case, we're better off using the pre-count since we're going to do it anyway.


if (signaturesLength.GetValueOrDefault() < 1)
{
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.MultiSignMessageMustCarryAtLeastOneSignature));
}

List<CoseSignature> signatures = new List<CoseSignature>(signaturesLength!.Value);

for (int i = 0; i < signaturesLength; i++)
while (reader.PeekState() == CborReaderState.StartArray)
{
int? length = reader.ReadStartArray();

if (length != CoseMultiSignMessage.CoseSignatureArrayLength)
if (length != null && length != CoseMultiSignMessage.CoseSignatureArrayLength)
{
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.DecodeCoseSignatureMustBeArrayOfThree));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A definite array that is the wrong length will get a nice exception message.

An indefinite array that is the wrong length will presumably get a CBOR exception of some sort.

The best way I'm seeing to unify on the informative message, off the top of my head, would be to keep peeking for EndArray before (or in) the field reader helper methods. Or to just precount the values for indefinite arrays.

(Right now precounting looks to be hard with CborReader, since it doesn't have PeekEncodedValue, but we could add one)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think is worth adding extra logic for validation of indefinite-length arrays, I don't envision that they will be commonly used for COSE objects given that the spec is very clear about the length of the arrays used and they are not unreasonably large to not use a definite length in most cases.

However, if we wanted to check up-front the length of indefinite-length arrays, we could use ReadEncodedValue() and then create another reader (new CborReader(encodedValue)) and use SkipValue() to count the elements. Then just throw away said reader and continue with the original one.

}
Expand All @@ -351,13 +344,18 @@ private static List<CoseSignature> DecodeCoseSignaturesArray(CborReader reader,
}

byte[] signatureBytes = DecodeSignature(reader);
reader.ReadEndArray();

signatures.Add(new CoseSignature(protectedHeaders, unprotectedHeaders, bodyProtected, signProtected, signatureBytes));

reader.ReadEndArray();
}

reader.ReadEndArray();

if (signatures.Count < 1)
{
throw new CryptographicException(SR.Format(SR.DecodeErrorWhileDecoding, SR.MultiSignMessageMustCarryAtLeastOneSignature));
}

return signatures;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace System.Security.Cryptography.Cose
/// </summary>
public sealed class CoseMultiSignMessage : CoseMessage
{
private const int MultiSignArrayLength = 4;
internal const int MultiSignArrayLength = 4;
private const int MultiSignSizeOfCborTag = 2;
internal const int CoseSignatureArrayLength = 3;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace System.Security.Cryptography.Cose
/// </summary>
public sealed class CoseSign1Message : CoseMessage
{
private const int Sign1ArrayLength = 4;
internal const int Sign1ArrayLength = 4;
private const int Sign1SizeOfCborTag = 1;
private readonly byte[] _signature;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,41 @@ public void RemoveKeyValuePair_DoesNotMatchKeyOrValue(bool changeKey)
Assert.Equal(1, map.Count);
}

[Fact]
public void SetEncodedValue_CriticalHeaders_ThrowIf_ArrayEmpty()
{
// definite length
var writer = new CborWriter();
writer.WriteStartArray(0);
writer.WriteEndArray();

Verify(writer.Encode());

// indefinite length
writer.Reset();
writer.WriteStartArray(null);
writer.WriteEndArray();

Verify(writer.Encode());

void Verify(byte[] encodedValue)
{
CoseHeaderMap map = new();
CoseHeaderValue value = CoseHeaderValue.FromEncodedValue(writer.Encode());
Assert.Throws<ArgumentException>(() => map[CoseHeaderLabel.CriticalHeaders] = value);
}
}

[Fact]
public void SetEncodedValue_CriticalHeaders_ThrowIf_IndefiniteLengthArrayMissingBreak()
{
byte[] encodedValue = GetDummyCritHeaderValue(useIndefiniteLength: true);

CoseHeaderMap map = new();
CoseHeaderValue value = CoseHeaderValue.FromEncodedValue(encodedValue.AsSpan(0, encodedValue.Length - 1));
Assert.Throws<ArgumentException>(() => map[CoseHeaderLabel.CriticalHeaders] = value);
}

public enum SetValueMethod
{
ItemSet,
Expand Down Expand Up @@ -493,7 +528,11 @@ public static IEnumerable<object[]> KnownHeadersEncodedValues_TestData()
writer.WriteInt32((int)ECDsaAlgorithm.ES256);
yield return ReturnDataAndReset(KnownHeaderAlg, writer, setMethod, getMethod);

WriteDummyCritHeaderValue(writer);
WriteDummyCritHeaderValue(writer, useIndefiniteLength: false);
yield return ReturnDataAndReset(KnownHeaderCrit, writer, setMethod, getMethod);


WriteDummyCritHeaderValue(writer, useIndefiniteLength: true);
yield return ReturnDataAndReset(KnownHeaderCrit, writer, setMethod, getMethod);

writer.WriteTextString(ContentTypeDummyValue);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Formats.Cbor;
using Test.Cryptography;
Expand Down Expand Up @@ -82,5 +83,52 @@ public void DecodeMultiSign_IncorrectStructure()
writer.WriteEndArray();
Assert.Throws<CryptographicException>(() => CoseMessage.DecodeMultiSign(writer.Encode()));
}

[Theory]
// COSE_Sign is an indefinite-length array
[InlineData("D8629F40A054546869732069732074686520636F6E74656E742E818343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30AFF")]
// [+COSE_Signature]
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E9F8343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30AFF")]
// COSE_Signature
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E819F43A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30AFF")]
// All of them
[InlineData("D8629F40A054546869732069732074686520636F6E74656E742E9F9F43A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30AFFFFFF")]
public void DecodeMultiSign_IndefiniteLengthArray(string hexCborPayload)
{
byte[] cborPayload = ByteUtils.HexToByteArray(hexCborPayload);
CoseMultiSignMessage msg = CoseMessage.DecodeMultiSign(cborPayload);

ReadOnlyCollection<CoseSignature> signatures = msg.Signatures;
Assert.Equal(1, signatures.Count);
Assert.True(signatures[0].VerifyEmbedded(DefaultKey));
}

[Theory]
// COSE_Sign
[InlineData("D8629F40A054546869732069732074686520636F6E74656E742E818343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A")]
// [+COSE_Signature]
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E9F8343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A")]
// COSE_Signature
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E819F43A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A")]
public void DecodeMultiSign_IndefiniteLengthArray_MissingBreak(string hexCborPayload)
{
byte[] cborPayload = ByteUtils.HexToByteArray(hexCborPayload);
CryptographicException ex = Assert.Throws<CryptographicException>(() => CoseMessage.DecodeMultiSign(cborPayload));
Assert.IsType<CborContentException>(ex.InnerException);
}

[Theory]
// COSE_Sign
[InlineData("D8629F40A054546869732069732074686520636F6E74656E742E818343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A40FF")]
// [+COSE_Signature]
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E9F8343A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A40FF")]
// COSE_Signature
[InlineData("D8628440A054546869732069732074686520636F6E74656E742E819F43A10126A1044231315840E2AEAFD40D69D19DFE6E52077C5D7FF4E408282CBEFB5D06CBF414AF2E19D982AC45AC98B8544C908B4507DE1E90B717C3D34816FE926A2B98F53AFD2FA0F30A40FF")]
public void DecodeMultiSign_IndefiniteLengthArray_LargerByOne(string hexCborPayload)
{
byte[] cborPayload = ByteUtils.HexToByteArray(hexCborPayload);
CryptographicException ex = Assert.Throws<CryptographicException>(() => CoseMessage.DecodeMultiSign(cborPayload));
Assert.IsType<InvalidOperationException>(ex.InnerException);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,30 @@ public void DecodeSign1_IncorrectStructure()
writer.WriteEndArray();
Assert.Throws<CryptographicException>(() => CoseMessage.DecodeSign1(writer.Encode()));
}

[Fact]
public void DecodeSign1_IndefiniteLengthArray()
{
byte[] cborPayload = ByteUtils.HexToByteArray("D29F43A10126A10442313154546869732069732074686520636F6E74656E742E58408EB33E4CA31D1C465AB05AAC34CC6B23D58FEF5C083106C4D25A91AEF0B0117E2AF9A291AA32E14AB834DC56ED2A223444547E01F11D3B0916E5A4C345CACB36FF");
CoseSign1Message msg = CoseMessage.DecodeSign1(cborPayload);

Assert.True(msg.VerifyEmbedded(DefaultKey));
}

[Fact]
public void DecodeSign1_IndefiniteLengthArray_MissingBreak()
{
byte[] cborPayload = ByteUtils.HexToByteArray("D29F43A10126A10442313154546869732069732074686520636F6E74656E742E58408EB33E4CA31D1C465AB05AAC34CC6B23D58FEF5C083106C4D25A91AEF0B0117E2AF9A291AA32E14AB834DC56ED2A223444547E01F11D3B0916E5A4C345CACB36");
CryptographicException ex = Assert.Throws<CryptographicException>(() => CoseMessage.DecodeSign1(cborPayload));
Assert.IsType<CborContentException>(ex.InnerException);
}

[Fact]
public void DecodeSign1_IndefiniteLengthArray_LargerByOne()
{
byte[] cborPayload = ByteUtils.HexToByteArray("D29F43A10126A10442313154546869732069732074686520636F6E74656E742E58408EB33E4CA31D1C465AB05AAC34CC6B23D58FEF5C083106C4D25A91AEF0B0117E2AF9A291AA32E14AB834DC56ED2A223444547E01F11D3B0916E5A4C345CACB3640FF");
CryptographicException ex = Assert.Throws<CryptographicException>(() => CoseMessage.DecodeSign1(cborPayload));
Assert.IsType<InvalidOperationException>(ex.InnerException);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels to me like this exception should look the same as the definite array case.

Definite:

System.Security.Cryptography.CryptographicException: Error while decoding COSE message. COSE_Sign1 must be an array of four elements.

Indefinite:

System.Security.Cryptography.CryptographicException: Error while decoding COSE message. See the inner exception for details.
----- Inner Exception -----
  System.InvalidOperationException: Not at end of the indefinite-length data item.

And for the "one too short case" (which could use a test) we get the same informative exception with the definite length, but indefinite becomes

System.Security.Cryptography.CryptographicException: Error while decoding COSE message. See the inner exception for details.
----- Inner Exception -----
  System.InvalidOperationException: Cannot perform the requested operation, the next CBOR data item is of major type '7'

(I think '7' is what'll report, at least).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar concern as your previous comment #74215 (comment).
Is it worth to check up-front indefinite-length arrays length just to get nice exceptions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of potential reasons someone gets here, but one of them is getting the method call backwards between COSE_Sign1 and COSE_Sign formats. In the case of definite length arrays there's a "you might be the wrong item" message. For indefinite the message is way more obscure.

Assuming the developer trying to read a file is not the one who wrote it, they don't control the definite or indefinite encoding; so it seems wrong to punish them for being in a corner case. So, yeah, I think we should check the length up front.

I agree that the only way to really pre-count it is to use ReadEncodedValue() and make a second reader (and, for indefinite, a third one), but ideally we won't do that when it's a definite length. So my recommendation is probably

  • Use pre-counting now.
  • Noodle around in the CBOR project to make sure that adding a PeekEncodedValue() member is as easy as I think it should be
  • API proposal for PeekEncodedValue()
  • When implementing PeekEncodedValue() also come here and make use of it:
ReadOnlyMemory<byte> encodedArray = reader.PeekEncodedValue();
int? len = reader.ReadStartArray();

if (!len.HasValue)
{
    // make a reader over encodedArray, count how many items were in it.
}

if (len != TheAppropriateConstant)
{
    throw new ...
}

That makes definite arrays continue as they are (with a minor hit for double-reading the start array and saving the ReadOnlyMemory), and only indefinite arrays pay the allocation of a second reader, and there's never a third reader.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -541,17 +541,17 @@ internal enum CoseMessageKind
MultiSign = 98
}

internal static void WriteDummyCritHeaderValue(CborWriter writer)
internal static void WriteDummyCritHeaderValue(CborWriter writer, bool useIndefiniteLength = false)
{
writer.WriteStartArray(1);
writer.WriteStartArray(useIndefiniteLength ? null : 1);
writer.WriteInt32(42);
writer.WriteEndArray();
}

internal static byte[] GetDummyCritHeaderValue()
internal static byte[] GetDummyCritHeaderValue(bool useIndefiniteLength = false)
{
var writer = new CborWriter();
WriteDummyCritHeaderValue(writer);
WriteDummyCritHeaderValue(writer, useIndefiniteLength);
return writer.Encode();
}

Expand Down