crypto: Change crypto_mac_sha3_256 to use the key length in the construction

Signed-off-by: David Goulet <dgoulet@torproject.org>

src/common/crypto.c

@@ -2109,25 +2109,32 @@ crypto_hmac_sha256(char *hmac_out,
   tor_assert(rv);
 }
 
-/** Compute an SHA3 MAC of <b>msg</b> using <b>key</b> as the key. The format
- * used for our MAC is SHA3(k | m). Write the DIGEST256_LEN-byte result into
- * <b>mac_out</b> of size <b>mac_out_len</b>. */
+/** Compute a MAC using SHA3-256 of <b>msg_len</b> bytes in <b>msg</b> using a
+ * <b>key</b> of length <b>key_len</b> and a <b>salt</b> of length
+ * <b>salt_len</b>. Store the result of <b>len_out</b> bytes in in
+ * <b>mac_out</b>. This function can't fail. */
 void
-crypto_mac_sha3_256(char *mac_out, size_t mac_out_len,
-                    const char *key, size_t key_len,
-                    const char *msg, size_t msg_len)
+crypto_mac_sha3_256(uint8_t *mac_out, size_t len_out,
+                    const uint8_t *key, size_t key_len,
+                    const uint8_t *msg, size_t msg_len)
 {
   crypto_digest_t *digest;
 
+  const uint64_t key_len_netorder = tor_htonll(key_len);
+
   tor_assert(mac_out);
   tor_assert(key);
   tor_assert(msg);
 
   digest = crypto_digest256_new(DIGEST_SHA3_256);
 
-  crypto_digest_add_bytes(digest, key, key_len);
-  crypto_digest_add_bytes(digest, msg, msg_len);
-  crypto_digest_get_digest(digest, mac_out, mac_out_len);
+  /* Order matters here that is any subsystem using this function should
+   * expect this very precise ordering in the MAC construction. */
+  crypto_digest_add_bytes(digest, (const char *) &key_len_netorder,
+                          sizeof(key_len_netorder));
+  crypto_digest_add_bytes(digest, (const char *) key, key_len);
+  crypto_digest_add_bytes(digest, (const char *) msg, msg_len);
+  crypto_digest_get_digest(digest, (char *) mac_out, len_out);
   crypto_digest_free(digest);
 }
 

src/common/crypto.h

@@ -255,9 +255,10 @@ void crypto_digest_assign(crypto_digest_t *into,
 void crypto_hmac_sha256(char *hmac_out,
                         const char *key, size_t key_len,
                         const char *msg, size_t msg_len);
-void crypto_mac_sha3_256(char *mac_out, size_t mac_out_len,
-                         const char *key, size_t key_len,
-                         const char *msg, size_t msg_len);
+void crypto_mac_sha3_256(uint8_t *mac_out, size_t len_out,
+                         const uint8_t *key, size_t key_len,
+                         const uint8_t *msg, size_t msg_len);
+
 crypto_xof_t *crypto_xof_new(void);
 void crypto_xof_add_bytes(crypto_xof_t *xof, const uint8_t *data, size_t len);
 void crypto_xof_squeeze_bytes(crypto_xof_t *xof, uint8_t *out, size_t len);

src/or/hs_intropoint.c

@@ -42,7 +42,7 @@ get_auth_key_from_establish_intro_cell(ed25519_public_key_t *auth_key_out,
  *  given <b>circuit_key_material</b>. Return 0 on success else -1 on error. */
 STATIC int
 verify_establish_intro_cell(const hs_cell_establish_intro_t *cell,
-                            const char *circuit_key_material,
+                            const uint8_t *circuit_key_material,
                             size_t circuit_key_material_len)
 {
   /* We only reach this function if the first byte of the cell is 0x02 which
@@ -62,7 +62,7 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell,
     return -1;
   }
 
-  const char *msg = (char*) cell->start_cell;
+  const uint8_t *msg = cell->start_cell;
 
   /* Verify the sig */
   {
@@ -79,7 +79,7 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell,
     ed25519_public_key_t auth_key;
     get_auth_key_from_establish_intro_cell(&auth_key, cell);
 
-    const size_t sig_msg_len = (char*) (cell->end_sig_fields) - msg;
+    const size_t sig_msg_len = cell->end_sig_fields - msg;
     int sig_mismatch = ed25519_checksig_prefixed(&sig_struct,
                                                  (uint8_t*) msg, sig_msg_len,
                                                  ESTABLISH_INTRO_SIG_PREFIX,
@@ -92,11 +92,11 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell,
 
   /* Verify the MAC */
   {
-    const size_t auth_msg_len = (char*) (cell->end_mac_fields) - msg;
-    char mac[DIGEST256_LEN];
+    const size_t auth_msg_len = cell->end_mac_fields - msg;
+    uint8_t mac[DIGEST256_LEN];
     crypto_mac_sha3_256(mac, sizeof(mac),
-                         circuit_key_material, circuit_key_material_len,
-                         msg, auth_msg_len);
+                        circuit_key_material, circuit_key_material_len,
+                        msg, auth_msg_len);
     if (tor_memneq(mac, cell->handshake_mac, sizeof(mac))) {
       log_warn(LD_PROTOCOL, "ESTABLISH_INTRO handshake_auth not as expected");
       return -1;
@@ -198,8 +198,8 @@ handle_establish_intro(or_circuit_t *circ, const uint8_t *request,
   }
 
   cell_ok = verify_establish_intro_cell(parsed_cell,
-                                       circ->rend_circ_nonce,
-                                       sizeof(circ->rend_circ_nonce));
+                                        (uint8_t *) circ->rend_circ_nonce,
+                                        sizeof(circ->rend_circ_nonce));
   if (cell_ok < 0) {
     log_warn(LD_PROTOCOL, "Failed to verify ESTABLISH_INTRO cell.");
     goto err;

src/or/hs_intropoint.h

@@ -28,7 +28,7 @@ int hs_intro_circuit_is_suitable(const or_circuit_t *circ);
 
 STATIC int
 verify_establish_intro_cell(const hs_cell_establish_intro_t *out,
-                            const char *circuit_key_material,
+                            const uint8_t *circuit_key_material,
                             size_t circuit_key_material_len);
 
 STATIC void

src/or/hs_service.c

@@ -60,7 +60,7 @@ set_cell_extensions(hs_cell_establish_intro_t *cell)
  *  returned cell is allocated on the heap and it's the responsibility of the
  *  caller to free it. */
 STATIC hs_cell_establish_intro_t *
-generate_establish_intro_cell(const char *circuit_key_material,
+generate_establish_intro_cell(const uint8_t *circuit_key_material,
                               size_t circuit_key_material_len)
 {
   hs_cell_establish_intro_t *cell = NULL;
@@ -109,7 +109,7 @@ generate_establish_intro_cell(const char *circuit_key_material,
     /* To calculate HANDSHAKE_AUTH, we dump the cell in bytes, and then derive
        the MAC from it. */
     uint8_t cell_bytes_tmp[RELAY_PAYLOAD_SIZE] = {0};
-    char mac[TRUNNEL_SHA3_256_LEN];
+    uint8_t mac[TRUNNEL_SHA3_256_LEN];
 
     encoded_len = hs_cell_establish_intro_encode(cell_bytes_tmp,
                                                  sizeof(cell_bytes_tmp),
@@ -125,7 +125,7 @@ generate_establish_intro_cell(const char *circuit_key_material,
     /* Calculate MAC of all fields before HANDSHAKE_AUTH */
     crypto_mac_sha3_256(mac, sizeof(mac),
                         circuit_key_material, circuit_key_material_len,
-                        (const char*)cell_bytes_tmp,
+                        cell_bytes_tmp,
                         encoded_len - (ED25519_SIG_LEN + 2 + TRUNNEL_SHA3_256_LEN));
     /* Write the MAC to the cell */
     uint8_t *handshake_ptr =

src/or/hs_service.h

@@ -17,7 +17,7 @@
 #ifdef TOR_UNIT_TESTS
 
 STATIC hs_cell_establish_intro_t *
-generate_establish_intro_cell(const char *circuit_key_material,
+generate_establish_intro_cell(const uint8_t *circuit_key_material,
                               size_t circuit_key_material_len);
 
 STATIC ssize_t

src/test/test_crypto.c

@@ -1147,28 +1147,34 @@ test_crypto_mac_sha3(void *arg)
   const char msg[] = "i am in a library somewhere using my computer";
   const char key[] = "i'm from the past talking to the future.";
 
-  char hmac_test[DIGEST256_LEN];
+  uint8_t hmac_test[DIGEST256_LEN];
   char hmac_manual[DIGEST256_LEN];
 
   (void) arg;
 
   /* First let's use our nice HMAC-SHA3 function */
   crypto_mac_sha3_256(hmac_test, sizeof(hmac_test),
-                      key, strlen(key),
-                      msg, strlen(msg));
+                      (uint8_t *) key, strlen(key),
+                      (uint8_t *) msg, strlen(msg));
 
-  /* Now let's try a manual H(k || m) construction */
+  /* Now let's try a manual H(len(k) || k || m) construction */
   {
-    char *key_msg_concat = NULL;
+    char *key_msg_concat = NULL, *all = NULL;
     int result;
+    const uint64_t key_len_netorder = tor_htonll(strlen(key));
+    size_t all_len;
 
     tor_asprintf(&key_msg_concat, "%s%s", key, msg);
+    all_len = sizeof(key_len_netorder) + strlen(key_msg_concat);
+    all = tor_malloc_zero(all_len);
+    memcpy(all, &key_len_netorder, sizeof(key_len_netorder));
+    memcpy(all + sizeof(key_len_netorder), key_msg_concat,
+           strlen(key_msg_concat));
 
-    result = crypto_digest256(hmac_manual,
-                              key_msg_concat, strlen(key_msg_concat),
-                              DIGEST_SHA3_256);
-    tt_int_op(result, ==, 0);
+    result = crypto_digest256(hmac_manual, all, all_len, DIGEST_SHA3_256);
     tor_free(key_msg_concat);
+    tor_free(all);
+    tt_int_op(result, ==, 0);
   }
 
   /* Now compare the two results */

src/test/test_hs_intropoint.c

@@ -44,12 +44,12 @@ test_establish_intro_wrong_purpose(void *arg)
   or_circuit_t *intro_circ = or_circuit_new(0,NULL);;
   uint8_t cell_body[RELAY_PAYLOAD_SIZE];
   ssize_t cell_len = 0;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   (void)arg;
 
   /* Get the auth key of the intro point */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   memcpy(intro_circ->rend_circ_nonce, circuit_key_material, DIGEST_LEN);
 
   /* Set a bad circuit purpose!! :) */
@@ -75,7 +75,8 @@ test_establish_intro_wrong_purpose(void *arg)
 
 /* Prepare a circuit for accepting an ESTABLISH_INTRO cell */
 static void
-helper_prepare_circ_for_intro(or_circuit_t *circ, char *circuit_key_material)
+helper_prepare_circ_for_intro(or_circuit_t *circ,
+                              uint8_t *circuit_key_material)
 {
   /* Prepare the circuit for the incoming ESTABLISH_INTRO */
   circuit_change_purpose(TO_CIRCUIT(circ), CIRCUIT_PURPOSE_OR);
@@ -88,12 +89,12 @@ test_establish_intro_wrong_keytype(void *arg)
 {
   int retval;
   or_circuit_t *intro_circ = or_circuit_new(0,NULL);;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   (void)arg;
 
   /* Get the auth key of the intro point */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   helper_prepare_circ_for_intro(intro_circ, circuit_key_material);
 
   /* Receive the cell. Should fail. */
@@ -113,12 +114,12 @@ test_establish_intro_wrong_keytype2(void *arg)
   or_circuit_t *intro_circ = or_circuit_new(0,NULL);;
   uint8_t cell_body[RELAY_PAYLOAD_SIZE];
   ssize_t cell_len = 0;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   (void)arg;
 
   /* Get the auth key of the intro point */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   helper_prepare_circ_for_intro(intro_circ, circuit_key_material);
 
   /* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we
@@ -152,12 +153,12 @@ test_establish_intro_wrong_sig(void *arg)
   or_circuit_t *intro_circ = or_circuit_new(0,NULL);;
   uint8_t cell_body[RELAY_PAYLOAD_SIZE];
   ssize_t cell_len = 0;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   (void)arg;
 
   /* Get the auth key of the intro point */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   helper_prepare_circ_for_intro(intro_circ, circuit_key_material);
 
   /* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we
@@ -190,12 +191,12 @@ helper_establish_intro_v3(or_circuit_t *intro_circ)
   hs_cell_establish_intro_t *establish_intro_cell = NULL;
   uint8_t cell_body[RELAY_PAYLOAD_SIZE];
   ssize_t cell_len = 0;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   tt_assert(intro_circ);
 
   /* Prepare the circuit for the incoming ESTABLISH_INTRO */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   helper_prepare_circ_for_intro(intro_circ, circuit_key_material);
 
   /* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we
@@ -224,12 +225,12 @@ helper_establish_intro_v2(or_circuit_t *intro_circ)
   int retval;
   uint8_t cell_body[RELAY_PAYLOAD_SIZE];
   ssize_t cell_len = 0;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   tt_assert(intro_circ);
 
   /* Prepare the circuit for the incoming ESTABLISH_INTRO */
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
   helper_prepare_circ_for_intro(intro_circ, circuit_key_material);
 
   /* Send legacy establish_intro */
@@ -238,7 +239,7 @@ helper_establish_intro_v2(or_circuit_t *intro_circ)
   /* Use old circuit_key_material why not */
   cell_len = encode_establish_intro_cell_legacy((char*)cell_body,
                                                 key1,
-                                                circuit_key_material);
+                                                (char *) circuit_key_material);
   tt_int_op(cell_len, >, 0);
 
   /* Receive legacy establish_intro */

src/test/test_hs_service.c

@@ -24,12 +24,12 @@ test_gen_establish_intro_cell(void *arg)
 {
   (void) arg;
   int retval;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
   uint8_t buf[RELAY_PAYLOAD_SIZE];
   hs_cell_establish_intro_t *cell_out = NULL;
   hs_cell_establish_intro_t *cell_in = NULL;
 
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
 
   /* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we
      attempt to parse it. */
@@ -79,11 +79,11 @@ test_gen_establish_intro_cell_bad(void *arg)
 {
   (void) arg;
   hs_cell_establish_intro_t *cell = NULL;
-  char circuit_key_material[DIGEST_LEN] = {0};
+  uint8_t circuit_key_material[DIGEST_LEN] = {0};
 
   MOCK(ed25519_sign_prefixed, mock_ed25519_sign_prefixed);
 
-  crypto_rand(circuit_key_material, sizeof(circuit_key_material));
+  crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));
 
   setup_full_capture_of_logs(LOG_WARN);
   /* Easiest way to make that function fail is to mock the