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

HKDF-Extract with empty salt #45

Closed
LuoZijun opened this issue Dec 8, 2020 · 2 comments · Fixed by #46
Closed

HKDF-Extract with empty salt #45

LuoZijun opened this issue Dec 8, 2020 · 2 comments · Fixed by #46

Comments

@LuoZijun
Copy link
Contributor

LuoZijun commented Dec 8, 2020

Follow HKDF RFC: https://tools.ietf.org/html/rfc5869#section-2.2

HashLen denotes the length of the hash function output in octets

optional salt value (a non-secret random value); if not provided, it is set to a string of HashLen zeros.

The HashLen is OutputLen, not BlockLen.

But for now, the hkdf code set to a string of BlockLen zeros when salt value not provided.

See code:

KDFs/hkdf/src/hkdf.rs

Lines 71 to 75 in e413e0a

pub fn new(salt: Option<&[u8]>) -> HkdfExtract<D> {
let hmac = match salt {
Some(s) => Hmac::<D>::new_varkey(s).expect("HMAC can take a key of any size"),
None => Hmac::<D>::new(&Default::default()),
};

HMAC Code Line:
https://github.com/RustCrypto/MACs/blob/4ab9f441fb08c754c22d65963fa948693c6e5116/hmac/src/lib.rs#L118-L128

impl<D> NewMac for Hmac<D>
where
    D: Update + BlockInput + FixedOutput + Reset + Default + Clone,
    D::BlockSize: ArrayLength<u8>,
    D::OutputSize: ArrayLength<u8>,
{
    type KeySize = D::BlockSize;

    fn new(key: &GenericArray<u8, Self::KeySize>) -> Self {
        Self::new_varkey(key.as_slice()).unwrap()
    }

Fix:

None => Hmac::<D>::new(&Default::default()), should be change to None => Hmac::<D>::new_varkey(&D::OutputSize::default()), .

@LuoZijun LuoZijun changed the title asdasd HKDF-Extract with empty salt Dec 8, 2020
@tarcieri
Copy link
Member

tarcieri commented Dec 8, 2020

Your reading appears to be correct and this does indeed look like a bug. Would you like to open a PR to fix it?

@LuoZijun
Copy link
Contributor Author

LuoZijun commented Dec 8, 2020

@tarcieri See PR #46

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 a pull request may close this issue.

2 participants