Add support for indefinite length arrays

[jozkee]

No description provided.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

null

Author:	Jozkee
Assignees:	Jozkee
Labels:	area-System.Security
Milestone:	8.0.0

[bartonjs]

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.

[bartonjs]

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.

[jozkee]

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.

[bartonjs]

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.

[jozkee]

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.

[bartonjs]

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)

[jozkee]

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.

[bartonjs]

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).

[jozkee]

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

[bartonjs]

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.