This mistake causes two possible bugs. I believe they are both
harmless IRL.
BUG 1: memory stomping
When we call the memset, we are overwriting two 0 bytes past the end
of packed_cell_t.body. But I think that's harmless in practice,
because the definition of packed_cell_t is:
// ...
typedef struct packed_cell_t {
TOR_SIMPLEQ_ENTRY(packed_cell_t) next;
char body[CELL_MAX_NETWORK_SIZE];
uint32_t inserted_time;
} packed_cell_t;
So we will overwrite either two bytes of inserted_time, or two bytes
of padding, depending on how the platform handles alignment.
If we're overwriting padding, that's safe.
If we are overwriting the inserted_time field, that's also safe: In
every case where we call cell_pack() from connection_or.c, we ignore
the inserted_time field. When we call cell_pack() from relay.c, we
don't set or use inserted_time until right after we have called
cell_pack(). SO I believe we're safe in that case too.
BUG 2: memory exposure
The original reason for this memset was to avoid the possibility of
accidentally leaking uninitialized ram to the network. Now
remember, if wide_circ_ids is false on a connection, we shouldn't
actually be sending more than 512 bytes of packed_cell_t.body, so
these two bytes can only leak to the network if there is another bug
somewhere else in the code that sends more data than is correct.
Fortunately, in relay.c, where we allocate packed_cell_t in
packed_cell_new() , we allocate it with tor_malloc_zero(), which
clears the RAM, right before we call cell_pack. So those
packed_cell_t.body bytes can't leak any information.
That leaves the two calls to cell_pack() in connection_or.c, which
use stack-alocated packed_cell_t instances.
In or_handshake_state_record_cell(), we pass the cell's contents to
crypto_digest_add_bytes(). When we do so, we get the number of
bytes to pass using the same setting of wide_circ_ids as we passed
to cell_pack(). So I believe that's safe.
In connection_or_write_cell_to_buf(), we also use the same setting
of wide_circ_ids in both calls. So I believe that's safe too.
I introduced this bug with 1c0e87f6d8
back in 0.2.4.11-alpha; it is bug 22737 and CID 1401591
This prevents us from calling
allowed_anonymous_connection_compression_method() on the unused
guessed method (if any), and rejecting something that was already
safe to use.
Rationale: When use a guessed compression method, we already gave a
PROTOCOL_WARN when our guess differed from the declared method,
AND we gave a PROTOCOL_WARN when the declared method failed. It is
not a protocol problem that the guessed method failed too; it's just
a recovery attempt that failed.
A cached_dir_t object (for now) is always compressed with
DEFLATE_METHOD, but in handle_get_status_vote() to we were using the
general compression-negotiation code decide what compression to
claim we were using.
This was one of the reasons behind 22502.
Fixes bug 22669; bugfix on 0.3.1.1-alpha
On an hidden service rendezvous circuit, a BEGIN_DIR could be sent
(maliciously) which would trigger a tor_assert() because
connection_edge_process_relay_cell() thought that the circuit is an
or_circuit_t but is an origin circuit in reality.
Fixes#22494
Reported-by: Roger Dingledine <arma@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
This fixes an assertion failure in relay_send_end_cell_from_edge_() when an
origin circuit and a cpath_layer = NULL were passed.
A service rendezvous circuit could do such a thing when a malformed BEGIN cell
is received but shouldn't in the first place because the service needs to send
an END cell on the circuit for which it can not do without a cpath_layer.
Fixes#22493
Reported-by: Roger Dingledine <arma@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
Apparently, the unit tests relied on being able to make ed->x509
link certs even when they hadn't set any server flags in the
options. So instead of making "client" mean "never generate an
ed->x509 cert", we'll have it mean "it's okay not to generate an
ed->x509 cert".
(Going with a minimal fix here, since this is supposed to be a
stable version.)
It's okay to call add_ed25519_cert with a NULL argument: so,
document that. Also, add a tor_assert_nonfatal() to catch any case
where we have failed to set own_link_cert when conn_in_server_mode.
Whenever we rotate our TLS context, we change our Ed25519
Signing->Link certificate. But if we've already started a TLS
connection, then we've already sent the old X509 link certificate,
so the new Ed25519 Signing->Link certificate won't match it.
To fix this, we now store a copy of the Signing->Link certificate
when we initialize the handshake state, and send that certificate
as part of our CERTS cell.
Fixes one case of bug22460; bugfix on 0.3.0.1-alpha.
Previously we could sometimes change our signing key, but not
regenerate the certificates (signing->link and signing->auth) that
were signed with it. Also, we would regularly replace our TLS x.509
link certificate (by rotating our TLS context) but not replace our
signing->link ed25519 certificate. In both cases, the resulting
inconsistency would make other relays reject our link handshakes.
Fixes two cases of bug 22460; bugfix on 0.3.0.1-alpha.
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@torproject.org>
A fair number of our mock_impl declarations were messed up so that
even our special AM_ETAGSFLAGS couldn't find them.
This should be a whitespace-only patch.
When directory authorities reject a router descriptor due to keypinning,
free the router descriptor rather than leaking the memory.
Fixes bug 22370; bugfix on 0.2.7.2-alpha.
If we add the element itself, we will later free it when we free the
descriptor, and the next time we go to look at MyFamily, things will
go badly.
Fixes the rest of bug 22368; bugfix on 0.3.1.1-alpha.
If we free them here, we will still attempt to access the freed memory
later on, and also we will double-free when we are freeing the config.
Fixes part of bug 22368.
This patch lifts the return value, rv, variable to the beginning of the
function, adds a 'done' label for clean-up and function exit and makes
the rest of the function use the rv value + goto done; instead of
cleaning up in multiple places.
See: https://bugs.torproject.org/22305
We used to not set the guard state in launch_direct_bridge_descriptor_fetch().
So when a bridge descriptor fetch failed, the guard subsystem would never
learn about the fail (and hence the guard's reachability state would not
be updated).
We used to not set the guard state in launch_direct_bridge_descriptor_fetch().
So when a bridge descriptor fetch failed, the guard subsystem would never
learn about the fail (and hence the guard's reachability state would not
be updated).
Directory authorities now reject relays running versions
0.2.9.1-alpha through 0.2.9.4-alpha, because those relays
suffer from bug 20499 and don't keep their consensus cache
up-to-date.
Resolves ticket 20509.
This gives an indication in the log that Tor was built with Rust
support, as well as laying some groundwork for further string-returning
APIs to be converted to Rust
config_get_lines is now split into two functions:
- config_get_lines which is the same as before we had %include
- config_get_lines_include which actually processes %include
