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>
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>
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.
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>
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>
It can be called with strings that should have been
length-delimited, but which in fact are not. This can cause a
CPU-DoS bug or, in a worse case, a crash.
Since this function isn't essential, the best solution for older
Tors is to just turn it off.
Fixes bug 40286; bugfix on 0.2.2.1-alpha when dump_desc() was
introduced.
Now that exit relays don't allow exit connections to directory authority
DirPorts, the follow-up step is to make directory authorities stop doing
DirPort reachability checks.
Fixes#40287
Signed-off-by: David Goulet <dgoulet@torproject.org>
Turns out, we forgot to add the METRICS connection type fo the finished
flushing handler.
Fixes#40295
Signed-off-by: David Goulet <dgoulet@torproject.org>
The comment of that specific unit test wanted 4 ORPorts but for some reasons
we tested for 3 which before the previous commit related to #40289, test would
pass but it was in fact wrong.
Now the code is correct and 4 was in fact correct expected number of ports.
Related to #40289
Signed-off-by: David Goulet <dgoulet@torproject.org>
We were just looking at the family which is not correct because it is possible
to have two explicit ORPort for the same family but different addresses. One
example is:
ORPort 127.0.0.1:9001 NoAdvertise
ORPort 1.2.3.4:9001 NoListen
Thus, this patch now ignores ports that have different addresses iff they are
both explicits. That is, if we have this example, also two different
addresses:
ORPort 9001
ORPort 127.0.0.1:9001 NoAdvertise
The first one is implicit and second one is explicit and thus we have to
consider them for removal which in this case would remove the "ORPort 9001" in
favor of the second port.
Fixes#40289
Signe-off-by: David Goulet <dgoulet@torproject.org>
