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 ML-KEM decapsulation key check. #507

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Add ML-KEM decapsulation key check. #507

merged 1 commit into from
Aug 19, 2024

Conversation

bwesterb
Copy link
Member

Described in section 7.3 of FIPS 203.

The check is only required if the private key is from an untrusted source. We do not distinguish between a trusted and untrusted source in the current API, so we'll perform the check every time we unmarshal the private key.

Described in section 7.3 of FIPS 203.

The check is only required if the private key is from an untrusted
source. We do not distinguish between a trusted and untrusted source
in the current API, so we'll perform the check every time we unmarshal
the private key.
Copy link
Contributor

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

Checks looks reasonable, but I have an open question.

buf = buf[cpapke.PublicKeySize:]
copy(sk.hpk[:], buf[:32])
copy(sk.z[:], buf[32:])
{{ if .NIST -}}
if !bytes.Equal(hpk[:], sk.hpk[:]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I manually compared these steps with the one from section 7.3 and 8 of https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.203.pdf, it matches 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

// Returns an error if buf is not of size PrivateKeySize, or private key
// doesn't pass the ML-KEM decapsulation key check.
func (sk *PrivateKey) Unpack(buf []byte) error {
if len(buf) != PrivateKeySize {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this addition, the extra check in UnmarshalBinaryPrivateKey for the same condition becomes redundant. Can we simplify the implementation template by unconditionally including this check and returning error for this function?

Are there users of Unpack other than UnmarshalBinaryPrivateKey? If so, should we try harder to prevent intermediate values in sk.sk, sk.pk, sk.hpk and sk.z when the error check fails? (E.g. by writing these only at the end, or clearing values on error?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to change the existing Kyber API.

@armfazh armfazh merged commit 62385a8 into main Aug 19, 2024
10 checks passed
@armfazh armfazh deleted the bas/mlkem-dkcheck branch August 19, 2024 21:23
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.

3 participants