Commit 44265dd6 authored by David Goulet's avatar David Goulet 🐼 Committed by Nick Mathewson
Browse files

sendme: Never fallback to v0 if unknown version 



There was a missing cell version check against our max supported version. In
other words, we do not fallback to v0 anymore in case we do know the SENDME
version.

We can either handle it or not, never fallback to the unauthenticated version
in order to avoid gaming the authenticated logic.

Add a unit tests making sure we properly test that and also test that we can
always handle the default emit and accepted versions.

Fixes #30428

Signed-off-by: David Goulet's avatarDavid Goulet <dgoulet@torproject.org>
parent 69e0d5bf

src/core/or/sendme.c

+26 −26
Original line number Diff line number Diff line
@@ -25,20 +25,6 @@ 
#include "lib/ctime/di_ops.h"

#include "trunnel/sendme.h"



/* The maximum supported version. Above that value, the cell can't be

 * recognized as a valid SENDME. */

#define SENDME_MAX_SUPPORTED_VERSION 1



/* The cell version constants for when emitting a cell. */

#define SENDME_EMIT_MIN_VERSION_DEFAULT 0

#define SENDME_EMIT_MIN_VERSION_MIN 0

#define SENDME_EMIT_MIN_VERSION_MAX UINT8_MAX



/* The cell version constants for when accepting a cell. */

#define SENDME_ACCEPT_MIN_VERSION_DEFAULT 0

#define SENDME_ACCEPT_MIN_VERSION_MIN 0

#define SENDME_ACCEPT_MIN_VERSION_MAX UINT8_MAX



/* Return the minimum version given by the consensus (if any) that should be

 * used when emitting a SENDME cell. */

STATIC int
@@ -123,30 +109,41 @@ cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) 
/* Return true iff the given cell version can be handled or if the minimum

 * accepted version from the consensus is known to us. */

STATIC bool

cell_version_is_valid(uint8_t cell_version)

cell_version_can_be_handled(uint8_t cell_version)

{

  int accept_version = get_accept_min_version();



  /* Can we handle this version? */

  /* We will first check if the consensus minimum accepted version can be

   * handled by us and if not, regardless of the cell version we got, we can't

   * continue. */

  if (accept_version > SENDME_MAX_SUPPORTED_VERSION) {

    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,

           "Unable to accept SENDME version %u (from consensus). "

           "We only support <= %d. Probably your tor is too old?",

           accept_version, cell_version);

           "We only support <= %u. Probably your tor is too old?",

           accept_version, SENDME_MAX_SUPPORTED_VERSION);

    goto invalid;

  }



  /* We only accept a SENDME cell from what the consensus tells us. */

  /* Then, is this version below the accepted version from the consensus? If

   * yes, we must not handle it. */

  if (cell_version < accept_version) {

    log_info(LD_PROTOCOL, "Unacceptable SENDME version %d. Only "

    log_info(LD_PROTOCOL, "Unacceptable SENDME version %u. Only "

                          "accepting %u (from consensus). Closing circuit.",

             cell_version, accept_version);

    goto invalid;

  }



  return 1;

  /* Is this cell version supported by us? */

  if (cell_version > SENDME_MAX_SUPPORTED_VERSION) {

    log_info(LD_PROTOCOL, "SENDME cell version %u is not supported by us. "

                          "We only support <= %u",

             cell_version, SENDME_MAX_SUPPORTED_VERSION);

    goto invalid;

  }



  return true;

 invalid:

  return 0;

  return false;

}



/* Return true iff the encoded SENDME cell in cell_payload of length
@@ -183,7 +180,7 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, 
  }



  /* Validate that we can handle this cell version. */

  if (!cell_version_is_valid(cell_version)) {

  if (!cell_version_can_be_handled(cell_version)) {

    goto invalid;

  }
@@ -195,10 +192,13 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, 
    }

    break;

  case 0x00:

    /* Fallthrough. Version 0, there is no work to be done on the payload so

     * it is necessarily valid if we pass the version validation. */

    /* Version 0, there is no work to be done on the payload so it is

     * necessarily valid if we pass the version validation. */

    break;

  default:

    /* Unknown version means we can't handle it so fallback to version 0. */

    log_warn(LD_PROTOCOL, "Unknown SENDME cell version %d received.",

             cell_version);

    tor_assert_nonfatal_unreached();

    break;

  }

src/core/or/sendme.h

+15 −1
Original line number Diff line number Diff line
@@ -45,6 +45,20 @@ bool sendme_circuit_cell_is_next(int window); 
/* Private section starts. */

#ifdef SENDME_PRIVATE



/* The maximum supported version. Above that value, the cell can't be

 * recognized as a valid SENDME. */

#define SENDME_MAX_SUPPORTED_VERSION 1



/* The cell version constants for when emitting a cell. */

#define SENDME_EMIT_MIN_VERSION_DEFAULT 0

#define SENDME_EMIT_MIN_VERSION_MIN 0

#define SENDME_EMIT_MIN_VERSION_MAX UINT8_MAX



/* The cell version constants for when accepting a cell. */

#define SENDME_ACCEPT_MIN_VERSION_DEFAULT 0

#define SENDME_ACCEPT_MIN_VERSION_MIN 0

#define SENDME_ACCEPT_MIN_VERSION_MAX UINT8_MAX



/*

 * Unit tests declaractions.

 */
@@ -53,7 +67,7 @@ bool sendme_circuit_cell_is_next(int window); 
STATIC int get_emit_min_version(void);

STATIC int get_accept_min_version(void);



STATIC bool cell_version_is_valid(uint8_t cell_version);

STATIC bool cell_version_can_be_handled(uint8_t cell_version);



STATIC ssize_t build_cell_payload_v1(const uint8_t *cell_digest,

                                     uint8_t *payload);

src/test/test_sendme.c

+31 −2
Original line number Diff line number Diff line
@@ -122,9 +122,9 @@ test_v1_consensus_params(void *arg) 
  smartlist_add(current_md_consensus->net_params,

                (void *) "sendme_accept_min_version=1");

  /* Minimum acceptable value is 1. */

  tt_int_op(cell_version_is_valid(1), OP_EQ, true);

  tt_int_op(cell_version_can_be_handled(1), OP_EQ, true);

  /* Minimum acceptable value is 1 so a cell version of 0 is refused. */

  tt_int_op(cell_version_is_valid(0), OP_EQ, false);

  tt_int_op(cell_version_can_be_handled(0), OP_EQ, false);



 done:

  free_mock_consensus();
@@ -239,6 +239,33 @@ test_cell_payload_pad(void *arg) 
  ;

}



static void

test_cell_version_validation(void *arg)

{

  (void) arg;



  /* We currently only support up to SENDME_MAX_SUPPORTED_VERSION so we are

   * going to test the boundaries there. */



  tt_assert(cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION));



  /* Version below our supported should pass. */

  tt_assert(cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION - 1));



  /* Extra version from our supported should fail. */

  tt_assert(!cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION + 1));



  /* Simple check for version 0. */

  tt_assert(cell_version_can_be_handled(0));



  /* We MUST handle the default cell version that we emit or accept. */

  tt_assert(cell_version_can_be_handled(SENDME_EMIT_MIN_VERSION_DEFAULT));

  tt_assert(cell_version_can_be_handled(SENDME_ACCEPT_MIN_VERSION_DEFAULT));



 done:

  ;

}



struct testcase_t sendme_tests[] = {

  { "v1_record_digest", test_v1_record_digest, TT_FORK,

    NULL, NULL },
@@ -248,6 +275,8 @@ struct testcase_t sendme_tests[] = { 
    NULL, NULL },

  { "cell_payload_pad", test_cell_payload_pad, TT_FORK,

    NULL, NULL },

  { "cell_version_validation", test_cell_version_validation, TT_FORK,

    NULL, NULL },



  END_OF_TESTCASES

};