Repository: RustCrypto/hashes (language: rust)

Issue:
blake2: MAC variants should not be constructable with an empty key?
`blake 0.10` permits construction of MAC variants with empty keys, but I don't think this is a valid construction of blake2. Using [RFC7693](https://www.rfc-editor.org/rfc/rfc7693.html) as reference:

[Section 2.5](https://www.rfc-editor.org/rfc/rfc7693.html#section-2.5) states the following:

> The second (little-endian) byte of the parameter block, "kk", specifies the key size in bytes. Set kk = 00 for unkeyed hashing.

I've to understand this as keyed Blake2 is mutually exclusive with an empty key. Yet `Blake2*Mac` variants seem to permit this construction with [`new_with_params`](https://github.com/RustCrypto/hashes/blob/59176e068a1b79058bfaa6d24a90d2f518b3c1ab/blake2/src/macros.rs#L283). This method will accept an empty key, perform operations to initialize blake2 in keyed mode (e.g. the operations in [Section 3.3](https://www.rfc-editor.org/rfc/rfc7693.html#section-3.3) that assume `kk > 0`), but then [pass a key length of 0 to `new_with_params`](https://github.com/RustCrypto/hashes/blob/59176e068a1b79058bfaa6d24a90d2f518b3c1ab/blake2/src/macros.rs#L297), which then [initializes the parameter block with `kk = 0x00`](https://github.com/RustCrypto/hashes/blob/59176e068a1b79058bfaa6d24a90d2f518b3c1ab/blake2/src/macros.rs#L43).

This results in a keyed construction of blake2 buffers with unkeyed initialization which seems incorrect?

If I'm correct, then I think we either need to special-case the empty key when provided to the MAC variants or have empty keys return an error?


Discussion / hints:

Fix patch:
diff --git a/blake2/src/macros.rs b/blake2/src/macros.rs
index 917a212c8..567e64876 100644
--- a/blake2/src/macros.rs
+++ b/blake2/src/macros.rs
@@ -276,9 +276,12 @@ macro_rules! blake2_mac_impl {
         {
             /// Create new instance using provided key, salt, and persona.
             ///
-            /// Key length should not be bigger than block size, salt and persona
-            /// length should not be bigger than quarter of block size. If any
-            /// of those conditions is false the method will return an error.
+            /// # Errors
+            ///
+            /// Key length should not be empty or bigger than the block size and
+            /// the salt and persona length should not be bigger than quarter of
+            /// block size. If any of those conditions is false the method will
+            /// return an error.
             #[inline]
             pub fn new_with_salt_and_personal(
                 key: &[u8],
@@ -288,7 +291,7 @@ macro_rules! blake2_mac_impl {
                 let kl = key.len();
                 let bs = <$hash as BlockSizeUser>::BlockSize::USIZE;
                 let qbs = bs / 4;
-                if kl > bs || salt.len() > qbs || persona.len() > qbs {
+                if kl == 0 || kl > bs || salt.len() > qbs || persona.len() > qbs {
                     return Err(InvalidLength);
                 }
                 let mut padded_key = Block::<$hash>::default();


Test patch:
diff --git a/blake2/tests/mac.rs b/blake2/tests/mac.rs
index dd75a5703..0812c90cc 100644
--- a/blake2/tests/mac.rs
+++ b/blake2/tests/mac.rs
@@ -27,3 +27,9 @@ fn blake2b_new_test() {
     run::<blake2::Blake2sMac256>(&[0x42; 32]);
     run::<blake2::Blake2bMac512>(&[0x42; 64]);
 }
+
+#[test]
+fn mac_refuses_empty_keys() {
+    assert!(blake2::Blake2bMac512::new_with_salt_and_personal(&[], b"salt", b"persona").is_err());
+    assert!(blake2::Blake2sMac256::new_with_salt_and_personal(&[], b"salt", b"persona").is_err());
+}