Browse Source

hs: Correctly validate v3 descriptor encrypted length

The encrypted_data_length_is_valid() function wasn't validating correctly the
length of the encrypted data of a v3 descriptor. The side effect of this is
that an HSDir was rejecting the descriptor and ultimately not storing it.

Fixes #22447

Signed-off-by: David Goulet <dgoulet@torproject.org>
David Goulet 7 years ago
parent
commit
5b33d95a3d
3 changed files with 9 additions and 22 deletions
  1. 3 0
      changes/bug22447
  2. 4 19
      src/or/hs_descriptor.c
  3. 2 3
      src/test/test_hs_descriptor.c

+ 3 - 0
changes/bug22447

@@ -0,0 +1,3 @@
+  o Major bugfixes (hidden service v3):
+    - HSDir failed to validate the encrypted size of a v3 descriptor and thus
+      rejecting it. Fixes bug 22447; bugfix on tor-0.3.0.1-alpha.

+ 4 - 19
src/or/hs_descriptor.c

@@ -1023,30 +1023,15 @@ cert_parse_and_validate(tor_cert_t **cert_out, const char *data,
 STATIC int
 encrypted_data_length_is_valid(size_t len)
 {
-  /* Check for the minimum length possible. */
-  if (len < HS_DESC_ENCRYPTED_MIN_LEN) {
+  /* Make sure there is enough data for the salt and the mac. The equality is
+   * there to ensure that there is at least one byte of encrypted data. */
+  if (len <= HS_DESC_ENCRYPTED_SALT_LEN + DIGEST256_LEN) {
     log_warn(LD_REND, "Length of descriptor's encrypted data is too small. "
                       "Got %lu but minimum value is %d",
-             (unsigned long)len, HS_DESC_ENCRYPTED_MIN_LEN);
+             (unsigned long)len, HS_DESC_ENCRYPTED_SALT_LEN + DIGEST256_LEN);
     goto err;
   }
 
-  /* Encrypted data has the salt and MAC concatenated to it so remove those
-   * from the validation calculation. */
-  len -= HS_DESC_ENCRYPTED_SALT_LEN + DIGEST256_LEN;
-
-  /* Check that it's aligned on the block size of the crypto algorithm. */
-  if (len % HS_DESC_PLAINTEXT_PADDING_MULTIPLE) {
-    log_warn(LD_REND, "Length of descriptor's encrypted data is invalid. "
-                      "Got %lu which is not a multiple of %d.",
-             (unsigned long) len, HS_DESC_PLAINTEXT_PADDING_MULTIPLE);
-    goto err;
-  }
-
-  /* XXX: Check maximum size. Will strongly depends on the maximum intro point
-   * allowed we decide on and probably if they will all have to use the legacy
-   * key which is bigger than the ed25519 key. */
-
   return 1;
  err:
   return 0;

+ 2 - 3
src/test/test_hs_descriptor.c

@@ -587,9 +587,8 @@ test_encrypted_data_len(void *arg)
   /* No length, error. */
   ret = encrypted_data_length_is_valid(0);
   tt_int_op(ret, OP_EQ, 0);
-  /* Not a multiple of our encryption algorithm (thus no padding). It's
-   * suppose to be aligned on HS_DESC_PLAINTEXT_PADDING_MULTIPLE. */
-  value = HS_DESC_PLAINTEXT_PADDING_MULTIPLE * 10 - 1;
+  /* This value is missing data. */
+  value = HS_DESC_ENCRYPTED_SALT_LEN + DIGEST256_LEN;
   ret = encrypted_data_length_is_valid(value);
   tt_int_op(ret, OP_EQ, 0);
   /* Valid value. */