Merge branch 'tor-github/pr/1925'

changes/bug30992

@@ -0,0 +1,4 @@
+  o Minor bugfixes (circuitpadding):
+    - Add a per-circuit padding machine instance counter, so we can
+      differentiate between shutdown requests for old machines on a circuit;
+      Fixes bug 30992; bugfix on 0.4.1.1-alpha.

src/core/or/circuit_st.h

@@ -238,6 +238,12 @@ struct circuit_t {
    *  Each element of this array corresponds to a different padding machine,
    *  and we can have up to CIRCPAD_MAX_MACHINES such machines. */
   struct circpad_machine_runtime_t *padding_info[CIRCPAD_MAX_MACHINES];
+
+  /** padding_machine_ctr increments each time a new padding machine
+   * is negotiated. It is used for shutdown conditions, to ensure
+   * that STOP commands actually correspond to the current machine,
+   * and not a previous one. */
+  uint32_t padding_machine_ctr;
 };
 
 #endif /* !defined(CIRCUIT_ST_H) */

src/core/or/circuitpadding.c

@@ -266,18 +266,31 @@ circpad_marked_circuit_for_padding(circuit_t *circ, int reason)
 /**
  * Free all the machineinfos in <b>circ</b> that match <b>machine_num</b>.
  *
+ * If machine_ctr is non-zero, also make sure it matches the padding_info's
+ * machine counter before freeing.
+ *
  * Returns true if any machineinfos with that number were freed.
  * False otherwise. */
 static int
-free_circ_machineinfos_with_machine_num(circuit_t *circ, int machine_num)
+free_circ_machineinfos_with_machine_num(circuit_t *circ, int machine_num,
+                                        uint32_t machine_ctr)
 {
   int found = 0;
   FOR_EACH_CIRCUIT_MACHINE_BEGIN(i) {
     if (circ->padding_machine[i] &&
         circ->padding_machine[i]->machine_num == machine_num) {
-      circpad_circuit_machineinfo_free_idx(circ, i);
+      /* If machine_ctr is non-zero, make sure it matches too. This
-      circ->padding_machine[i] = NULL;
+       * is to ensure that old STOP messages don't shutdown newer machines. */
-      found = 1;
+      if (machine_ctr && circ->padding_info[i] &&
+          circ->padding_info[i]->machine_ctr != machine_ctr) {
+        log_info(LD_CIRC,
+                 "Padding shutdown for wrong (old?) machine ctr: %u vs %u",
+                 machine_ctr, circ->padding_info[i]->machine_ctr);
+      } else {
+        circpad_circuit_machineinfo_free_idx(circ, i);
+        circ->padding_machine[i] = NULL;
+        found = 1;
+      }
     }
   } FOR_EACH_CIRCUIT_MACHINE_END;
 
@@ -306,6 +319,7 @@ circpad_circuit_machineinfo_new(circuit_t *on_circ, int machine_index)
   mi->machine_index = machine_index;
   mi->on_circ = on_circ;
   mi->last_cell_time_sec = approx_time();
+  mi->machine_ctr = on_circ->padding_machine_ctr;
 
   return mi;
 }
@@ -1556,19 +1570,23 @@ circpad_machine_spec_transitioned_to_end(circpad_machine_runtime_t *mi)
       /* We free the machine info here so that we can be replaced
        * by a different machine. But we must leave the padding_machine
        * in place to wait for the negotiated response */
+      uint32_t machine_ctr = mi->machine_ctr;
       circpad_circuit_machineinfo_free_idx(on_circ,
                                            machine->machine_index);
       circpad_negotiate_padding(TO_ORIGIN_CIRCUIT(on_circ),
                                 machine->machine_num,
                                 machine->target_hopnum,
-                                CIRCPAD_COMMAND_STOP);
+                                CIRCPAD_COMMAND_STOP,
+                                machine_ctr);
     } else {
+      uint32_t machine_ctr = mi->machine_ctr;
       circpad_circuit_machineinfo_free_idx(on_circ,
                                            machine->machine_index);
       circpad_padding_negotiated(on_circ,
                                 machine->machine_num,
                                 CIRCPAD_COMMAND_STOP,
-                                CIRCPAD_RESPONSE_OK);
+                                CIRCPAD_RESPONSE_OK,
+                                machine_ctr);
       on_circ->padding_machine[machine->machine_index] = NULL;
     }
   }
@@ -2099,13 +2117,15 @@ circpad_shutdown_old_machines(origin_circuit_t *on_circ)
   FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, circ) {
     if (!circpad_machine_conditions_met(on_circ,
                                         circ->padding_machine[i])) {
+      uint32_t machine_ctr = circ->padding_info[i]->machine_ctr;
       // Clear machineinfo (frees timers)
       circpad_circuit_machineinfo_free_idx(circ, i);
       // Send padding negotiate stop
       circpad_negotiate_padding(on_circ,
                                 circ->padding_machine[i]->machine_num,
                                 circ->padding_machine[i]->target_hopnum,
-                                CIRCPAD_COMMAND_STOP);
+                                CIRCPAD_COMMAND_STOP,
+                                machine_ctr);
     }
   } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END;
 }
@@ -2172,7 +2192,8 @@ circpad_add_matching_machines(origin_circuit_t *on_circ,
         circpad_setup_machine_on_circ(circ, machine);
         if (circpad_negotiate_padding(on_circ, machine->machine_num,
                                   machine->target_hopnum,
-                                  CIRCPAD_COMMAND_START) < 0) {
+                                  CIRCPAD_COMMAND_START,
+                                  circ->padding_machine_ctr) < 0) {
           log_info(LD_CIRC,
                    "Padding not negotiated. Cleaning machine from circuit %u",
              CIRCUIT_IS_ORIGIN(circ) ?
@@ -2463,6 +2484,17 @@ circpad_setup_machine_on_circ(circuit_t *on_circ,
              machine->name, on_circ->purpose);
   }
 
+  /* Padding machine ctr starts at 1, so we increment this ctr first.
+   * (machine ctr of 0 means "any machine").
+   *
+   * See https://bugs.tororject.org/30992. */
+  on_circ->padding_machine_ctr++;
+
+  /* uint32 wraparound check: 0 is special, just wrap to 1 */
+  if (on_circ->padding_machine_ctr == 0) {
+    on_circ->padding_machine_ctr = 1;
+  }
+
   on_circ->padding_info[machine->machine_index] =
       circpad_circuit_machineinfo_new(on_circ, machine->machine_index);
   on_circ->padding_machine[machine->machine_index] = machine;
@@ -2816,7 +2848,8 @@ signed_error_t
 circpad_negotiate_padding(origin_circuit_t *circ,
                           circpad_machine_num_t machine,
                           uint8_t target_hopnum,
-                          uint8_t command)
+                          uint8_t command,
+                          uint32_t machine_ctr)
 {
   circpad_negotiate_t type;
   cell_t cell;
@@ -2838,14 +2871,16 @@ circpad_negotiate_padding(origin_circuit_t *circ,
   circpad_negotiate_set_command(&type, command);
   circpad_negotiate_set_version(&type, 0);
   circpad_negotiate_set_machine_type(&type, machine);
+  circpad_negotiate_set_machine_ctr(&type, machine_ctr);
 
   if ((len = circpad_negotiate_encode(cell.payload, CELL_PAYLOAD_SIZE,
         &type)) < 0)
     return -1;
 
   log_fn(LOG_INFO,LD_CIRC,
-         "Negotiating padding on circuit %u (%d), command %d",
+         "Negotiating padding on circuit %u (%d), command %d, for ctr %u",
-         circ->global_identifier, TO_CIRCUIT(circ)->purpose, command);
+         circ->global_identifier, TO_CIRCUIT(circ)->purpose, command,
+         machine_ctr);
 
   return circpad_send_command_to_hop(circ, target_hopnum,
                                      RELAY_COMMAND_PADDING_NEGOTIATE,
@@ -2861,7 +2896,8 @@ bool
 circpad_padding_negotiated(circuit_t *circ,
                            circpad_machine_num_t machine,
                            uint8_t command,
-                           uint8_t response)
+                           uint8_t response,
+                           uint32_t machine_ctr)
 {
   circpad_negotiated_t type;
   cell_t cell;
@@ -2878,6 +2914,7 @@ circpad_padding_negotiated(circuit_t *circ,
   circpad_negotiated_set_response(&type, response);
   circpad_negotiated_set_version(&type, 0);
   circpad_negotiated_set_machine_type(&type, machine);
+  circpad_negotiated_set_machine_ctr(&type, machine_ctr);
 
   if ((len = circpad_negotiated_encode(cell.payload, CELL_PAYLOAD_SIZE,
         &type)) < 0)
@@ -2923,19 +2960,33 @@ circpad_handle_padding_negotiate(circuit_t *circ, cell_t *cell)
   if (negotiate->command == CIRCPAD_COMMAND_STOP) {
     /* Free the machine corresponding to this machine type */
     if (free_circ_machineinfos_with_machine_num(circ,
-                negotiate->machine_type)) {
+                negotiate->machine_type,
-      log_info(LD_CIRC, "Received STOP command for machine %u",
+                negotiate->machine_ctr)) {
-               negotiate->machine_type);
+      log_info(LD_CIRC, "Received STOP command for machine %u, ctr %u",
+               negotiate->machine_type, negotiate->machine_ctr);
       goto done;
     }
-    log_fn(LOG_PROTOCOL_WARN, LD_CIRC,
+    if (negotiate->machine_ctr <= circ->padding_machine_ctr) {
-          "Received circuit padding stop command for unknown machine.");
+      log_info(LD_CIRC, "Received STOP command for old machine %u, ctr %u",
-    goto err;
+               negotiate->machine_type, negotiate->machine_ctr);
-  } else if (negotiate->command == CIRCPAD_COMMAND_START) {
+      goto done;
+
+    } else {
+      log_fn(LOG_PROTOCOL_WARN, LD_CIRC,
+                "Received circuit padding stop command for unknown machine.");
+      goto err;
+    }
+ } else if (negotiate->command == CIRCPAD_COMMAND_START) {
     SMARTLIST_FOREACH_BEGIN(relay_padding_machines,
                             const circpad_machine_spec_t *, m) {
       if (m->machine_num == negotiate->machine_type) {
         circpad_setup_machine_on_circ(circ, m);
+        if (negotiate->machine_ctr &&
+            circ->padding_machine_ctr != negotiate->machine_ctr) {
+            log_fn(LOG_PROTOCOL_WARN, LD_CIRC,
+              "Client and relay have different counts for padding machines: "
+              "%u vs %u", circ->padding_machine_ctr, negotiate->machine_ctr);
+        }
         circpad_cell_event_nonpadding_received(circ);
         goto done;
       }
@@ -2948,7 +2999,8 @@ circpad_handle_padding_negotiate(circuit_t *circ, cell_t *cell)
   done:
     circpad_padding_negotiated(circ, negotiate->machine_type,
                    negotiate->command,
-                   (retval == 0) ? CIRCPAD_RESPONSE_OK : CIRCPAD_RESPONSE_ERR);
+                   (retval == 0) ? CIRCPAD_RESPONSE_OK : CIRCPAD_RESPONSE_ERR,
+                   negotiate->machine_ctr);
     circpad_negotiate_free(negotiate);
 
     return retval;
@@ -2999,17 +3051,22 @@ circpad_handle_padding_negotiated(circuit_t *circ, cell_t *cell,
      * circpad_add_matching_matchines() added a new machine,
      * there may be a padding_machine for a different machine num
      * than this response. */
-    free_circ_machineinfos_with_machine_num(circ, negotiated->machine_type);
+    free_circ_machineinfos_with_machine_num(circ, negotiated->machine_type,
+                                            negotiated->machine_ctr);
   } else if (negotiated->command == CIRCPAD_COMMAND_START &&
              negotiated->response == CIRCPAD_RESPONSE_ERR) {
-    // This can happen due to consensus drift.. free the machines
+    // This can still happen due to consensus drift.. free the machines
     // and be sad
-    free_circ_machineinfos_with_machine_num(circ, negotiated->machine_type);
+    if (free_circ_machineinfos_with_machine_num(circ, negotiated->machine_type,
-    TO_ORIGIN_CIRCUIT(circ)->padding_negotiation_failed = 1;
+                                            negotiated->machine_ctr)) {
-    log_fn(LOG_PROTOCOL_WARN, LD_CIRC,
+      // Only fail if a machine was there and matched the error cell
-           "Middle node did not accept our padding request on circuit %u (%d)",
+      TO_ORIGIN_CIRCUIT(circ)->padding_negotiation_failed = 1;
-           TO_ORIGIN_CIRCUIT(circ)->global_identifier,
+      log_fn(LOG_PROTOCOL_WARN, LD_CIRC,
-           circ->purpose);
+             "Middle node did not accept our padding request on circuit "
+             "%u (%d)",
+             TO_ORIGIN_CIRCUIT(circ)->global_identifier,
+             circ->purpose);
+    }
   }
 
   circpad_negotiated_free(negotiated);

src/core/or/circuitpadding.h

@@ -565,6 +565,13 @@ typedef struct circpad_machine_runtime_t {
   /** What state is this machine in? */
   circpad_statenum_t current_state;
 
+  /** Machine counter, for shutdown sync.
+   *
+   *  Set from circuit_t.padding_machine_ctr, which is incremented each
+   *  padding machine instantiation.
+   */
+  uint32_t machine_ctr;
+
   /**
    * True if we have scheduled a timer for padding.
    *
@@ -726,11 +733,13 @@ signed_error_t circpad_handle_padding_negotiated(struct circuit_t *circ,
 signed_error_t circpad_negotiate_padding(struct origin_circuit_t *circ,
                           circpad_machine_num_t machine,
                           uint8_t target_hopnum,
-                          uint8_t command);
+                          uint8_t command,
+                          uint32_t machine_ctr);
 bool circpad_padding_negotiated(struct circuit_t *circ,
                            circpad_machine_num_t machine,
                            uint8_t command,
-                           uint8_t response);
+                           uint8_t response,
+                           uint32_t machine_ctr);
 
 circpad_purpose_mask_t circpad_circ_purpose_to_mask(uint8_t circ_purpose);
 

src/test/test_circuitpadding.c

@@ -1361,7 +1361,7 @@ test_circuitpadding_wronghop(void *arg)
 
   /* 5. Test that asking to stop the wrong machine does nothing */
   circpad_negotiate_padding(TO_ORIGIN_CIRCUIT(client_side),
-                            255, 2, CIRCPAD_COMMAND_STOP);
+                            255, 2, CIRCPAD_COMMAND_STOP, 0);
   tt_ptr_op(client_side->padding_machine[0], OP_NE, NULL);
   tt_ptr_op(client_side->padding_info[0], OP_NE, NULL);
   tt_ptr_op(relay_side->padding_machine[0], OP_NE, NULL);
@@ -1409,7 +1409,7 @@ test_circuitpadding_wronghop(void *arg)
   circpad_padding_negotiated(relay_side,
                              CIRCPAD_MACHINE_CIRC_SETUP,
                              CIRCPAD_COMMAND_START,
-                             CIRCPAD_RESPONSE_OK);
+                             CIRCPAD_RESPONSE_OK, 0);
 
   /* verify no padding was negotiated */
   tt_ptr_op(relay_side->padding_machine[0], OP_EQ, NULL);
@@ -1418,7 +1418,7 @@ test_circuitpadding_wronghop(void *arg)
   circpad_padding_negotiated(relay_side,
                              CIRCPAD_MACHINE_CIRC_SETUP,
                              CIRCPAD_COMMAND_START,
-                             CIRCPAD_RESPONSE_ERR);
+                             CIRCPAD_RESPONSE_ERR, 0);
 
   /* verify no padding was negotiated */
   tt_ptr_op(relay_side->padding_machine[0], OP_EQ, NULL);
@@ -1521,7 +1521,7 @@ test_circuitpadding_negotiation(void *arg)
   /* Force negotiate padding. */
   circpad_negotiate_padding(TO_ORIGIN_CIRCUIT(client_side),
                             CIRCPAD_MACHINE_CIRC_SETUP,
-                            2, CIRCPAD_COMMAND_START);
+                            2, CIRCPAD_COMMAND_START, 0);
 
   /* verify no padding was negotiated */
   tt_ptr_op(relay_side->padding_machine[0], OP_EQ, NULL);

src/trunnel/circpad_negotiation.c

@@ -112,6 +112,17 @@ circpad_negotiate_set_echo_request(circpad_negotiate_t *inp, uint8_t val)
   inp->echo_request = val;
   return 0;
 }
+uint32_t
+circpad_negotiate_get_machine_ctr(const circpad_negotiate_t *inp)
+{
+  return inp->machine_ctr;
+}
+int
+circpad_negotiate_set_machine_ctr(circpad_negotiate_t *inp, uint32_t val)
+{
+  inp->machine_ctr = val;
+  return 0;
+}
 const char *
 circpad_negotiate_check(const circpad_negotiate_t *obj)
 {
@@ -148,6 +159,9 @@ circpad_negotiate_encoded_len(const circpad_negotiate_t *obj)
 
   /* Length of u8 echo_request IN [0, 1] */
   result += 1;
+
+  /* Length of u32 machine_ctr */
+  result += 4;
   return result;
 }
 int
@@ -203,6 +217,13 @@ circpad_negotiate_encode(uint8_t *output, const size_t avail, const circpad_nego
   trunnel_set_uint8(ptr, (obj->echo_request));
   written += 1; ptr += 1;
 
+  /* Encode u32 machine_ctr */
+  trunnel_assert(written <= avail);
+  if (avail - written < 4)
+    goto truncated;
+  trunnel_set_uint32(ptr, trunnel_htonl(obj->machine_ctr));
+  written += 4; ptr += 4;
+
 
   trunnel_assert(ptr == output + written);
 #ifdef TRUNNEL_CHECK_ENCODED_LEN
@@ -263,6 +284,11 @@ circpad_negotiate_parse_into(circpad_negotiate_t *obj, const uint8_t *input, con
   remaining -= 1; ptr += 1;
   if (! (obj->echo_request == 0 || obj->echo_request == 1))
     goto fail;
+
+  /* Parse u32 machine_ctr */
+  CHECK_REMAINING(4, truncated);
+  obj->machine_ctr = trunnel_ntohl(trunnel_get_uint32(ptr));
+  remaining -= 4; ptr += 4;
   trunnel_assert(ptr + remaining == input + len_in);
   return len_in - remaining;
 
@@ -372,6 +398,17 @@ circpad_negotiated_set_machine_type(circpad_negotiated_t *inp, uint8_t val)
   inp->machine_type = val;
   return 0;
 }
+uint32_t
+circpad_negotiated_get_machine_ctr(const circpad_negotiated_t *inp)
+{
+  return inp->machine_ctr;
+}
+int
+circpad_negotiated_set_machine_ctr(circpad_negotiated_t *inp, uint32_t val)
+{
+  inp->machine_ctr = val;
+  return 0;
+}
 const char *
 circpad_negotiated_check(const circpad_negotiated_t *obj)
 {
@@ -408,6 +445,9 @@ circpad_negotiated_encoded_len(const circpad_negotiated_t *obj)
 
   /* Length of u8 machine_type */
   result += 1;
+
+  /* Length of u32 machine_ctr */
+  result += 4;
   return result;
 }
 int
@@ -463,6 +503,13 @@ circpad_negotiated_encode(uint8_t *output, const size_t avail, const circpad_neg
   trunnel_set_uint8(ptr, (obj->machine_type));
   written += 1; ptr += 1;
 
+  /* Encode u32 machine_ctr */
+  trunnel_assert(written <= avail);
+  if (avail - written < 4)
+    goto truncated;
+  trunnel_set_uint32(ptr, trunnel_htonl(obj->machine_ctr));
+  written += 4; ptr += 4;
+
 
   trunnel_assert(ptr == output + written);
 #ifdef TRUNNEL_CHECK_ENCODED_LEN
@@ -523,6 +570,11 @@ circpad_negotiated_parse_into(circpad_negotiated_t *obj, const uint8_t *input, c
   CHECK_REMAINING(1, truncated);
   obj->machine_type = (trunnel_get_uint8(ptr));
   remaining -= 1; ptr += 1;
+
+  /* Parse u32 machine_ctr */
+  CHECK_REMAINING(4, truncated);
+  obj->machine_ctr = trunnel_ntohl(trunnel_get_uint32(ptr));
+  remaining -= 4; ptr += 4;
   trunnel_assert(ptr + remaining == input + len_in);
   return len_in - remaining;
 

src/trunnel/circpad_negotiation.h

@@ -26,6 +26,7 @@ struct circpad_negotiate_st {
   uint8_t machine_type;
   /** If true, send a relay_drop reply.. */
   uint8_t echo_request;
+  uint32_t machine_ctr;
   uint8_t trunnel_error_code_;
 };
 #endif
@@ -42,6 +43,14 @@ struct circpad_negotiated_st {
   /** Machine type is left unbounded because we can specify
      * new machines in the consensus */
   uint8_t machine_type;
+  /**
+     * This field is used for shutdown synchronization. It is OK if
+     * it wraps, because all we need to do is make sure the STOP
+     * command is actually for the currently active machine.
+     * For backward-compatibility, though, 0 has special meaning
+     * (it means match any machine).
+     */
+  uint32_t machine_ctr;
   uint8_t trunnel_error_code_;
 };
 #endif
@@ -118,6 +127,15 @@ uint8_t circpad_negotiate_get_echo_request(const circpad_negotiate_t *inp);
  * code on 'inp' on failure.
  */
 int circpad_negotiate_set_echo_request(circpad_negotiate_t *inp, uint8_t val);
+/** Return the value of the machine_ctr field of the
+ * circpad_negotiate_t in 'inp'
+ */
+uint32_t circpad_negotiate_get_machine_ctr(const circpad_negotiate_t *inp);
+/** Set the value of the machine_ctr field of the circpad_negotiate_t
+ * in 'inp' to 'val'. Return 0 on success; return -1 and set the error
+ * code on 'inp' on failure.
+ */
+int circpad_negotiate_set_machine_ctr(circpad_negotiate_t *inp, uint32_t val);
 /** Return a newly allocated circpad_negotiated with all elements set
  * to zero.
  */
@@ -190,6 +208,15 @@ uint8_t circpad_negotiated_get_machine_type(const circpad_negotiated_t *inp);
  * -1 and set the error code on 'inp' on failure.
  */
 int circpad_negotiated_set_machine_type(circpad_negotiated_t *inp, uint8_t val);
+/** Return the value of the machine_ctr field of the
+ * circpad_negotiated_t in 'inp'
+ */
+uint32_t circpad_negotiated_get_machine_ctr(const circpad_negotiated_t *inp);
+/** Set the value of the machine_ctr field of the circpad_negotiated_t
+ * in 'inp' to 'val'. Return 0 on success; return -1 and set the error
+ * code on 'inp' on failure.
+ */
+int circpad_negotiated_set_machine_ctr(circpad_negotiated_t *inp, uint32_t val);
 
 
 #endif

src/trunnel/circpad_negotiation.trunnel

@@ -27,6 +27,13 @@ struct circpad_negotiate {
   // FIXME-MP-AP: Maybe we just say to transition to the first state
   // here instead.. Also what about delay before responding?
   u8 echo_request IN [0,1];
+
+  // This field is used for shutdown synchronization. It is OK if
+  // it wraps, because all we need to do is make sure the STOP
+  // command is actually for the currently active machine.
+  // For backward-compatibility, though, 0 has special meaning
+  // (it means match any machine).
+  u32 machine_ctr;
 };
 
 /**
@@ -41,4 +48,14 @@ struct circpad_negotiated {
   /** Machine type is left unbounded because we can specify
    * new machines in the consensus */
   u8 machine_type;
+
+  /**
+   * This field is used for shutdown synchronization. It is OK if
+   * it wraps, because all we need to do is make sure the STOP
+   * command is actually for the currently active machine.
+   * For backward-compatibility, though, 0 has special meaning
+   * (it means match any machine).
+   */
+  u32 machine_ctr;
+
 };