-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 1 commit
49a3629
caa8bc8
85361e4
e91be78
b159ee7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
} | ||
|
@@ -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(); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is I think I'd expect one of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I agree that 1 is the most likely value for Count; but I think we should use literally any other number for initial capacity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't know that was how |
||
|
||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
@@ -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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Indefinite:
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
(I think '7' is what'll report, at least). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar concern as your previous comment #74215 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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. |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.