sketching a rationale for the negative tests

[thomas-fossati]

No description provided.

[qmuntal]

LGTM

[thomas-fossati]

Hey @shizhMSFT, could you please have a look?

[qmuntal]

AFAIU the spec does not enforce the alg label to be present in the COSE_Sign1 nor COSE_Sign, so these negative conditions are too strict.

[shizhMSFT]

Reference: https://datatracker.ietf.org/doc/html/rfc8152#section-3.1

alg: This parameter is used to indicate the algorithm used for the
security processing. This parameter MUST be authenticated where
the ability to do so exists. This support is provided by AEAD
algorithms or construction (COSE_Sign, COSE_Sign0, COSE_Mac, and
COSE_Mac0). This authentication can be done either by placing the
header in the protected header bucket or as part of the externally
supplied data. The value is taken from the "COSE Algorithms"
registry (see Section 16.4).

and also

 Generic_Headers = (
     ? 1 => int / tstr,  ; algorithm identifier
     ? 2 => [+label],    ; criticality
     ? 3 => tstr / int,  ; content type
     ? 4 => bstr,        ; key identifier
     ? 5 => bstr,        ; IV
     ? 6 => bstr,        ; Partial IV
     ? 7 => COSE_Signature / [+COSE_Signature] ; Counter signature
 )

[thomas-fossati]

AFAIU the spec does not enforce the alg label to be present in the COSE_Sign1 nor COSE_Sign, so these negative conditions are too strict.

The spec says "This header parameter MUST be authenticated where the ability to do so exists." - which is always the case for Sign1 - and gives us two ways to achieve that, namely:

1. placing the header parameter in the protected-header-parameters bucket
2. as part of the externally supplied data.

Now, since "external" is not present in the test vector (i.e., we assume no OOB channel), the only choice for the implementation is to pull it from protected header. And since is not there either, our "Sign1 producer" seems to have failed to adhere to the spec, no?

[qmuntal]

This level of detail goes out of my knowledge. I'll agree with whatever you and @shizhMSFT decide.

[shizhMSFT]

@thomas-fossati Can I interpret

This parameter MUST be authenticated where the ability to do so exists.

as that the alg header must present either in the protected header or the external data?

In other words, if externally supplied data is nil or empty, alg MUST exists in the protected header?

[shizhMSFT]

If externally supplied data exists, the caller of the go-cose library should check alg by itself?

[yogeshbdeshpande]

@thomas-fossati , @qmuntal @shizhMSFT : As far as I understand, three unique cases exists:

1. alg-id present in protected header
2. alg-id part of external data
3. Implicit alg-id applied. (id not present)

My understanding based on https://www.rfc-editor.org/authors/rfc9052.html#appendix-A

@thomas-fossati : Are we planning a test involving item 3) as well ? If yes, I am ok if we add a github issue to do this, in future, so as to proceed with current changes.

[thomas-fossati]

@thomas-fossati Can I interpret

This parameter MUST be authenticated where the ability to do so exists.

as that the alg header must present either in the protected header or the external data?

In other words, if externally supplied data is nil or empty, alg MUST exists in the protected header?

Yes, I think so.

[thomas-fossati]

If externally supplied data exists, the caller of the go-cose library should check alg by itself?

I guess the UX angle here is that the API should allow setting the verification Algorithm explicitly to allow for the case where the peers use external data to carry the alg.

[thomas-fossati]

@thomas-fossati : Are we planning a test involving item 3) as well ?

No, that is completely application specific.

[qmuntal]

AFAIU the spec recommends using shorted encoding possible, but it does not enforce it, so the receiver should not reject it.

[shizhMSFT]

Reference: https://datatracker.ietf.org/doc/html/rfc8152#section-14

The restriction applies to the encoding of the Sig_structure, the
Enc_structure, and the MAC_structure.

For signing, Sig_structure is an intermediate structure for computing the digest and is not in COSE_Sign or COSE_Sign1 messages.

Sig_structure = [
     context : "Signature" / "Signature1" / "CounterSignature",
     body_protected : empty_or_serialized_map,
     ? sign_protected : empty_or_serialized_map,
     external_aad : bstr,
     payload : bstr
]

[thomas-fossati]

If I read https://www.rfc-editor.org/authors/rfc9052.html#section-9-2.2 correctly, using the shortest encoding is an actual MUST.

[thomas-fossati]

OK, I've just re-read the spec. Thanks @shizhMSFT for pointing me in the right direction - I got confused by the scope being a bullet point itself. I agree with you and will drop negative tests 6 and 7.

[laurencelundblade]

Hi, I'm just peanut gallery for now, not much bandwidth to do much here...

Maybe you already are planning critical headers tests. They are definitely needed.

The negative tests that t_cose tests with are partly represented in this header here: https://github.com/laurencelundblade/t_cose/blob/master/test/t_cose_make_test_messages.h

I modified the t_cose signing code to make bad COSE_Sign1 messages for a lot of the negative tests. Many of them are for critical headers. Also a few of them are for not-well-formed CBOR.

Also worry a large and grand test suite for COSE might end up having to have a core common set of tests and a series of tests on top of that for different profiles because there are so many different ways to use COSE. The algorithm ID discussion above is an example.

Glad to see this work happening!

[SteveLasker]

Thanks @laurencelundblade for the pointers.