[tor-commits] [tor/release-0.3.0] hs: Correctly validate v3 descriptor encrypted length

[nickm at torproject.org]

commit 5b33d95a3dfe943625d78983bb53be2901a51150
Author: David Goulet <dgoulet at torproject.org>
Date:   Tue May 30 10:27:42 2017 -0400

    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 at torproject.org>
---
 changes/bug22447              |  3 +++
 src/or/hs_descriptor.c        | 23 ++++-------------------
 src/test/test_hs_descriptor.c |  5 ++---
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/changes/bug22447 b/changes/bug22447
new file mode 100644
index 0000000..f5649d6
--- /dev/null
+++ b/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.
diff --git a/src/or/hs_descriptor.c b/src/or/hs_descriptor.c
index f16a2fd..938b7a7 100644
--- a/src/or/hs_descriptor.c
+++ b/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;
diff --git a/src/test/test_hs_descriptor.c b/src/test/test_hs_descriptor.c
index 02a71aa..97fe191 100644
--- a/src/test/test_hs_descriptor.c
+++ b/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. */