commit 44265dd6716887b997bb03d2db1641efd7ae9c19 Author: David Goulet <dgoulet@torproject.org> Date: Tue May 7 09:44:10 2019 -0400 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 <dgoulet@torproject.org> --- src/core/or/sendme.c | 52 +++++++++++++++++++++++++------------------------- src/core/or/sendme.h | 16 +++++++++++++++- src/test/test_sendme.c | 33 ++++++++++++++++++++++++++++++-- 3 files changed, 72 insertions(+), 29 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index baa57f4f2..80d6b7d16 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -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; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 2fb73f76d..a71891996 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -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); diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index 463a0ec08..a36c904c2 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -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 };