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

Update description of aad and info. #54

Closed
wants to merge 10 commits into from

Conversation

dajiaji
Copy link
Contributor

@dajiaji dajiaji commented Feb 25, 2024

@hannestschofenig @OR13 @laurencelundblade @ilari

Since I believe it will be easier to discuss based on a concrete proposal, I have updated the descriptions of COSE_KDF_Context, aad and info.

Could you please review it?

Closes #25 #53

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

JOSE has text about the related headers that says they MUST be understood, does COSE not have that text?

We might consider, listing the headers, and noting that while they are not used in the context, they can be relied on based on their being in the protected header, and the header being in aad.

draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
@OR13
Copy link
Contributor

OR13 commented Feb 25, 2024

Consider, reviewing https://datatracker.ietf.org/doc/html/draft-madden-jose-ecdh-1pu-04 for context on how these the headers have been used in past.

draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
specification MAY populate the fields of the COSE_KDF_Context structure or MAY use
a different structure as input to the "info" parameter. If no content for the
"info" parameter is not supplied, it defaults to a zero-length byte string.
Specifically, the KEM and KDF steps in HPKE supply sufficient context information
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct.

The public keys are, of course, included in the computation. This is not what the context is about. The context is about the endpoint identifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it's not accurate. I think your point is valid.

Instead of claiming that information equivalent to COSE_KDF_Context is provided, it might be more appropriate to correctly refer SP800-53A and demonstrate that we have gained resistance to unknown key-share attacks, etc.

{{RFC9053}} as a foundation for the info structure. This payload becomes the content
of the info parameter for the HPKE functions, when utilized. For better readability of
this specification the COSE_KDF_Context structure is repeated in {{cddl-cose-kdf}}.
Therefore, there is no need to use the COSE_KDF_Context structure defined in {{RFC9053}}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree with this conclusion. If this is the case then why are there attacks like this https://datatracker.ietf.org/doc/rfc8844/ ?

I understand that it is annoying to provide this context information but unless we can proof that there are no problems we should be following the advice by cryptographers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like you to read the following message I posted previously to COSE WG mailinglist.

https://mailarchive.ietf.org/arch/msg/cose/XjOlXbTvMRqxhdXhovv5K3fYNnw/

First of all, COSE_KDF_Context is designed to be compliant with NIST SP800
56A#Section 5.8.2.1, but it's for One-step Key Derivation.

On the other hand, HPKE adopts Two-step Key Derivation internally (See
RFC9180#KeySchedule(), LabeledExtract(), LabeledExpand()
definitions), so we should refer not Section 5.8.2.1 but Section
5.8.2.2, Two-step Key
Derivation (Extraction-then-Expansion)
.
...

Copy link
Contributor

@laurencelundblade laurencelundblade left a comment

Choose a reason for hiding this comment

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

Would like to see us go the way of the discussion of the mailing list with no COSE_KDF_Context and a new structure for the aad input to Seal()

the external_aad, while the "info" should be specified with an empty string.

In both "HPKE direct encryption" and "HPKE key encryption", the aad provided to the Seal and Open operations MUST be the protected headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hoping aad input becomes a newly defined Recipient_Structure as discussed on mailing list.

draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

The language around existing header parameters for COSE KDF context could probably be improved further.

The document needs to acknowledge these parameters can be present, and that they are ignored... But that because external AAD could be used consistently by sender and receiver and might make use of them, it's possible that they could be used if they are understood, but they are not required to be understood.

using this AAD as "aad" for HPKE is more compatible and natural with the COSE convention.


In both "HPKE direct encryption" and "HPKE key encryption", the aad provided to the Seal and Open operations MUST be the protected headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In both "HPKE direct encryption" and "HPKE key encryption", the aad provided to the Seal and Open operations MUST be the protected headers.
In both "HPKE direct encryption" and "HPKE key encryption", the aad provided to the Seal and Open operations MUST be the HPKE_Recipient_Enc_Structure.
HPKE_Recipient_Enc_Structure = CBOR ([
"HPKE_1_layer", layer1ProtectedHeaderBytes,
external_hpke_aad || empty buffer
])

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this should align with COSE and be an Enc_Structure at the layer 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably true.

draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
This mode MUST NOT be used with COSE_Encrypt0 (tag 16).
This mode MUST be used with COSE_Encrypt (tag 96).

The `aad` provided to HPKE Seal and HPKE Open MUST be the protected bytes of the COSE_Recipient, as defined in {{Section 5.1 of RFC9052 }}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `aad` provided to HPKE Seal and HPKE Open MUST be the protected bytes of the COSE_Recipient, as defined in {{Section 5.1 of RFC9052 }}.
The `aad` provided to HPKE Seal and HPKE Open MUST be the HPKE_Recipient_Enc_Structure.
HPKE_Recipient_Enc_Structure = CBOR ([
"HPKE_2_layer", layer1ProtectedHeaderBytes, layer2RecipientProtectedHeaderBytes,
external_hpke_aad || empty buffer
])

Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly favor just using the algorithm ID from layer1, rather than all the headers. If it's all the headers, you have to pass arbitrarily large amounts of data between layers, rather than just an algorithm ID. (Specifically, passing the algorithm ID was easy in t_cose. Passing all the headers would require restructuring and forcing a size limit on headers that is not there now).

I would like to not make this specific to HPKE. I think it should be used for the COSE -29 replacement too. So name it differently and use a different context string.

This is for a COSE_Recipient, so there's no external data as there is in layer 0 or in an Enc_Structure.

I think there should be optional private context data to replace the private data in COSE_KDF_Context that we are eliminating. It should be a bstr, empty if there is none.

Copy link
Contributor

@OR13 OR13 Mar 17, 2024

Choose a reason for hiding this comment

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

I don't understand how the recipient protected header is secured in the case that algorithm is the only parameter passed from recipient protected header... And layer 1 protected header is already variable length.

Treating both protected headers as bytes, seems simplest and safest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ilari did make a comment that protected headers don't always have alg.

That is a good argument to include alg, but I would still want to make sure that headers were committed too.

I think a spanning set of cases and a new enc structure would help land this.

Copy link
Contributor Author

@dajiaji dajiaji Mar 17, 2024

Choose a reason for hiding this comment

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

Hmm, I agree that a simple alternative to COSE_KDF_Context is necessary as a measure against the lamps attack, but in COSE-HPKE, wouldn't it be better to prohibit the use of non-AEAD algorithms for key wrapping (such as -29)?

I believe that a countermeasure against the lamps attack should be addressed in a separate draft, mainly targeting legacy key derivation algorithms (such as -25) before HPKE.

draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
draft-ietf-cose-hpke.md Outdated Show resolved Hide resolved
@dajiaji
Copy link
Contributor Author

dajiaji commented Apr 3, 2024

@OR13 @hannestschofenig the updates around COSE_KDF_Conxt will be done by #58, so could you merge this PR?

@OR13 OR13 self-requested a review April 3, 2024 23:43
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

I would suggest we merge #58, close this PR and open a follow up PR to address remaining issues.

It's not clear to me that the discussion on this PR was resolved and I believe it's easier to simply merge 58 first and then raise a new PR to cover anything in this PR we need to.

@dajiaji
Copy link
Contributor Author

dajiaji commented Apr 3, 2024

I would suggest we merge #58, close this PR and open a follow up PR to address remaining issues.

Okay. I agree it's a simpler way. After merging #58, then I'll make a new PR to address the remaining issues.

@dajiaji
Copy link
Contributor Author

dajiaji commented Apr 3, 2024

Following Orie's suggestion, I close the PR.

@dajiaji dajiaji closed this Apr 3, 2024
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.

Empty String for Info Value
4 participants