[tor-commits] [tor/release-0.3.0] hs: Fix bad use of sizeof() when encoding ESTABLISH_INTRO legacy cell

[nickm at torproject.org]

commit 4ed10e5053ebef31d5f922933f7236a6ab743bf9
Author: David Goulet <dgoulet at torproject.org>
Date:   Fri Feb 24 09:48:14 2017 -0500

    hs: Fix bad use of sizeof() when encoding ESTABLISH_INTRO legacy cell
    
    When encoding a legacy ESTABLISH_INTRO cell, we were using the sizeof() on a
    pointer instead of using the real size of the destination buffer leading to an
    overflow passing an enormous value to the signing digest function.
    Fortunately, that value was only used to make sure the destination buffer
    length was big enough for the key size and in this case it always was because
    of the overflow.
    
    Fixes #21553
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/bug21553              |  7 +++++++
 src/or/rendservice.c          | 12 +++++++-----
 src/or/rendservice.h          |  1 +
 src/test/test_hs_intropoint.c |  1 +
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/changes/bug21553 b/changes/bug21553
new file mode 100644
index 0000000..6ffa3e2
--- /dev/null
+++ b/changes/bug21553
@@ -0,0 +1,7 @@
+  o Minor bugfixes (hidden service):
+    - When encoding a legacy ESTABLISH_INTRO cell, we were using the sizeof()
+      on a pointer instead of real size of the destination buffer leading to
+      an overflow passing an enormous value to the signing digest function.
+      Fortunately, that value was only used to make sure the destination
+      buffer length was big enough for the key size and in this case it was.
+      Fixes bug 21553; bugfix on tor-0.3.0.1-alpha.
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 1d6fc0f..522f33e 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -3174,8 +3174,9 @@ count_intro_point_circuits(const rend_service_t *service)
    of bytes written. On fail, return -1.
  */
 STATIC ssize_t
-encode_establish_intro_cell_legacy(char *cell_body_out, crypto_pk_t *intro_key,
-                                   char *rend_circ_nonce)
+encode_establish_intro_cell_legacy(char *cell_body_out,
+                                   size_t cell_body_out_len,
+                                   crypto_pk_t *intro_key, char *rend_circ_nonce)
 {
   int retval = -1;
   int r;
@@ -3202,7 +3203,7 @@ encode_establish_intro_cell_legacy(char *cell_body_out, crypto_pk_t *intro_key,
   len += 20;
   note_crypto_pk_op(REND_SERVER);
   r = crypto_pk_private_sign_digest(intro_key, cell_body_out+len,
-                                    sizeof(cell_body_out)-len,
+                                    cell_body_out_len - len,
                                     cell_body_out, len);
   if (r<0) {
     log_warn(LD_BUG, "Internal error: couldn't sign introduction request.");
@@ -3313,8 +3314,9 @@ rend_service_intro_has_opened(origin_circuit_t *circuit)
   /* Send the ESTABLISH_INTRO cell */
   {
     ssize_t len;
-    len = encode_establish_intro_cell_legacy(buf, circuit->intro_key,
-                                        circuit->cpath->prev->rend_circ_nonce);
+    len = encode_establish_intro_cell_legacy(buf, sizeof(buf),
+                                      circuit->intro_key,
+                                      circuit->cpath->prev->rend_circ_nonce);
     if (len < 0) {
       reason = END_CIRC_REASON_INTERNAL;
       goto err;
diff --git a/src/or/rendservice.h b/src/or/rendservice.h
index 3bfac0b..85daaae 100644
--- a/src/or/rendservice.h
+++ b/src/or/rendservice.h
@@ -130,6 +130,7 @@ STATIC int rend_service_poison_new_single_onion_dir(
                                                   const rend_service_t *s,
                                                   const or_options_t* options);
 STATIC ssize_t encode_establish_intro_cell_legacy(char *cell_body_out,
+                                                  size_t cell_body_out_len,
                                                   crypto_pk_t *intro_key,
                                                   char *rend_circ_nonce);
 STATIC void prune_services_on_reload(smartlist_t *old_service_list,
diff --git a/src/test/test_hs_intropoint.c b/src/test/test_hs_intropoint.c
index ea12aeb..b207cd4 100644
--- a/src/test/test_hs_intropoint.c
+++ b/src/test/test_hs_intropoint.c
@@ -489,6 +489,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,
+                                                sizeof(cell_body),
                                                 key1,
                                                 (char *) circuit_key_material);
   tt_int_op(cell_len, >, 0);