Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add untagged COSE_Sign1 support #143

Merged
merged 1 commit into from
May 18, 2023
Merged

Add untagged COSE_Sign1 support #143

merged 1 commit into from
May 18, 2023

Conversation

setrofim
Copy link
Contributor

RFC8152 specifies that a single-signer token can be either tagged (COSE_Sign1_Tagged) or untagged (COSE_Sign1). Add support for parsing, and optionally generating, the untagged version.

The default behaviour is to still generated COSE_Sign1_Tagged.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #143 (0d70fc7) into main (bfa9f54) will decrease coverage by 0.01%.
The diff coverage is 95.94%.

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   92.77%   92.76%   -0.01%     
==========================================
  Files          10       10              
  Lines        1024     1065      +41     
==========================================
+ Hits          950      988      +38     
- Misses         49       51       +2     
- Partials       25       26       +1     
Impacted Files Coverage Δ
sign1.go 90.10% <95.94%> (+0.74%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sign1.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left some minor nits, if we can tidy up further!

@shizhMSFT, @qmuntal Please review as well..

sign1_test.go Outdated
@@ -320,7 +343,7 @@ func TestSign1Message_UnmarshalCBOR(t *testing.T) {
0x40, 0xa0, // empty headers
0xf6, // payload
},
wantErr: "cbor: invalid COSE_Sign1_Tagged object",
wantErr: "cbor: CBOR tag isn't allowed",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error print does not align with the description of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not supposed to -- the descrition of the test says why a particular value is in valid. The returned error merely indicates that the value is invalid. The actual error comes form the underlying 'cbor' parser.

sign1_test.go Outdated
@@ -331,7 +354,7 @@ func TestSign1Message_UnmarshalCBOR(t *testing.T) {
0x41, 0x00, // signature
0x40,
},
wantErr: "cbor: invalid COSE_Sign1_Tagged object",
wantErr: "cbor: CBOR tag isn't allowed",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in line 357

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reply above.

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

IIUC, verification will swallow both COSE_Sign1 and COSE_Sign1_Tagged transparently?

sign1.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yogeshbdeshpande
Copy link
Contributor

@setrofim : For the convenience of the external reviewers, can you please also attach a Feature request github issue, to highlight the need for making this change?

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have security concerns in the untagged COSE_Sign1 support.

According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

We can have workaround with discussions like

type UntaggedSign1Message Sign1Message

func (m *UntaggedSign1Message) MarshalCBOR() ([]byte, error) {
    data, err := ((*Sign1Message)m).MarshalCBOR()
    if err != nil {
        return nil, err
    }
    return data[1:], nil
}

func (m *UntaggedSign1Message) UnmarshalCBOR(data []byte) error {
    return ((*Sign1Message)m).UnmarshalCBOR(append([]byte{sign1MessagePrefix[0]}, data...))
}

With above code, application should be able to decode the object according to the media type and fail early if there's anything wrong.

Of course, the above code is for illustration only and has no CPU / memory optimizations.

sign1.go Outdated
Comment on lines 91 to 75
var err error
var isUntagged bool

if bytes.HasPrefix(data, sign1MessagePrefix) {
err = decModeWithTagsForbidden.Unmarshal(data[1:], &raw)
isUntagged = false
} else {
err = decModeWithTagsForbidden.Unmarshal(data, &raw)
isUntagged = true
}
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still can perform the fast message check

Suggested change
var err error
var isUntagged bool
if bytes.HasPrefix(data, sign1MessagePrefix) {
err = decModeWithTagsForbidden.Unmarshal(data[1:], &raw)
isUntagged = false
} else {
err = decModeWithTagsForbidden.Unmarshal(data, &raw)
isUntagged = true
}
if err != nil {
return err
}
// fast message check
isUntagged := !bytes.HasPrefix(data, sign1MessagePrefix)
if isUntagged {
if !bytes.HasPrefix(data, sign1MessagePrefix[1:]) {
return errors.New("cbor: invalid COSE_Sign1 / COSE_Sign1_Tagged object")
}
data = data[1:]
}
// decode to sign1Message and parse
if err := decModeWithTagsForbidden.Unmarshal(data, &raw); err != nil {
return err
}

sign1.go Outdated Show resolved Hide resolved
@setrofim
Copy link
Contributor Author

setrofim commented May 16, 2023

@setrofim : For the convenience of the external reviewers, can you please also attach a Feature request github issue, to highlight the need for making this change?

This is more of a bug fix than a feature implementation -- the "request" here is simply "allow parsing of valid COSE structures".

@yogeshbdeshpande
Copy link
Contributor

yogeshbdeshpande commented May 16, 2023

rding to the media type and fail early if there's anything wrong.

ok, then please raise a github issue, explaining which section of specification we are not compliant with and what would be the
implications of non-compliance?

@thomas-fossati
Copy link
Contributor

thomas-fossati commented May 16, 2023

I have security concerns in the untagged COSE_Sign1 support.

According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

While I like the increased robustness in the approach you suggest, I don't think there's a security concern here. I reckon that the bit of spec you quote above is intended to protect against confusion attacks with different top-level formats (e.g., Sign vs Sign1) rather than within the same format (e.g., Sign1 vs Sign1_Tagged).

@setrofim
Copy link
Contributor Author

ok, then please raise a github issue, explaining which section of specification we are not compliant with and what would be the
implications of non-compliance?

This addressed in the commit message. Issues are to-do items there is no point in raising one for something that is already being done. They are not meant to provide context to existing changes -- the place for that is the commit message for the change.

@setrofim
Copy link
Contributor Author

setrofim commented May 16, 2023

I have security concerns in the untagged COSE_Sign1 support.
According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

While I like the increased robustness in the approach you suggest, I don't think there's a security concern here. I reckon that the bit of spec you quote above is intended to protect against confusion attacks with different top-level formats (e.g., Sign vs Sign1) rather than within the same format (e.g., Sign1 vs Sign1_Tagged).

I agree that @shizhMSFT's suggestion is neater and a lot more robust than my idea of using a field to sindicate tagged status. I've re-implemented untagged COSE_Sign1 support based on that. Please re-review.

@yogeshbdeshpande
Copy link
Contributor

LGTM!
Was wondering we should have alteast a one-liner in the main README.md to say, we support Untagged COSE also and refer to examples for how to use it?

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

sign1.go Show resolved Hide resolved
@setrofim setrofim force-pushed the setrofim branch 2 times, most recently from b7e6157 to c33b925 Compare May 17, 2023 09:21
RFC8152 specifies that a single-signer token can be either tagged
(COSE_Sign1_Tagged) or untagged (COSE_Sign1).

The existing Sign1Message implementation only handles COSE_Sign1_Tagged
objects. Add UntaggedSign1Message to analogously handle COSE_Sign1
objects.

Signed-off-by: setrofim <setrofim@gmail.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting and closing the gap!
I will raise an issue to do the same for COSE_Sign.

@setrofim setrofim merged commit 354ac99 into main May 18, 2023
@setrofim setrofim deleted the setrofim branch May 18, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants