S.S.C.Cose: Add new MultiSign APIs and address API review feedback on existing ones

[jozkee]

It also addresses all bullets on Checkpoint MVP+2 #62600.

Fixes #62599.
Fixes #69896.
Fixes #70189 (5fb06cc).

[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

---

It also addresses all bullets on Checkpoint MVP+2 #62600.

Fixes #62599.
Fixes #69896.

TODO:

• Extend tests to use Pkcs1.
• Add tests for CoseHeaderValue type.

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

[bartonjs]

This exception doesn't make sense when there is at least one signature but it used an indefinite length array.

Assuming I recall correctly and that COSE forbids indefinite length arrays then there should be a check for !signaturesLength.HasValue, and a special throw, before this.

[bartonjs]

(Repeat for basically every place that calls ReadStartArray)

[bartonjs]

One thing to consider is making a struct wrapper over top of a CborReader (e.g. CoseCborReader) and have it enforce all of these policy things. E.g.

internal struct CoseCborReader
{
    private CborReader _reader;

    internal CoseCborReader(...)
    {
        ...;
    }

    internal int ReadStartArray()
    {
        int? len;

        try
        {
            len = _reader.ReadStartArray();
        }
        catch (CborContentException e)
        {
            ...
        }

        if (!len.HasValue)
        {
            throw new CryptographicException(SR.CoseForbidsIndefiniteArray);
        }

        return len.GetValueOrDefault();
    }
}

In core crypto I've done a similar thing. The AsnValueReader type is a variant of AsnReader, but it always wraps AsnContentException in a CryptographicException (because that's the exception type that the methods threw before we wrote AsnReader)

[jozkee]

From https://datatracker.ietf.org/doc/html/rfc8152#section-14, it seems to me that indefinite length is only forbidden for Sig_Structure, the text on that section probably also means that we should use a CborWriter with Canonical conformance mode on CreateToBeSigned.

I can fix the exception messages stating that indefinite-length arrays are forbidden as part of this PR but I would prefer a separate PR to address/validate support of messages using indefinite length arrays, what do you think?

I can't find anything right now stating that indefinite length arrays are forbidden, so for example, can you receive a COSE_Sign message inside a indefinite-length array?

[bartonjs]

If indefinite length arrays are already inconsistently supported and you'd have to touch currently untouched code to fix it up, then a separate PR makes more sense. If everything that would have to be changed is in this PR already, then we shouldn't merge code we know is wrong.

[jozkee]

The current state of the code is the former, an example is CoseMessage.DecodeSign1, it requires a CBOR array of length 4 to successfully decode.

[bartonjs]

Suggested change

	throw new CryptographicException(SR.CoseSignerRSAKeyNeedsPadding);
	throw new ArgumentException(SR.CoseSignerRSAKeyNeedsPadding, nameof(key));

[bartonjs]

Suggested change

	map.TryGetValue(label, out CoseHeaderValue value);
	return value.EncodedValue.ToArray();
	bool found = map.TryGetValue(label, out CoseHeaderValue value);
	Assert.True(found, "TryGetValue on a known value");
	return value.EncodedValue.ToArray();