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

Missing VerifyPSKInputs check in the key schedule #69

Open
gendx opened this issue Oct 1, 2024 · 1 comment
Open

Missing VerifyPSKInputs check in the key schedule #69

gendx opened this issue Oct 1, 2024 · 1 comment

Comments

@gendx
Copy link

gendx commented Oct 1, 2024

According to RFC 9180 section 5.1, the first step of the KeySchedule() function is to run the validation function VerifyPSKInputs(), which checks that:

  • psk and psk_id are either both empty or both non-empty,
  • they are not empty in PSK mode (and they are empty in non-PSK mode).

These checks are emphasized with "MUST" language:

The psk and psk_id fields MUST appear together or not at all. That is, if a non-default value is provided for one of them, then the other MUST be set to a non-default value. This requirement is encoded in VerifyPSKInputs() below.

In the hpke crate, the RFC pseudo-code is pasted as comment for the derive_enc_ctx() function ; however I didn't see the VerifyPSKInputs() check implemented in the function body.

As far as I can tell, the hpke crate represents the PSK state as part of the PskBundle struct, which has public fields (https://github.com/rozbb/rust-hpke/blob/main/src/op_mode.rs). Given that these fields are public, it may be fine to let the user bear the responsibility of setting consistent (i.e. non-empty) values, but the documentation doesn't look strong enough (it mentions "psk MUST contain at least 32 bytes of entroy", but gives no requirement for psk_id).

In any case, adding a check within derive_enc_ctx() and either returning an error or panicking in case the PSK bundle is invalid would be good defense in depth.

@rozbb
Copy link
Owner

rozbb commented Nov 10, 2024

This is a great point, thank you! It might be a breaking change. I can draft this this weekend

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

No branches or pull requests

2 participants