Ensure critical header contains valid labels

[qmuntal]

Issue found by Trail of Bits during security review.

A protected header parameter containing a non-comparable element type, such a slice or a map, will produce a runtime panic while being encoded and decoded.

This is because the element is being used as map key to validate there are no missing critical headers, but non-comparable types can't be used as map keys, as per the Go spec.

Investigating this issue, I found we are missing to validate that the critical header array only contains valid labels (RFC 8152 Section 3.1), which are restricted to integers and strings (RFC 8152 Section 1.2).

This PR adds a new validation step which ensures that the elements of the critical header are valid labels when encoding and decoding a protected header.

By doing that we also fix the original issue, as integers and strings are always comparable.

@yogeshbdeshpande @shizhMSFT @thomas-fossati

Signed-off-by: qmuntal qmuntaldiaz@microsoft.com

[shizhMSFT]

LGTM

[thomas-fossati]

Thanks! Question: this seems scoped to the decoding of protected headers. Should there be a similar check for the unprotected blob?

[qmuntal]

Thanks! Question: this seems scoped to the decoding of protected headers. Should there be a similar check for the unprotected blob?

The critical parameters must be placed in the protected header, so the check does not apply to the unprotected header.

[thomas-fossati]

Thanks! Question: this seems scoped to the decoding of protected headers. Should there be a similar check for the unprotected blob?

The critical parameters must be placed in the protected header, so the check does not apply to the unprotected header.

Right, so if we want to be compliant with the COSE spec, shouldn't we also enforce the check on the unprotected headers map? ( Apologies in advance if this is a stupid question, I'm context-switching from something completely different and I may be slightly confused :-) )

[qmuntal]

Right, so if we want to be compliant with the COSE spec, shouldn't we also enforce the check on the unprotected headers map? ( Apologies in advance if this is a stupid question, I'm context-switching from something completely different and I may be slightly confused :-) )

Not stupid at all. I'll let @shizhMSFT and @yogeshbdeshpande decide, as I might also be missing some context.

The spec says:

crit: ... When present, this parameter MUST be placed in the protected header bucket.

This seems to imply that the unprotected header can't contain the crit parameter, so we have to error out when it happens.

[yogeshbdeshpande]

Reviewing now: let me check and comment!

[thomas-fossati]

Right, so if we want to be compliant with the COSE spec, shouldn't we also enforce the check on the unprotected headers map? ( Apologies in advance if this is a stupid question, I'm context-switching from something completely different and I may be slightly confused :-) )

Not stupid at all. I'll let @shizhMSFT and @yogeshbdeshpande decide, as I might also be missing some context.

The spec says:

crit: ... When present, this parameter MUST be placed in the protected header bucket.

This seems to imply that the unprotected header can't contain the crit parameter, so we have to error out when it happens.

True dat. What I was referencing is the other bit of the spec that says that it's an error to have labels that aren't int / tstr -- for which we seem to be missing the corresponding check. Again, unless I'm utterly confused.

[qmuntal]

True dat. What I was referencing is the other bit of the spec that says that it's an error to have labels that aren't int / tstr -- for which we seem to be missing the corresponding check. Again, unless I'm utterly confused.

The only other use of labels are header map keys, and we are already checking those are either integers or strings in validateHeaderLabel, so we are not missing any more checks, at least that I'm aware of.

[yogeshbdeshpande]

Understanding the thread and checking: There are two points of checking here, if I understand correctly.

1. Verify that the Value part of Protected Header, Critical Header Map(crit) is all Labels, which in turn could be int/tstr. This check ensures that.

2. Should we check whether all the Map Keys in all Headers (Protected and Unprotected) are all valid Labels.

I think, 2 does seem to be a valid check and we should address it. However ValidateHeaderLable used in both Marshal and UnMarshal so it is fine. My take.

[thomas-fossati]

True dat. What I was referencing is the other bit of the spec that says that it's an error to have labels that aren't int / tstr -- for which we seem to be missing the corresponding check. Again, unless I'm utterly confused.

The only other use of labels are header map keys, and we are already checking those are either integers or strings in validateHeaderLabel, so we are not missing any more checks, at least that I'm aware of.

Isn't that scoped to the decoding side only? If not, ignore me :-) , otherwise this bit of the spec may apply:

   The presence of a label in a COSE map that is not a string or an
   integer is an error.  Applications can either fail processing or
   process messages with incorrect labels; however, they MUST NOT create
   messages with incorrect labels.

in paricular, the however, they MUST NOT create [...] which refers to the Marshalling ops

[yogeshbdeshpande]

LGTM!

[shizhMSFT]

LGTM

[thomas-fossati]

should we intercept the potential overflow here?

[qmuntal]

I'll rather do it in another PR. This is clearly an issue but requires its own discussion.

[thomas-fossati]

Sure. #79 for tracking this.

[thomas-fossati]

True dat. What I was referencing is the other bit of the spec that says that it's an error to have labels that aren't int / tstr -- for which we seem to be missing the corresponding check. Again, unless I'm utterly confused.

The only other use of labels are header map keys, and we are already checking those are either integers or strings in validateHeaderLabel, so we are not missing any more checks, at least that I'm aware of.

Isn't that scoped to the decoding side only? If not, ignore me :-) , otherwise this bit of the spec may apply:

   The presence of a label in a COSE map that is not a string or an
   integer is an error.  Applications can either fail processing or
   process messages with incorrect labels; however, they MUST NOT create
   messages with incorrect labels.

in paricular, the however, they MUST NOT create [...] which refers to the Marshalling ops

Scratch that, I was talking rubbish.

[thomas-fossati]

🚢 it!