bug: fail to verify COSE signature with extra attributes in the protected header

[shizhMSFT]

When the COSE signature contains extra attribute keys, which are integers not strings, the code reports error:

extendedAttributes key requires string type

Those keys should be ignored if not marked as critical, or report error if marked as critical. If they are known attributes, we may convert keys into strings so that it can fill in the signature.Attribute structure.

Related code:

notation-core-go/signature/cose/envelope.go

Lines 531 to 544 in b5df54c

	// fetch all the extended signed attributes
	systemHeaders := []interface{}{cose.HeaderLabelAlgorithm, cose.HeaderLabelCritical, cose.HeaderLabelContentType,
	headerLabelExpiry, headerLabelSigningScheme, headerLabelSigningTime, headerLabelAuthenticSigningTime}
	var extendedAttributeKeys []string
	for label := range protected {
	if contains(systemHeaders, label) {
	continue
	}
	label, ok := label.(string)
	if !ok {
	return nil, &signature.InvalidSignatureError{Msg: "extendedAttributes key requires string type"}
	}
	extendedAttributeKeys = append(extendedAttributeKeys, label)
	}

[patrickzheng200]

I will take this one.

[priteshbandi]

Those keys should be ignored if not marked as critical, or report error if marked as critical.

Can this cause issues if a plugin is expecting those attributes? Also, silently ignoring attributes doesn't look right. Do you have a specific usecase?

If they are known attributes, we may convert keys into strings so that it can fill in the signature.Attribute structure.

What will we do in case of a collision? IMO unless we have a specific usecase to support, we should continue to fail verification if we encounter non-string keys in signed attributes.

[patrickzheng200]

Those keys should be ignored if not marked as critical, or report error if marked as critical.

Can this cause issues if a plugin is expecting those attributes? Also, silently ignoring attributes doesn't look right. Do you have a specific usecase?

If they are known attributes, we may convert keys into strings so that it can fill in the signature.Attribute structure.

What will we do in case of a collision? IMO unless we have a specific usecase to support, we should continue to fail verification if we encounter non-string keys in signed attributes.

Please correct me if I'm wrong, according to our implementation in notation-go executePlugin, we are only passing critical extended attributes to plugin.VerifySignatureRequest. In other words, if a plugin expects a certain attribute, it has to be marked as critical anyway.

For COSE envelope, there could be non-string keys, for example, key of kid is 4. In this case, if it is non-critical, we can ignore it instead of failing the verification (one of our users is blocked due to this logic). If it's marked critical, we return an error requiring string type. If a plugin expects this extended attribute, then it needs to mark the attribute as critical for now.
Finally, there shouldn't be collision issue for a COSE envelope, since ProtectedHeader is a Map by design.
@shizhMSFT @priteshbandi

[priteshbandi]

Those keys should be ignored if not marked as critical, or report error if marked as critical.

Can this cause issues if a plugin is expecting those attributes? Also, silently ignoring attributes doesn't look right. Do you have a specific usecase?

If they are known attributes, we may convert keys into strings so that it can fill in the signature.Attribute structure.

What will we do in case of a collision? IMO unless we have a specific usecase to support, we should continue to fail verification if we encounter non-string keys in signed attributes.

Please correct me if I'm wrong, according to our implementation in notation-go executePlugin, we are only passing critical extended attributes to plugin.VerifySignatureRequest. In other words, if a plugin expects a certain attribute, it has to be marked as critical anyway.

For COSE envelope, there could be non-string keys, for example, key of kid is 4. In this case, if it is non-critical, we can ignore it instead of failing the verification (one of our users is blocked due to this logic). If it's marked critical, we return an error requiring string type. If a plugin expects this extended attribute, then it needs to mark the attribute as critical for now. Finally, there shouldn't be collision issue for a COSE envelope, since ProtectedHeader is a Map by design. @shizhMSFT @priteshbandi

Yes, currently we only pass critical attributes to plugin but we don't want to limit it for future usecase.

In cose both 4 as int and 4 as string can co-exist in ProtectedHeader map. If both are marked critical which one will we prefer and why?
Signature's generated using notation wont have signed integer keys in ProtectedHeader. Do you have a specific usecase that you want to support with ignoring integer values?

[patrickzheng200]

Those keys should be ignored if not marked as critical, or report error if marked as critical.

Can this cause issues if a plugin is expecting those attributes? Also, silently ignoring attributes doesn't look right. Do you have a specific usecase?

If they are known attributes, we may convert keys into strings so that it can fill in the signature.Attribute structure.

What will we do in case of a collision? IMO unless we have a specific usecase to support, we should continue to fail verification if we encounter non-string keys in signed attributes.

Please correct me if I'm wrong, according to our implementation in notation-go executePlugin, we are only passing critical extended attributes to plugin.VerifySignatureRequest. In other words, if a plugin expects a certain attribute, it has to be marked as critical anyway.
For COSE envelope, there could be non-string keys, for example, key of kid is 4. In this case, if it is non-critical, we can ignore it instead of failing the verification (one of our users is blocked due to this logic). If it's marked critical, we return an error requiring string type. If a plugin expects this extended attribute, then it needs to mark the attribute as critical for now. Finally, there shouldn't be collision issue for a COSE envelope, since ProtectedHeader is a Map by design. @shizhMSFT @priteshbandi

Yes, currently we only pass critical attributes to plugin but we don't want to limit it for future usecase.

In cose both 4 as int and 4 as string can co-exist in ProtectedHeader map. If both are marked critical which one will we prefer and why? Signature's generated using notation wont have signed integer keys in ProtectedHeader. Do you have a specific usecase that you want to support with ignoring integer values?

So we have 2 scenarios here:

1. Critical extended attributes require keys of string type. -> In case of 4 as int and marked as critical mentioned in your comment, we would fail the verification. And therefore, this kind of collision will not happen.
2. Non-critical extended attributes with non-string type keys will be ignored. -> This is solving one of our use cases, where it is failed because it has x5t (34 as int) in COSE's extended attribute. (This Signature is not generated using Notation.) And we don't want to fail the verification directly in this scenario.

For Notation, my understanding is that Sign and Verification are two separated operations. One can generate his own COSE Envelope somewhere else and use Notation to Verify it. Hence, I think we should accept a more general form of Signature Envelope like the one I mentioned above.

[patrickzheng200]

@rgnote @priteshbandi Any further comments/concerns on this one?

[rgnote]

Non-critical extended attributes with non-string type keys will be ignored. -> This is solving one of our use cases, where it is failed because it has x5t (34 as int) in COSE's extended attribute. (This Signature is not generated using Notation.) And we don't want to fail the verification directly in this scenario.

Ignoring keys silently doesn't seem the right approach here. It is a one-way door decision where notation will never be able to enforce string keys for non-critical extended attributes in the future. I agree that users should be able to verify signatures that are generated outside notation, but that doesn't mean notation allows verification of signatures that don't follow the notation guidelines. We should keep signatures restricted to what types notation is expecting today so that we can loosen these restrictions when we see new usecases (as opposed to the other way around)

That being said, can you explain your usecase where x5t (34 as int) can't be marked as string?

[patrickzheng200]

That being said, can you explain your usecase where x5t (34 as int) can't be marked as string?

It's because the COSE Header Parameters regulates the label type of a COSE header, from which it can be string or integer. In other words, if we claim to support COSE format, then we need to follow the COSE standard (in standard, x5t has label 34 as int).
After a discussion with @shizhMSFT, we propose another option here to avoid ignoring keys silently. We can change the key type of Attribute from string to interface{}. Note, JWS format will still enforce extended attribute keys as string type. This change will NOT touch JWS.
In case of the above-mentioned concern regarding collision when 4 as int and 4 as string both exist, we don't think it's a collision. Because we never do type conversion on keys, i.e. 4 as int and 4 as string are just two independent keys.
For more details, please take a look at PR #89. @rgnote @priteshbandi

[rgnote]

LGTM

[rgnote]

Reopening as this is causing an issue with verification plugins.

See https://github.com/notaryproject/notaryproject/blob/main/specs/plugin-extensibility.md#verify-signature-command

As part of criticalAttributes, the Verifier needs to send extended critical attributes to the verification plugin. That means, we will need to covert types of attributes to string (encoding/json doesn't support interface{} keys). That also means, attribute collisions (4 as int and 4 as string) are possible.

@shizhMSFT @patrickzheng200

[shizhMSFT]

@rgnote In the spec you've pointed, unprocessedAttributes is an array of names of critical attributes that plugin. Since it is an array not a map, it can accept any type of keys although the spec is not well defined.

[rgnote]

@shizhMSFT The issue is around extendedAttributes not unprocessedAttributes

[shizhMSFT]

@rgnote I propose that extendedAttributes should be an array instead of a map. Again, as discussed in the meeting, non-critical attributes (i.e. optional attributes) are not passed to the plugin if unprocessedAttributes contains only names of the attributes.

[rgnote]

I propose that extendedAttributes should be an array instead of a map.

Can you clarify how we will pass the attribute values if it is a array?

Again, as discussed in the meeting, non-critical attributes (i.e. optional attributes) are not passed to the plugin if unprocessedAttributes contains only names of the attributes.

non-critical attributes are not passed at all to the plugins. Please take a look at this code. extendedAttributes and unprocessedAttributes are tightly related. extendedAttributes contains the key-value pairs and the same keys will be present in unprocessedAttributes

[rgnote]

I raised this PR to unblock JWS signatures. Please raise a PR for spec updates once you have an idea on how verification plugins needs to deal with COSE extended attributes

[shizhMSFT]

Closed by notaryproject/notation-go#188 and will follow up notaryproject/specifications#201