We currently assume that the only way for Tor to listen on ports in the
privileged port range (1 to 1023), on Linux, is if we are granted the
NET_BIND_SERVICE capability. Today on Linux, it's possible to specify
the beginning of the unprivileged port range using a sysctl
configuration option. Docker (and thus the CI service Tor uses) recently
changed this sysctl value to 0, which causes our tests to fail as they
assume that we should NOT be able to bind to a privileged port *without*
the NET_BIND_SERVICE capability.
In this patch, we read the value of the sysctl value via the /proc/sys/
filesystem iff it's present, otherwise we assume the default
unprivileged port range begins at port 1024.
See: tor#40275
This code is based directly on the specification, without looking at
the reference implementation or the implementation in Arti.
Nonetheless, it is now passing with the test vectors generated by
the reference implementation.
When a directory request fails, we flag the relay as non Running so we
don't use it anymore.
This can be problematic with onion services because there are cases
where a tor instance could have a lot of services, ephemeral ones, and
keeps failing to upload descriptors, let say due to a bad network, and
thus flag a lot of nodes as non Running which then in turn can not be
used for circuit building.
This commit makes it that we never flag nodes as non Running on a onion
service directory request (upload or fetch) failure as to keep the
hashring intact and not affect other parts of tor.
Fortunately, the onion service hashring is _not_ selected by looking at
the Running flag but since we do a 3-hop circuit to the HSDir, other
services on the same instance can influence each other by removing nodes
from the consensus for path selection.
This was made apparent with a small network that ran out of nodes to
used due to rapid succession of onion services uploading and failing.
See #40434 for details.
Fixes#40434
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fixes bug 40078.
As reported by hdevalence our batch verification logic can cause an assert
crash.
The assert happens because when the batch verification of ed25519-donna fails,
the code in `ed25519_checksig_batch()` falls back to doing a single
verification for each signature.
The crash occurs because batch verification failed, but then all signatures
individually verified just fine.
That's because batch verification and single verification use a different
equation which means that there are sigs that can pass single verification
but fail batch verification.
Fixing this would require modding ed25519-donna which is not in scope for
this ticket, and will be soon deprecated in favor of arti and
ed25519-dalek, so my branch instead removes batch verification.
Now the circuits look like this:
client rend: C -> G -> L2 -> Rend
client intro: C -> G -> L2 -> M -> Intro
client hsdir: C -> G -> L2 -> M -> HSDir
service rend: C -> G -> L2 -> M -> Rend
service intro: C -> G -> L2 -> M -> Intro
service hsdir: C -> G -> L2 -> M -> HSDir
Bridge operators should generally publish their descriptors except for
rare cases, and instead use the BridgeDistribution none setting to
prevent their bridge from being distributed by BridgeDB.
Continue having a tor_gmtime_impl() unit test so that we can detect
any problems in our replacement function; add a new test function to
make sure that gmtime<->timegm are a round-trip on now-ish times.
This is a fix for bug #40383, wherein we ran into trouble because
tor_timegm() does not believe that time_t should include a count of
leap seconds, but FreeBSD's gmtime believes that it should. This
disagreement meant that for a certain amount of time each day,
instead of calculating the most recent midnight, our voting-schedule
functions would calculate the second-most-recent midnight, and lead
to an assertion failure.
I am calling this a bugfix on 0.2.0.3-alpha when we first started
calculating our voting schedule in this way.
My clang doesn't like it when we write code like this:
char *list[] = {
"abc",
"def",
"ghi"
"jkl"
}
It wonders whether we meant to put a comma between "ghi" and "jkl"
or not, and gives a warning.
To suppress this warning (since in this case, we did mean to omit
the comma), we just wrap the two strings in parentheses.
Closes#40426; bugfix on 0.4.0.4-rc.
We already did this in a couple of places, but there are more that
we didn't get. This is necessary for systems with versions of
NSS that don't do their prototypes properly.
Fixes#40409; bugfix on 0.3.5.1-alpha.
This patch enables the deterministic RNG for address set tests,
including the tests which uses address set indirectly via the nodelist
API.
This should prevent random test failures in the highly unlikely case of
a false positive which was seen in tor#40419.
See: tpo/core/tor#40419.
This issue was reported by Jann Horn part of Google's Project Zero.
Jann's one-sentence summary: entry/middle relays can spoof RELAY_END cells on
half-closed streams, which can lead to stream confusion between OP and
exit.
Fixes#40389
Previously, we would detect errors from a missing RNG
implementation, but not failures from the RNG code itself.
Fortunately, it appears those failures do not happen in practice
when Tor is using OpenSSL's default RNG implementation. Fixes bug
40390; bugfix on 0.2.8.1-alpha. This issue is also tracked as
TROVE-2021-004. Reported by Jann Horn at Google's Project Zero.
Without this message getting logged at 'WARN', it's hard to
contextualize the messages we get about compression bombs, so this
message should fix#40175.
I'm rate-limiting this, however, since it _could_ get spammy if
somebody on the network starts acting up. (Right now it should be
very quiet; I've asked Sebastian to check it, and he says that he
doesn't hit this message in practice.)
Closes#40175.
Cached_dir_t is a somewhat "legacy" kind of storage when used for
consensus documents, and it appears that there are cases when
changing our settings causes us to stop updating those entries.
This can cause trouble, as @arma found out in #40375, where he
changed his settings around, and consensus diff application got
messed up: consensus diffs were being _requested_ based on the
latest consensus, but were being (incorrectly) applied to a
consensus that was no longer the latest one.
This patch is a minimal fix for backporting purposes: it has Tor do
the same search when applying consensus diffs as we use to request
them. This should be sufficient for correct behavior.
There's a similar case in GETINFO handling; I've fixed that too.
Fixes#40375; bugfix on 0.3.1.1-alpha.
When seccomp sandbox is active, SAVECONF failed because it was not
able to save the backup files for torrc. This commit simplifies
the implementation of SAVECONF and sandbox by making it keep only
one backup of the configuration file.
Current counters are reset every heartbeat. This commit adds two
counters for the assigned and dropped onionskins that are not reset so
they can be exported onto the MetricsPort.
Closes#40387
Signed-off-by: David Goulet <dgoulet@torproject.org>
The connection type for the listener part was missing from the "is
connection a listener" function.
This lead to our periodic event that retries our listeners to keep
trying to bind() again on an already opened MetricsPort.
Closes#40370
Signed-off-by: David Goulet <dgoulet@torproject.org>
This resulted in the labels not being surrounded by double quotes and
thus Prometheus not liking it.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Emit on the MetricsPort all the DNS statistics we have that is the total
number of queries seen and errors per record type.
Related to #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
We now keep track of all errors and total number of request seen. This
is so we can expose those values to the MetricsPort to help Exit
operators monitor the DNS requests and failures.
Related to #40367.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This emits two events (read and write) of the total number that the
global connection limit was reached.
Related to #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
With this commit, a relay will emit metrics that give the total number
of sockets and total number of opened sockets.
Related to #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
With this commit, a relay now emits metrics event on the MetricsPort
related to how many onionskins were handled (processed or dropped) for
each handshake type.
Related to #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
With this commit, a relay now emits metrics event on the MetricsPort
related to the OOM invocation for:
- DNS cache
- GeoIP database
- Cell queues
- HSDir caches
Everytime the OOM is invoked, the number of bytes is added to the
metrics counter for that specific type of invocation.
Related to #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
The basic functions for the relay subsystem to expose metrics onto the
MetricsPort.
Part of #40367
Signed-off-by: David Goulet <dgoulet@torproject.org>
It is a common function that a lot of subsystem can use which is to
format a label so move it out of the HS subsystem into the more generic
metrics library.
Signed-off-by: David Goulet <dgoulet@torproject.org>
An entry can have multiple labels but only print once the comments at
the first one. This follows the Promtheus best practices.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This change permits the newfstatat() system call, and fixes issues
40382 (and 40381).
This isn't a free change. From the commit:
// Libc 2.33 uses this syscall to implement both fstat() and stat().
//
// The trouble is that to implement fstat(fd, &st), it calls:
// newfstatat(fs, "", &st, AT_EMPTY_PATH)
// We can't detect this usage in particular, because "" is a pointer
// we don't control. And we can't just look for AT_EMPTY_PATH, since
// AT_EMPTY_PATH only has effect when the path string is empty.
//
// So our only solution seems to be allowing all fstatat calls, which
// means that an attacker can stat() anything on the filesystem. That's
// not a great solution, but I can't find a better one.
As of GCC 11.1.1, the compiler warns us about code like this:
if (a)
b;
c;
and that's a good thing: we wouldn't want to "goto fail". But we
had an instance if this in circuituse.c, which was making our
compilation sad.
Fixes bug 40380; bugfix on 0.3.0.1-alpha.
Turns out that passing client authorization keys to ADD_ONION for v3 was
not working because we were not setting the "is_client_auth_enabled"
flag to true once the clients were configured. This lead to the
descriptor being encoded without the clients.
This patch removes that flag and instead adds an inline function that
can be used to check if a given service has client authorization
enabled.
This will be much less error prone of needing to keep in sync the client
list and a flag instead.
Fixes#40378
Signed-off-by: David Goulet <dgoulet@torproject.org>
This function has been a no-op since Libevent 2.0.4-alpha, when
libevent got an arc4random() implementation. Libevent has finally
removed it, which will break our compilation unless we stop calling
it. (This is currently breaking compilation in OSS-fuzz.)
Closes#40371.
This is related to ticket #40360 which found this problem when a Bridge entry
with a transport name (let say obfs4) is set without a fingerprint:
Bridge obfs4 <IP>:<PORT> cert=<...> iat-mode=0
(Notice, no fingerprint between PORT and "cert=")
Problem: commit 09c6d03246 added a check in
get_sampled_guard_for_bridge() that would return NULL if the selected bridge
did not have a valid transport name (that is the Bridge transport name that
corresponds to a ClientTransportPlugin).
Unfortuantely, this function is also used when selecting our eligible guards
which is done *before* the transport list is populated and so the added check
for the bridge<->transport name is querying an empty list of transports
resulting in always returning NULL.
For completion, the logic is: Pick eligible guards (use bridge(s) if need be)
then for those, initiate a connection to the pluggable transport proxy and
then populate the transport list once we've connected.
Back to get_sampled_guard_for_bridge(). As said earlier, it is used when
selecting our eligible guards in a way that prevents us from selecting
duplicates. In other words, if that function returns non-NULL, the selection
continues considering the bridge was sampled before. But if it returns NULL,
the relay is added to the eligible list.
This bug made it that our eligible guard list was populated with the *same*
bridge 3 times like so (remember no fingerprint):
[info] entry_guards_update_primary(): Primary entry guards have changed. New primary guard list is:
[info] entry_guards_update_primary(): 1/3: [bridge] ($0000000000000000000000000000000000000000)
[info] entry_guards_update_primary(): 2/3: [bridge] ($0000000000000000000000000000000000000000)
[info] entry_guards_update_primary(): 3/3: [bridge] ($0000000000000000000000000000000000000000)
When tor starts, it will find the bridge fingerprint by connecting to it and
will then update the primary guard list by calling
entry_guard_learned_bridge_identity() which then goes and update only 1 single
entry resulting in this list:
[debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($<FINGERPRINT>) is still listed.
[debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($0000000000000000000000000000000000000000) is still listed.
[debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($0000000000000000000000000000000000000000) is still listed.
And here lies the problem, now tor is stuck attempting to wait for a valid
descriptor for at least 2 guards where the second one is a bunch of zeroes and
thus tor will never fully bootstraps:
[info] I learned some more directory information, but not enough to build a
circuit: We're missing descriptors for 1/2 of our primary entry guards
(total microdescriptors: 6671/6703). That's ok. We will try to fetch missing
descriptors soon.
Now, why passing the fingerprint then works? This is because the list of
guards contains 3 times the same bridge but they all have a fingerprint and so
the descriptor can be found and tor can bootstraps.
The solution here is to entirely remove the transport name check in
get_sampled_guard_for_bridge() since the transport_list is empty at that
point. That way, the eligible guard list only gets 1 entry, the bridge, and
can then go on to bootstrap properly.
It is OK to do so since when launching a bridge descriptor fetch, we validate
that the bridge transport name is OK and thus avoid connecting to a bridge
without a ClientTransportPlugin. If we wanted to keep the check in place, we
would need to populate the transport_list much earlier and this would require
a much bigger refactoring.
Fixes#40360
Signed-off-by: David Goulet <dgoulet@torproject.org>
When seccomp sandbox is active, SAVECONF failed because it was not
able to save the backup files for torrc. This commit simplifies
the implementation of SAVECONF and sandbox by making it keep only
one backup of the configuration file.
On Linux systems, glob automatically ignores the errors ENOENT and
ENOTDIR because they are expected during glob expansion. But BSD
systems do not ignore these, resulting in glob failing when globs
expand to invalid paths. This is fixed by adding a custom error
handler that ignores only these two errors and removing the
GLOB_ERR flag as it makes glob fail even if the error handler
ignores the error and is unnecessary as the error handler will
make glob fail on all other errors anyway.
Fortunately, our tor_free() is setting the variable to NULL after so we were
in a situation where NULL was always used instead of the transport name.
This first appeared in 894ff2dc84 and results in
basically no bridge with a transport being able to use DoS defenses.
Fixes#40345
Signed-off-by: David Goulet <dgoulet@torproject.org>
We use it in router.c, where chunks are joined with "", not with
NL... so leaving off the terminating NL will lead to an unparseable
extrainfo.
Found by toralf. Bug not in any released Tor.
```
src/feature/stats/rephist.c: In function ‘overload_happened_recently’:
src/feature/stats/rephist.c:215:21: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (overload_time > approx_time() - 3600 * n_hours) {
```
from https://gitlab.torproject.org/tpo/core/tor/-/issues/40341#note_2729364
Clients now check whether their streams are attempting to re-enter
the Tor network (i.e. to send Tor traffic over Tor), and they close
them preemptively if they think exit relays will refuse them.
See bug 2667 for details. Resolves ticket 40271.
- Implement overload statistics structure.
- Implement function that keeps track of overload statistics.
- Implement function that writes overload statistics to descriptor.
- Unittest for the whole logic.
This option changes the time for which a bandwidth measurement period
must have been in progress before we include it when reporting our
observed bandwidth in our descriptors. Without this option, we only
consider a time period towards our maximum if it has been running
for a full day. Obviously, that's unacceptable for testing
networks, where we'd like to get results as soon as possible.
For non-testing networks, I've put a (somewhat arbitrary) 2-hour
minimum on the option, since there are traffic analysis concerns
with immediate reporting here.
Closes#40337.
We were looking for the first instance of "directory-signature "
when instead the correct behavior is to look for the first instance
of "directory-signature " at the start of a line.
Unfortunately, this can be exploited as to crash authorities while
they're voting.
Fixes#40316; bugfix on 0.2.2.4-alpha. This is TROVE-2021-002,
also tracked as CVE-2021-28090.
It was made to convert Maxmind's "mmdb" files into the older format
that we used. But now thanks to IPFire Location, we don't have to
touch Maxmind formats any more. (See ticket #40224.)
When reloading a service, we can re-register a service and thus end up again
in the metrics store initialization code path which is fine. No need to BUG()
anymore.
Fixes#40334
Signed-off-by: David Goulet <dgoulet@torproject.org>
Use find_str_at_start_of_line(), not strstr() here: we don't want
to match "MemTotal: " if it appears in the middle of a line.
Fixes#40315; bugfix on 0.2.5.4-alpha.
The directory_fetches_from_authorities() is used to know if a client or relay
should fetch data from an authority early in the boot process.
We had a condition in that function that made a relay trigger that fetch if it
didn't know its address (so we can learn it). However, when this is called,
the address discovery has not been done yet so it would always return true for
a relay.
Furthermore, it would always trigger a log notice that the IPv4 couldn't be
found which was inevitable because the address discovery process has not been
done yet (done when building our first descriptor).
It is also important to point out that starting in 0.4.5.1-alpha, asking an
authority for an address is done during address discovery time using a one-hop
circuit thus independent from the relay deciding to fetch or not documents
from an authority.
Small fix also is to reverse the "IPv(4|6)Only" flag in the notice so that if
we can't find IPv6 it would output to use IPv4Only.
Fixes#40300
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fix a bug introduced in 94b56eaa75 which
overwrite the connection message line.
Furthermore, improve how we generate that line by using a smartlist and change
the format so it is clearer of what is being rejected/detected and, if
applicable, which option is disabled thus yielding no stats.
Closes#40308
Signed-off-by: David Goulet <dgoulet@torproject.org>
No behavior change except for logging. This is so the connection related
statistics are in the right object.
Related to #40253
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a new detection type which is that a relay can now control the rate of
client connections from a single address.
The mechanism is pretty simple, if the rate/burst is reached, the address is
marked for a period of time and any connection from that address is denied.
Closes#40253
Signed-off-by: David Goulet <dgoulet@torproject.org>
When trying to find our address to publish, we would log notice if we couldn't
find it from the cache but then we would look at the suggested cache (which
contains the address from the authorities) in which we might actually have the
address.
Thus that log notice was misplaced. Move it down after the suggested address
cache lookup.
Closes#40300
Signed-off-by: David Goulet <dgoulet@torproject.org>
Relay will always publish 0 as DirPort value in their descriptor from now on
except authorities.
Related to #40282
Signed-off-by: David Goulet <dgoulet@torproject.org>
Regular relays are about to get their DirPort removed so that reachability
test is not useful anymore
Authorities will still use the DirPort but because network reentry towards
their DirPort is now denied network wide, this test is not useful anymore and
so it should simply be considered reachable at all time.
Part of #40282
Signed-off-by: David Goulet <dgoulet@torproject.org>
This bug made the pipeline fail. It basically tries to access a service we just
freed because it's still on the service list.
It only occurs about once every 10 tests and it looks like this:
$ ./src/test/test hs_control/hs_control_add_onion_helper_add_service
hs_control/hs_control_add_onion_helper_add_service: [forking] =================================================================
==354311==ERROR: AddressSanitizer: heap-use-after-free on address 0x613000000940 at pc 0x55a159251b03 bp 0x7ffc6abb5b30 sp 0x7ffc6abb5b28
READ of size 8 at 0x613000000940 thread T0
^[[A
#0 0x55a159251b02 in hs_service_ht_HT_FIND_P_ src/feature/hs/hs_service.c:153
#1 0x55a159251b02 in hs_service_ht_HT_FIND src/feature/hs/hs_service.c:153
#2 0x55a159251b02 in find_service src/feature/hs/hs_service.c:175
#3 0x55a159251c2c in register_service src/feature/hs/hs_service.c:188
#4 0x55a159262379 in hs_service_add_ephemeral src/feature/hs/hs_service.c:3811
#5 0x55a158e865e6 in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:847
#6 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#7 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#8 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#9 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#10 0x55a158b1b1a4 in main src/test/testing_common.c:420
#11 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
#12 0x55a158b21f69 in _start (/home/f/Computers/tor/mytor/src/test/test+0x372f69)
0x613000000940 is located 64 bytes inside of 344-byte region [0x613000000900,0x613000000a58)
freed by thread T0 here:
#0 0x7f7f0774ab6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
#1 0x55a158e86508 in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:838
#2 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#3 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#4 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#5 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#6 0x55a158b1b1a4 in main src/test/testing_common.c:420
#7 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
previously allocated by thread T0 here:
#0 0x7f7f0774ae8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x55a15948b728 in tor_malloc_ src/lib/malloc/malloc.c:45
#2 0x55a15948b7c0 in tor_malloc_zero_ src/lib/malloc/malloc.c:71
#3 0x55a159261bb5 in hs_service_new src/feature/hs/hs_service.c:4290
#4 0x55a159261f49 in hs_service_add_ephemeral src/feature/hs/hs_service.c:3758
#5 0x55a158e8619f in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:832
#6 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#7 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#8 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#9 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#10 0x55a158b1b1a4 in main src/test/testing_common.c:420
#11 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
SUMMARY: AddressSanitizer: heap-use-after-free src/feature/hs/hs_service.c:153 in hs_service_ht_HT_FIND_P_
Shadow bytes around the buggy address:
0x0c267fff80d0: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c267fff80e0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
0x0c267fff80f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8110: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
=>0x0c267fff8120: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd
0x0c267fff8130: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c267fff8140: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
0x0c267fff8150: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
0x0c267fff8160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==354311==ABORTING
[Lost connection!]
[hs_control_add_onion_helper_add_service FAILED]
1/1 TESTS FAILED. (0 skipped)
