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
If COMPRESS_OK occurs but data is neither consumed nor generated,
treat it as a BUG and a COMPRESS_ERROR.
This change is meant to prevent infinite loops in the case where
we've made a mistake in one of our compression backends.
Closes ticket 22672.
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>
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.
I went into this to fix 6892 and say "we don't do anything for
circuit isolation." But instead I did a fair amount of text-removal
to stop implying that torify does anything more than call torsocks.
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.
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.
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.
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
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.
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Deprecated in 0.2.9.2-alpha, this commits changes it as OBSOLETE() and cleans
up the code associated with it.
Partially fixes#22060
Signed-off-by: David Goulet <dgoulet@torproject.org>
Accomplished via the following:
1. Use NETINFO cells to determine if both peers will agree on canonical
status. Prefer connections where they agree to those where they do not.
2. Alter channel_is_better() to prefer older orconns in the case of multiple
canonical connections, and use the orconn with more circuits on it in case
of age ties.
Also perform some hourly accounting on how many of these types of connections
there are and log it at info or notice level.
This unifies CircuitIdleTimeout and PredictedCircsRelevanceTime into a single
option, and randomizes it.
It also gives us control over the default value as well as relay-to-relay
connection lifespan through the consensus.
Conflicts:
src/or/circuituse.c
src/or/config.c
src/or/main.c
src/test/testing_common.c
This defense will cause Cisco, Juniper, Fortinet, and other routers operating
in the default configuration to collapse netflow records that would normally
be split due to the 15 second flow idle timeout.
Collapsing these records should greatly reduce the utility of default netflow
data for correlation attacks, since all client-side records should become 30
minute chunks of total bytes sent/received, rather than creating multiple
separate records for every webpage load/ssh command interaction/XMPP chat/whatever
else happens to be inactive for more than 15 seconds.
The defense adds consensus parameters to govern the range of timeout values
for sending padding packets, as well as for keeping connections open.
The defense only sends padding when connections are otherwise inactive, and it
does not pad connections used solely for directory traffic at all. By default
it also doesn't pad inter-relay connections.
Statistics on the total padding in the last 24 hours are exported to the
extra-info descriptors.
This was a >630-line function, which doesn't make anybody happy. It
was also mostly composed of a bunch of if-statements that handled
different directory responses differently depending on the original
purpose of the directory connection. The logical refactoring here
is to move the body of each switch statement into a separate handler
function, and to invoke those functions from a separate switch
statement.
This commit leaves whitespace mostly untouched, for ease of review.
I'll reindent in the next commit.
Inform the control port with an HS_DESC failed event when the client is unable
to pick an HSDir. It's followed by an empty HS_DESC_CONTENT event. In order to
achieve that, some control port code had to be modified to accept a NULL HSDir
identity digest.
This commit also adds a trigger of a failed event when we are unable to
base64-decode the descriptor cookie.
Fixes#22042
Signed-off-by: David Goulet <dgoulet@torproject.org>
Increase the maximum allowed size passed to mprotect(PROT_WRITE)
from 1MB to 16MB. This was necessary with the glibc allocator
in order to allow worker threads to allocate more memory --
which in turn is necessary because of our new use of worker
threads for compression.
Closes ticket #22096. Found while working on #21648.
Bridge lines in torrc can contain key=value settings as per-connection
arguments to a pluggable transport. tor.1.txt hadn't been updated to
reflect this.
This commit adds the src/trace directory containing the basics for our tracing
subsystem. It is not used in the code base. The "src/trace/debug.h" file
contains an example on how we can map our tor trace events to log_debug().
The tracing subsystem can only be enabled by tracing framework at compile
time. This commit introduces the "--enable-tracing-debug" option that will
make all "tor_trace()" function be maped to "log_debug()".
Closes#13802
Signed-off-by: David Goulet <dgoulet@torproject.org>
That log statement can be triggered if somebody on the Internet behaves badly
which is possible with buggy implementation for instance.
Fixes#21293
Signed-off-by: David Goulet <dgoulet@torproject.org>
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>
Pinning EntryNodes along with hidden services can be possibly harmful (for
instance #14917 and #21155) so at the very least warn the operator if this is
the case.
Fixes#21155
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.
When a client tried to connect to an invalid port of an hidden service, a
warning was printed:
[warn] connection_edge_process_relay_cell (at origin) failed.
This is because the connection subsystem wants to close the circuit because
the port can't be found and then returns a negative reason to achieve that.
However, that specific situation triggered a warning. This commit prevents it
for the specific case of an invalid hidden service port.
Fixes#16706
Signed-off-by: David Goulet <dgoulet@torproject.org>
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>
The old implementation had duplicated code in a bunch of places, and
it interspersed spool-management with resource management. The new
implementation should make it easier to add new resource types and
maintain the spooling code.
Closing ticket 21651.
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
This patch adds a new timer that is executed when it is time to expire
our current set of old onion keys. Because of proposal #274 this can no
longer be assumed to be at the same time we rotate our onion keys since
they will be updated less frequently.
See: https://bugs.torproject.org/21641
We could use one of these for holding "junk" descriptors and
unparseable things -- but we'll _need_ it for having cached
consensuses and diffs between them.
Windows doesn't let you check the socket error for a socket with
WSAGetLastError() and getsockopt(SO_ERROR). But
getsockopt(SO_ERROR) clears the error on the socket, so you can't
call it more than once per error.
When we introduced recv_ni to help drain alert sockets, back in
0.2.6.3-alpha, we had the failure path for recv_ni call getsockopt()
twice, though: once to check for EINTR and one to check for EAGAIN.
Of course, we never got the eagain, so we treated it as an error,
and warned about: "No error".
The fix here is to have these functions return -errno on failure.
Fixes bug 21540; bugfix on 0.2.6.3-alpha.
The 64-bit load and store code was generating pretty bad output with
my compiler, so I extracted the code from csiphash and used that instead.
Close ticket 21737
This patch removes the `tor_fgets()` wrapper around `fgets(3)` since it
is no longer needed. The function was created due to inconsistency
between the returned values of `fgets(3)` on different versions of Unix
when using `fgets(3)` on non-blocking file descriptors, but with the
recent changes in bug #21654 we switch from unbuffered to direct I/O on
non-blocking file descriptors in our utility module.
We continue to use `fgets(3)` directly in the geoip and dirserv module
since this usage is considered safe.
This patch also removes the test-case that was created to detect
differences in the implementation of `fgets(3)` as well as the changes
file since these changes was not included in any releases yet.
See: https://bugs.torproject.org/21654
This patch removes the buffered I/O stream usage in process_handle_t and
its related utility functions. This simplifies the code and avoids racy
code where we used buffered I/O on non-blocking file descriptors.
See: https://bugs.torproject.org/21654
