named-data-iot / ndn-lite

A lightweight NDN protocol stack with high-level application support including security bootstrapping, access control, trust management, etc.
https://ndn-lite.named-data.net
GNU Lesser General Public License v3.0
44 stars 16 forks source link

ndn_hkdf: does not conform to RFC 5869 #65

Open yoursunny opened 5 years ago

yoursunny commented 5 years ago

5aea33b5f8ace2934d67a3f175c2c90229fdb9ec 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: https://github.com/named-data-iot/ndn-lite/blob/5aea33b5f8ace2934d67a3f175c2c90229fdb9ec/security/default-backend/ndn-lite-default-hmac-impl.c#L37-L39

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

tianyuan129 commented 5 years ago

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 commented 5 years ago

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.