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

ndn_hkdf: does not conform to RFC 5869 #65

Open
yoursunny opened this issue Sep 10, 2019 · 5 comments
Open

ndn_hkdf: does not conform to RFC 5869 #65

yoursunny opened this issue Sep 10, 2019 · 5 comments

Comments

@yoursunny
Copy link
Contributor

5aea33b attempts to fix #56, but it is not a valid fix. It seems that I cannot reopen that issue, so I'm opening a new issue.

RFC5869 does not limit length of IKM. Thus, the implementation should support arbitrary key length.
The underlying TinyCrypt library does not have any limitation on HMAC-SHA256 key length. The limitation comes from a flawed design here:

memset(hmac_key->key_value, 0, NDN_SEC_HMAC_MAX_KEY_SIZE);
memcpy(hmac_key->key_value, key_value, key_size);
hmac_key->key_size = key_size;

To avoid this problem:

  1. Store tc_hmac_state_struct as part of abstract_hmac_key.
  2. In ndn_lite_default_hmac_load_key, load the key with tc_hmac_set_key, and don't copy the key.
  3. In ndn_lite_default_hmac_sha256_init, memcpy the tc_hmac_state_struct from abstract_hmac_key to abstract_hmac_sha256_state_t.

Also, #54 was accidentally reverted and brought back some warnings.

@tianyuan129
Copy link
Member

tianyuan129 commented Sep 11, 2019

@Zhiyi-Zhang How do you see this HMAC change?

Personally I agree with "don't copy the key". But in default AES and ECC, they do copy keys as well. If we change them all, numbers of existing tests and examples would require modification.

I wonder is there a balance here: when key length below NDN_SEC_HMAC_MAX_KEY_SIZE, do normal key copy, and when above it, load the key with tc_hmac_set_key. No modification to existing tests and examples would be required.

@yoursunny
Copy link
Contributor Author

numbers of existing tests and examples would require modification

No. tc_hmac_state_struct should have all the information from the key. It does not require the caller to retain memory of the key itself.

when key length below NDN_SEC_HMAC_MAX_KEY_SIZE, do normal key copy, and when above it, load the key with tc_hmac_set_key.

This would be inconsistent behavior and very surprising to users.

@Zhiyi-Zhang
Copy link
Member

I agree with Junxiao's comments. Let's follow his suggestion on this issue.

@tianyuan129
Copy link
Member

Clear, while still a little question: should a method be reserved for users retrieving the key bytes from the ndn_hmac_key? This job is currently done by copying keys in ndn_lite_hmac_load_key.

Also, #54 was accidentally reverted and brought back some warnings.

It was a mistake for my carelessness, sorry for that. I've already changed it back.

@yoursunny
Copy link
Contributor Author

retrieving the key bytes from the ndn_hmac_key

No. In general, secret keys (HMAC, EC, etc) are not extractable. It may as well reside in a hardware TPM (e.g. ECC508 chip).
Caller can see secret key only when the key pair is being generated. Caller can also import secret key into a key instance. From a key instance, there's no way to get the secret key back.

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

3 participants