Ensure critical header contains valid labels by qmuntal · Pull Request #78 · veraison/go-cose

qmuntal · 2022-06-20T09:48:12Z

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

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 · 2022-06-20T10:57:31Z

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 · 2022-06-20T11:04:31Z

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 · 2022-06-20T11:16:06Z

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 · 2022-06-20T11:17:50Z

Reviewing now: let me check and comment!

thomas-fossati · 2022-06-20T11:20:58Z

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 · 2022-06-20T11:26:08Z

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 · 2022-06-20T11:46:07Z

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

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

headers.go

thomas-fossati · 2022-06-20T12:11:01Z

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

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

yogeshbdeshpande

LGTM!

shizhMSFT

LGTM

thomas-fossati · 2022-06-21T09:44:39Z

headers.go

+	case uint64:
+		label = int64(v)


should we intercept the potential overflow here?

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

Sure. #79 for tracking this.

thomas-fossati · 2022-06-21T09:45:55Z

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!

qmuntal added 2 commits June 20, 2022 11:23

enusre critical header contains valid labels

0845404

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

format switch

936bb8f

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

qmuntal changed the title ~~Enusre critical header contains valid labels~~ Ensure critical header contains valid labels Jun 20, 2022

shizhMSFT approved these changes Jun 20, 2022

View reviewed changes

thomas-fossati reviewed Jun 20, 2022

View reviewed changes

yogeshbdeshpande reviewed Jun 20, 2022

View reviewed changes

headers.go Outdated Show resolved Hide resolved

factor our label validation

e0a1374

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

yogeshbdeshpande approved these changes Jun 20, 2022

View reviewed changes

shizhMSFT approved these changes Jun 20, 2022

View reviewed changes

thomas-fossati reviewed Jun 21, 2022

View reviewed changes

thomas-fossati approved these changes Jun 21, 2022

View reviewed changes

thomas-fossati merged commit b870a00 into veraison:main Jun 21, 2022

Conversation

qmuntal commented Jun 20, 2022

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

qmuntal commented Jun 20, 2022

Uh oh!

thomas-fossati commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmuntal commented Jun 20, 2022

Uh oh!

yogeshbdeshpande commented Jun 20, 2022

Uh oh!

thomas-fossati commented Jun 20, 2022

Uh oh!

qmuntal commented Jun 20, 2022

Uh oh!

yogeshbdeshpande commented Jun 20, 2022

Uh oh!

Uh oh!

thomas-fossati commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

thomas-fossati Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

qmuntal Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

thomas-fossati Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

thomas-fossati commented Jun 21, 2022

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomas-fossati commented Jun 20, 2022 •

edited

Loading

thomas-fossati commented Jun 20, 2022 •

edited

Loading