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

Ensure IV and Partial IV are not both present #66

Merged
merged 4 commits into from
May 20, 2022

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented May 9, 2022

The NCC Group found this issue:

Partial IV: The ‘Initialization Vector’ and ‘Partial Initialization Vector’ parameters
MUST NOT both be present in the same security layer. (Section 3.1)

The go-cose library does not provide any explicit support for these parameters. A user is
free to set them within a message, but go-cose will not prevent usage that contradicts the
above requirement.

To meet this requirement, we should ensure that the IV and Partial IV are not both present in the protected header when marshaling and unmarshaling it. This PR does that.

@thomas-fossati
Copy link
Contributor

I guess I could have raised it myself -- see https://www.rfc-editor.org/errata/eid6909 :-)

I will add a GlueCOSE test vector for this.

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

headers.go Outdated Show resolved Hide resolved
@shizhMSFT
Copy link
Contributor

It is weird to check IVs as we never use them but we should do it since it is go-cose.

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 a general question here.

In RFC 8152 3.1, it states

The IV can be placed in the unprotected header as
modifying the IV will cause the decryption to yield plaintext that
is readily detectable as garbled.

and also

The 'Initialization Vector' and 'Partial
Initialization Vector' parameters MUST NOT both be present in the
same security layer.

What does the "security layer" refer to?

To me, it refers to the "protected" layer and "unprotected" layer, which means we also need to check the unprotected header.

headers.go Outdated Show resolved Hide resolved
@qmuntal
Copy link
Contributor Author

qmuntal commented May 18, 2022

I have a general question here.

In RFC 8152 3.1, it states

The IV can be placed in the unprotected header as
modifying the IV will cause the decryption to yield plaintext that
is readily detectable as garbled.

and also

The 'Initialization Vector' and 'Partial
Initialization Vector' parameters MUST NOT both be present in the
same security layer.

What does the "security layer" refer to?

To me, it refers to the "protected" layer and "unprotected" layer, which means we also need to check the unprotected header.

"security layer" is not defined in the spec, but what I understood is that every layer is a security layer, but the spec is just using a fancy adjective.

And yes, we also have to check the unprotected header, as it is part of the same layer the protected header is in.

@thomas-fossati
Copy link
Contributor

To me, it refers to the "protected" layer and "unprotected" layer, which means we also need to check the unprotected header.

In principle one can nest COSE messages. See https://www.rfc-editor.org/authors/rfc9052.html#appendix-B for an example.

Sibling header buckets (protected and unprotected) -- i.e., found in the same COSE message -- are considered to be at the same layer.

@qmuntal qmuntal requested a review from shizhMSFT May 18, 2022 21:51
@shizhMSFT
Copy link
Contributor

In principle one can nest COSE messages. See https://www.rfc-editor.org/authors/rfc9052.html#appendix-B for an example.

Thanks @thomas-fossati for pointing me to the right place. From the above appendix, a security layer means one COSE structure. In our case, that's COSE_Sign / COSE_Sign1 / COSE_Signature.

Therefore, the IV and Partial IV cannot exists in the same header set. For example, it's not allowed if there is an IV in the protected header and Partial IV in the unprotected header in a COSE_Sign1 message since they are in the same security layer. However, it is allowed to have IV in the header of COSE_Sign and Partial IV in the header of one of its COSE_Signature since they are different security layers.

@qmuntal
Copy link
Contributor Author

qmuntal commented May 19, 2022

Therefore, the IV and Partial IV cannot exists in the same header set. For example, it's not allowed if there is an IV in the protected header and Partial IV in the unprotected header in a COSE_Sign1 message since they are in the same security layer. However, it is allowed to have IV in the header of COSE_Sign and Partial IV in the header of one of its COSE_Signature since they are different security layers.

@shizhMSFT I've implemented this check, please take another look.

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

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
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.

5 participants