Instead of replacing connection_t.{addr,port} with a canonical
orport, and tracking the truth in real_addr, we now leave
connection_t.addr alone, and put the canonical address in
canonical_orport.
Closes#40042Closes#33898
(This is not fully general yet: we only pick randomly among
_supported_ addresses, and each extendinfo contains at most one IPv4
address and at most one IPv6 address, no matter what the extend cell
had.)
This change will help dual-stack relays do IPv6 reachability tests,
in theory, by having them sometimes do IPv4 connections and
sometimes do ipv6 connections.
Closes ticket 33220.
This fixes CID 1465291, which was a complaint that we never actually
checked the return value of this function. It turns out that this
function was failing, and it didn't matter, because it wasn't
necessary for this test.
This is an automated commit made with a python script.
After running the automated script, I had to hand-revert the cases where it
made the conversion functions call themselves.
Additionally, I had to edit a variable declaration in control_bootstrap.c so
that the result of a const cast could be put in a const field.
Only log the 'real_addr' when it is set to something.
Only log the ID when it is set.
When scrubbing the address, don't include a canonical address.
(There should never be a canonical address for a connection with no
ID set.)
Now that we've clarified that these functions only need to describe
the peer in a human-readable way, we can have them delegate to
connection_describe_peer().
This mode was only used in one place, and it caused a dangerous
mingling of functionality. The method is supposed to _describe_ the
peer's address, not give its actual address. We already had a
function to get the actual address.
* We no longer call this an optional method
* We document that it returns the real address, not a canonical one.
* We have it try harder if the real address hasn't been set yet.
This patch ensures that we strip "\r" characters on both Windows as well
as Unix when we read text files. This should prevent the issue where
some Tor state files have been moved from a Windows machine, and thus
contains CRLF line ending, to a Unix machine where only \n is needed.
We add a test-case to ensure that we handle this properly on all our
platforms.
See: https://bugs.torproject.org/tpo/core/tor/33781
Pass the IPv4 before the IPv6 like all our other interfaces.
Changes unreleased code related to #40043.
Closes#40045
Signed-off-by: David Goulet <dgoulet@torproject.org>
This changes a LOT of code but in the end, behavior is the same.
Unfortunately, many functions had to be changed to accomodate but in majority
of cases, to become simpler.
Functions are also removed specifically those that were there to convert an
IPv4 as a host format to a tor_addr_t. Those are not needed anymore.
The IPv4 address field has been standardized to "ipv4_addr", the ORPort to
"ipv4_orport" (currently IPv6 uses ipv6_orport) and DirPort to "ipv4_dirport".
This is related to Sponsor 55 work that adds IPv6 support for relays and this
work is needed in order to have a common interface between IPv4 and IPv6.
Closes#40043.
Signed-off-by: David Goulet <dgoulet@torproject.org>
These fields have a complicated history, some slightly complicated
behavior, and some definitely inadequate documentation. Before we
go fixing them up, let's document how they work now.
Fix on unreleased code.
Logical || in the BUG() made it that it would always trigger the BUG().
Fixes#40034
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fix on unreleased code.
The relay_new_address_suggestion() is called when a NETINFO cell is received
thus not only for relay or bridges.
Remove the BUG() that made sure only in server mode we could handle the
suggested address.
Fixes#40032
Signed-off-by: David Goulet <dgoulet@torproject.org>
We've done a lot to improve our internal APIs since we wrote this
code, and it shows. We can just use a buf_t to build up the
bandwidth lines, and save a bunch of stack fiddling.
Additionally, we can use a function to format a single line, and
thereby get rid of the cheezy pattern that does
for (i=0;i<n;++i) {
switch (i) {
...
}
...
}
Unclear but that somehow failed on Windows once (?) according to ticket #33768
but we are not seeing that failure.
Nevertheless, add a simple unit test.
Closes#33768
Signed-off-by: David Goulet <dgoulet@torproject.org>
This will make sure that we always properly initialize the cache by the exact
size all the time.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
When receiving an introduction NACK, the client either decides to close or
re-extend the circuit to another intro point.
In order to do this, the service descriptor needs to exists but it is possible
that it gets removed from the cache between the establishement of the
introduction circuit and the reception of the (N)ACK.
For that reason, the BUG(desc == NULL) is removed because it is a possible
normal use case. Tor recovers gracefully already.
Fixes#34087
Signed-off-by: David Goulet <dgoulet@torproject.org>
It now uses the 'goto err' pattern, instead of the fatal_unreached()
pattern. The latter pattern is usually used when there is a loop, but there is
no loop in this function so it can be simplified easily.
This commit modifies the behavior of `parse_extended_address` in such a way
that if it fails, it will always return a `BAD_HOSTNAME` value, which is then
used to return the 0xF6 extended error code. This way, in any case that is
not a valid v2 address, we return the 0xF6 error code, which is the expected
behavior.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Built in tracing should _not_ be run if it was not set on purpose. Warn as
loud as we can in order to inform the user that they are running a version
with tracing capabilities built in.
This commit also adds a subsys stub because utlimately the logging will happen
in the init phase but because the default log file is not set in the
sys_logging init function, the stub is not useful for now.
Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to disambiguate the subsystem and event_name identifiers in the
tor_trace() macro, add TR_SUBSYS() and TR_EV() which help to identify the
parameters of tor_trace() explicitly.
Signed-off-by: David Goulet <dgoulet@torproject.org>
LTTng tracepoint probe declaration is not really following a C standard that
coccinelle and checkSpace.pl likes.
Move everything to a .inc file and standardize the trace_probes_circuit.h
header to include that LTTng specific file if the instrumentation was enabled
at configure time.
Part of #32910
Signed-off-by: David Goulet <dgoulet@torproject.org>
For now, trace_probes_circuit.c only contains LTTng probes so build it only if
enabled within in the build system _and_ the code.
Also, ignore trace_probes_circuit.h for coccinelle parsing.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is the very first tracepoint in tor. It is in the circuit subsystem for
when a new circuit opens.
LTTng instrumentation requires lot more around a tracepoint than USDT thus
this commit only adds one tracepoint in order to outline a base to add more
tracepoints later.
The idea is that we separate subsystem into what LTTng defines as "providers"
so the circuit provider contains the tracepoint definitions for the circuit
subsystem.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
In the next commits, we'll add more tracing options for instrumentation and
specific tracer.
This rename follows a more meaningful naming standard. It also adds a catch
all "HAVE_TRACING" define that indicate in the code that we have tracing
enabled.
Part of #32910
Signed-off-by: David Goulet <dgoulet@torproject.org>
We do look at the address but with this we also look if the identity digest of
the relay suggesting us an address is a trusted source.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
This reverts commit bf2a399fc0.
Don't set by default the prefer IPv6 feature on client ports because it breaks
the torsocks use case. The SOCKS resolve command is lacking a mechanism to ask
for a specific address family (v4 or v6) thus prioritizing IPv6 when an IPv4
address is asked on the resolve SOCKS interface resulting in a failure.
Tor Browser explicitly set PreferIPv6 so this should not affect the majority
of our users.
Closes#33796
Signed-off-by: David Goulet <dgoulet@torproject.org>
If no Address statement are found in the configuration file, attempt to learn
our address by looking at the ORPort address if any. Specifying an address is
optional so if we can't find one, it is fine, we move on to the next discovery
mechanism.
Note that specifying a hostname on the ORPort is not yet supported at this
commit.
Closes#33236
Signed-off-by: David Goulet <dgoulet@torproject.org>
Attempt to learn our address from the NETINFO cell.
At this commit, the address won't be used in the descriptor if selected. Next
commit will make it happen.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
This behaves like router_new_address_suggestion() but differs in couple of
ways:
1. It takes a tor_addr_t instead of an address string and supports both
AF_INET and AF_INET6.
2. It does _not_ use the last_guessed_ip local cache and instead only relies
on the last resolved address cache in resolve_addr.c
It is not used at this commit. This function is made to process a suggested
address found in a NETINFO cell exactly like router_new_address_suggestion()
does with the address a directory suggests us.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
Rename the static function update_resolved_cache() to resolved_addr_set_last()
and make it public.
We are about to use it in order to record any suggested address from a NETINFO
cell.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
In the spirit of reducing technical debt. Move code that marks a channel as a
client into its own function and document it properly.
No behavior change, only code movement.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to process a NETINFO cell, the OR connection needs to go through a
series of validation else we don't process the cell.
Move those into its own function in and improve documentation.
This is an attempt at reducing technical debt of the rather large and
complicated channel_tls_process_netinfo_cell() function.
Related to #40022
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch changes our bits-to-bytes conversion logic in the NSS
implementation of `tor_tls_cert_matches_key()` from using (x >> 3) to
((x + 7) >> 3) since DER bit-strings are allowed to contain a number of
bits that is not a multiple of 8.
Additionally, we add a comment on why we cannot use the
`DER_ConvertBitString()` macro from NSS, as we would potentially apply
the bits-to-bytes conversion logic twice, which would lead to an
insignificant amount of bytes being compared in
`SECITEM_ItemsAreEqual()` and thus turn the logic into being a
prefix match instead of a full match.
The `DER_ConvertBitString()` macro is defined in NSS as:
/*
** Macro to convert der decoded bit string into a decoded octet
** string. All it needs to do is fiddle with the length code.
*/
#define DER_ConvertBitString(item) \
{ \
(item)->len = ((item)->len + 7) >> 3; \
}
Thanks to Taylor Yu for spotting this problem.
This patch is part of the fix for TROVE-2020-001.
See: https://bugs.torproject.org/33119
We add constness to `peer_info_orig_len` and `cert_info_orig_len` in
`tor_tls_cert_matches_key` to ensure that we don't accidentally alter
the variables.
This patch is part of the fix for TROVE-2020-001.
See: https://bugs.torproject.org/33119
This patch fixes an out-of-bound memory read in
`tor_tls_cert_matches_key()` when Tor is compiled to use Mozilla's NSS
instead of OpenSSL.
The NSS library stores some length fields in bits instead of bytes, but
the comparison function found in `SECITEM_ItemsAreEqual()` needs the
length to be encoded in bytes. This means that for a 140-byte,
DER-encoded, SubjectPublicKeyInfo struct (with a 1024-bit RSA public key
in it), we would ask `SECITEM_ItemsAreEqual()` to compare the first 1120
bytes instead of 140 (140bytes * 8bits = 1120bits).
This patch fixes the issue by converting from bits to bytes before
calling `SECITEM_ItemsAreEqual()` and convert the `len`-fields back to
bits before we leave the function.
This patch is part of the fix for TROVE-2020-001.
See: https://bugs.torproject.org/33119
This patch lifts the `tor_tls_cert_matches_key()` tests out of the
OpenSSL specific TLS test suite and moves it into the generic TLS test
suite that is executed for both OpenSSL and NSS.
This patch is largely a code movement, but we had to rewrite parts of
the test to avoid using OpenSSL specific data-types (such as `X509 *`)
and replace it with the generic Tor abstraction type
(`tor_x509_cert_impl_t *`).
This patch is part of the fix for TROVE-2020-001.
See: https://bugs.torproject.org/33119
If at least one service is configured as a version 2, a log warning is emitted
once and only once.
Closes#40003
Signed-off-by: David Goulet <dgoulet@torproject.org>
We don't need to log that we're about to look for a channel for a
given extend_info_t, since we're either going to log that we're
launching one (at info), or that we're using an existing one (at
debug).
In practice, there will be at most one ipv4 address and ipv6 address
for now, but this code is designed to not care which address is
which until forced to do so.
This patch does not yet actually create extend_info_t objects with
multiple addresses.
Closes#34069.
The find_my_address() function now prioritize the local interface over the
local hostname when guessing the IP address.
See proposal 312, section 3.2.1, general case:
https://gitweb.torproject.org/torspec.git/tree/proposals/312-relay-auto-ipv6-addr.txt#n359
The entire unit tests had to be refactored to make this possible. Instead of
hot patching it, it has been rewritten to cover all possible cases and the
test interface has been changed to accomodate both IPv4 and IPv6 in order for
them to be tested identically.
Closes#33238
Signed-off-by: David Goulet <dgoulet@torproject.org>
Now instead of saying "DONE, DONE" or "MISC, MISC" or "TLS_ERROR,
TLS_ERROR", we can finally give a nice sensible "TLS_ERROR,
wrong version number" which should help debug a great deal.
Closes ticket 32622.
