As of ac2f6b608a in 0.2.1.19-alpha,
Sebastian fixed bug 888 by marking descriptors as "impossible" by
digest if they got rejected during the
router_load_routers_from_string() phase. This fix stopped clients
and relays from downloading the same thing over and over.
But we never made the same change for descriptors rejected during
dirserv_add_{descriptor,extrainfo}. Instead, we tried to notice in
advance that we'd reject them with dirserv_would_reject().
This notice-in-advance check stopped working once we added
key-pinning and didn't make a corresponding key-pinning change to
dirserv_would_reject() [since a routerstatus_t doesn't include an
ed25519 key].
So as a fix, let's make the dirserv_add_*() functions mark digests
as undownloadable when they are rejected.
Fixes bug 22349; I am calling this a fix on 0.2.1.19-alpha, though
you could also argue for it being a fix on 0.2.7.2-alpha.
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
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>
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>
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.
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.
Before we've set our options, we can neither call get_options() nor
networkstatus_get_latest_consensus().
Fixes bug 22252; bugfix on 4d9d2553ba
in 0.2.9.3-alpha.
Replace the 177 fallbacks originally introduced in Tor 0.2.9.8 in
December 2016 (of which ~126 were still functional), with a list of
151 fallbacks (32 new, 119 existing, 58 removed) generated in May 2017.
Resolves ticket 21564.
Failure to do this caused an assertion failure with #22246 . This
assertion failure can be triggered remotely, so we're tracking it as
medium-severity TROVE-2017-002.
Closes bug 22245; bugfix on 0.0.9rc1, when bandwidth accounting was
first introduced.
Found by Andrey Karpov and reported at https://www.viva64.com/en/b/0507/
One (HeapEnableTerminationOnCorruption) is on-by-default since win8;
the other (PROCESS_DEP_DISABLE_ATL_THUNK_EMULATION) supposedly only
affects ATL, which (we think) we don't use. Still, these are good
hygiene. Closes ticket 21953.
The GETINFO extra-info/digest/<digest> broke in commit 568dc27a19 that
refactored the base16_decode() API to return the decoded length.
Unfortunately, that if() condition should have checked for the correct length
instead of an error which broke the command in tor-0.2.9.1-alpha.
Fixes#22034
Signed-off-by: David Goulet <dgoulet@torproject.org>
When we "fixed" #18280 in 4e4a7d2b0c
in 0291 it appears that we introduced a bug: The base32_encode
function can read off the end of the input buffer, if the input
buffer size modulo 5 is not equal to 0 or 3.
This is not completely horrible, for two reasons:
* The extra bits that are read are never actually used: so this
is only a crash when asan is enabled, in the worst case. Not a
data leak.
* The input sizes passed to base32_encode are only ever multiples
of 5. They are all either DIGEST_LEN (20), REND_SERVICE_ID_LEN
(10), sizeof(rand_bytes) in addressmap.c (10), or an input in
crypto.c that is forced to a multiple of 5.
So this bug can't actually trigger in today's Tor.
Closes bug 21894; bugfix on 0.2.9.1-alpha.
In order to avoid src/or/hs_service.o to contain no symbols and thus making
clang throw a warning, the functions are now exposed not just to unit tests.
Signed-off-by: David Goulet <dgoulet@torproject.org>
When calculating max sampled size, Tor would only count the number of
bridges in torrc, without considering that our state file might already
have sampled bridges in it. This caused problems when people swap
bridges, since the following error would trigger:
[warn] Not expanding the guard sample any further; just hit the
maximum sample threshold of 1
Make hidden services with 8 to 10 introduction points check for failed
circuits immediately after startup. Previously, they would wait for 5
minutes before performing their first checks.
Fixes bug 21594; bugfix on commit 190aac0eab in Tor 0.2.3.9-alpha.
Reported by alecmuffett.
This change is the only one necessary to allow future versions of
the microdescriptor consensus to replace every 'published' date with
e.g. 2038-01-01 00:00:00; this will save 50-75% in compressed
microdescriptor diff size, which is quite significant.
This commit is a minimal change for 0.2.9; future series will
reduce the use of the 'published' date even more.
Implements part of ticket 21642; implements part of proposal 275.
Previously, they would stop checking when they exceeded their intro point
creation limit.
Fixes bug 21596; bugfix on commit d67bf8b2f2 in Tor 0.2.7.2-alpha.
Reported by alecmuffett.
- Make sure we check at least two guards for descriptor before making
circuits. We typically use the first primary guard for circuits, but
it can also happen that we use the second primary guard (e.g. if we
pick our first primary guard as an exit), so we should make sure we
have descriptors for both of them.
- Remove BUG() from the guard_has_descriptor() check since we now know
that this can happen in rare but legitimate situations as well, and we
should just move to the next guard in that case.
(But use bash if it's available.)
This is a workaround until we remove bash-specific code in 19699.
Fixes bug 21581; bugfix on 21562, not in any released version of tor.
Previously I'd made a bad assumption in the implementation of
prop271 in 0.3.0.1-alpha: I'd assumed that there couldn't be two
guards with the same identity. That's true for non-bridges, but in
the bridge case, we allow two bridges to have the same ID if they
have different addr:port combinations -- in order to have the same
bridge ID running multiple PTs.
Fortunately, this assumption wasn't deeply ingrained: we stop
enforcing the "one guard per ID" rule in the bridge case, and
instead enforce "one guard per <id,addr,port>".
We also needed to tweak our implementation of
get_bridge_info_for_guard, since it made the same incorrect
assumption.
Fixes bug 21027; bugfix on 0.3.0.1-alpha.
Since 0.2.4.11-alpha (in 0196647970) we've tried to randomize
the start time to up to some time in the past. But unfortunately we
allowed the start time to be in the future as well, which isn't
really legit.
The new behavior lets the start time be be up to
MAX(cert_lifetime-2days, 0) in the past, but never in the future.
Fixes bug 21420; bugfix on 0.2.4.11-alpha.
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@torproject.org>
strto* and _atoi64 accept +, -, and various whitespace before numeric
characters. And permitted whitespace is different between POSIX and Windows.
Fixes bug 21507 and part of 21508; bugfix on 0.0.8pre1.
Instead of returning 404 error code, this led to a NULL pointer being used and
thus a crash of tor.
Fixes#21471
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fixes bug 20894; bugfix on 0.2.0.16-alpha.
We already applied a workaround for this as 20834, so no need to
freak out (unless you didn't apply 20384 yet).
Fix for TROVE-2017-001 and bug 21278.
(Note: Instead of handling signed ints "correctly", we keep the old
behavior, except for the part where we would crash with -ftrapv.)
According to 21116, it seems to be needed for Wheezy Raspbian build. Also,
manpage of socket(2) does confirm that this errno value should be catched as
well in case of no support from the OS of IPv4 or/and IPv6.
Fixes#21116
Signed-off-by: David Goulet <dgoulet@torproject.org>
This reverts commit 5446cb8d3d.
The underlying revert was done in 0.2.6, since we aren't backporting
seccomp2 loosening fixes to 0.2.6. But the fix (for 17354) already
went out in 0.2.7.4-rc, so we shouldn't revert it in 0.2.7.
maint-0.2.7-redux is an attempt to try to re-create a plausible
maint-0.2.7 branch. I've started from the tor-0.2.7.6, and then I
merged maint-0.2.6 into the branch.
This has produced 2 conflicts: one related to the
rendcommon->rendcache move, and one to the authority refactoring.
Check size argument to memwipe() for underflow.
Closes bug #18089. Reported by "gk", patch by "teor".
Bugfix on 0.2.3.25 and 0.2.4.6-alpha (#7352),
commit 49dd5ef3 on 7 Nov 2012.
The length of auth_data from an INTRODUCE2 cell is checked when the
auth_type is recognized (1 or 2), but not for any other non-zero
auth_type. Later, auth_data is assumed to have at least
REND_DESC_COOKIE_LEN bytes, leading to a client-triggered out of bounds
read.
Fixed by checking auth_len before comparing the descriptor cookie
against known clients.
Fixes#15823; bugfix on 0.2.1.6-alpha.
Bug 21242 occurred because we asserted that extend_info_from_node()
had succeeded...even though we already had the code to handle such a
failure. We fixed that in 93b39c5162.
But there were four other cases in our code where we called
extend_info_from_node() and either tor_assert()ed that it returned
non-NULL, or [in one case] silently assumed that it returned
non-NULL. That's not such a great idea. This patch makes those
cases check for a bug of this kind instead.
Fixes bug 21372; bugfix on 0.2.3.1-alpha when
extend_info_from_node() was introduced.
Once a second, we go over all services and consider the validity of the intro
points. Now, also try to remove expiring nodes that have no more circuit
associated to them. This is possible if we moved an intro point object
previously to that list and the circuit actually timed out or was closed by
the introduction point itself.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously the dirserv_orconn_tls_done() function would skip routers
when they advertised an ed25519 key but didn't present it during the
link handshake. But that covers all versions between 0.2.7.2-alpha
and 0.2.9.x inclusive!
Fixes bug 21107; bugfix on 0.3.0.1-alpha.
Because we don't allow client functionalities in non anonymous mode,
recommending Tor2web is a bad idea.
If a user wants to use Tor2web as a client (losing all anonymity), it should
run a second tor, not use it with a single onion service tor.
Fixes#21294.
Signed-off-by: David Goulet <dgoulet@torproject.org>
