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>
This patch adds checks for expected log messages for failure cases of
different ill-formed ESTABLISH_INTRO cell's.
See: https://bugs.torproject.org/21266
In rend_consider_services_intro_points(), we had a possible interger underflow
which could lead to creating a very large number of intro points. We had a
safe guard against that *except* if the expiring_nodes list was not empty
which is realistic thing.
This commit removes the check on the expiring nodes length being zero. It's
not because we have an empty list of expiring nodes that we don't want to open
new IPs. Prior to this check, we remove invalid IP nodes from the main list of
a service so it should be the only thing to look at when deciding if we need
to create new IP(s) or not.
Partially fixes#21302.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Determining if OpenSSL structures are opaque now uses an autoconf check
instead of comparing the version number. Some definitions have been
moved to their own check as assumptions which were true for OpenSSL
with opaque structures did not hold for LibreSSL. Closes ticket 21359.
This disregards anything smaller than an IPv6 /64, and rejects ports that
are rejected on an IPv6 /16 or larger.
Adjust existing unit tests, and add more to cover exceptional cases.
No IPv4 behaviour changes.
Fixes bug 21357
This interim fix results in too many IPv6 rejections.
No behaviour change for IPv4 counts, except for overflow fixes that
would require 4 billion redundant 0.0.0.0/0 policy entries to trigger.
Part of 21357
Stop modifying the value of our torrc option HiddenServiceStatistics just
because we're not a bridge or relay. This bug was causing Tor Browser users to
write "HiddenServiceStatistics 0" in their torrc files as if they had chosen
to change the config.
Fixes#21150
Signed-off-by: David Goulet <dgoulet@torproject.org>
Since we can call this function more than once before we update all
the confirmed_idx fields, we can't rely on all the relays having an
accurate confirmed_idx.
Fixes bug 21129; bugfix on 0.3.0.1-alpha
We need to call it before nt_service_parse_options(), since
nt_service_parse_options() can call back into nt_service_main(),
which calls do_main_loop().
Fixes bug 21356; bugfix on 0.2.9.1-alpha.
In addition to not wanting to build circuits until we can see most
of the paths in the network, and in addition to not wanting to build
circuits until we have a consensus ... we shouldn't build circuits
till all of our (in-use) primary guards have descriptors that we can
use for them.
This is another bug 21242 fix.
Actually, it's _fine_ to use a descriptorless guard for fetching
directory info -- we just shouldn't use it when building circuits.
Fortunately, we already have a "usage" flag that we can use here.
Partial fix for bug 21242.
This relates to the 21242 fix -- entry_guard_pick_for_circuit()
should never yield nodes without descriptors when the node is going
to be used for traffic, since we won't be able to extend through
them.
This assertion triggered in the (error) case where we got a result
from guards_choose_guard() without a descriptor. That's not
supposed to be possible, but it's not worth crashing over.
I broke "GETCONF *Port" in 20956, when I made SocksPort a
subordinate option of the virtual option SocksPortLines, so that I
could make SocksPort and __SocksPort provide qthe same
functionality. The problem was that you can't pass a subordinate
option to GETCONF.
So, this patch fixes that by letting you fetch subordinate options.
It won't always be meaningful to consider these options
out-of-context, but that can be the controller-user's
responsibility to check.
Closes ticket 21300.
If there are no ephemeral or detached onion services, then
"GETINFO onions/current" or "GETINFO onions/detached" should
return an empty list instead of an error
If tor_mmap_file is called with a file which is larger than SIZE_MAX,
only a small part of the file will be memory-mapped due to integer
truncation.
This can only realistically happen on 32 bit architectures with large
file support.
If a hostname is supplied to tor-resolve which is too long, it will be
silently truncated, resulting in a different hostname lookup:
$ tor-resolve $(python -c 'print("google.com" + "m" * 256)')
If tor-resolve uses SOCKS5, the length is stored in an unsigned char,
which overflows in this case and leads to the hostname "google.com".
As this one is a valid hostname, it returns an address instead of giving
an error due to the invalid supplied hostname.
When marking for close a circuit, the reason value, a integer, was assigned to
a uint16_t converting any negative reasons (internal) to the wrong value. On
the HS side, this was causing the client to flag introduction points to be
unreachable as the internal reason was wrongfully converted to a positive
16bit value leading to flag 2 out of 3 intro points to be unreachable.
Fixes#20307 and partially fixes#21056
Signed-off-by: David Goulet <dgoulet@torproject.org>
- Also remove LCOV marks from blocks of code that can be reachable by tests
if we mock relay_send_command_from_edge().
Signed-off-by: David Goulet <dgoulet@torproject.org>
With the previous commit, we validate the circuit _before_ calling
rend_mid_introduce() which handles the INTRODUCE1 payload.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Adds a better semantic and it also follows the same interface for the
INTRODUCE1 API which is circuit_is_suitable_for_introduce1().
Signed-off-by: David Goulet <dgoulet@torproject.org>
That way, when we are parsing the options and LearnCircuitBuildTimeout is set
to 0, we don't assert trying to get the options list with get_options().
Fixes#21062
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch refactors duplicated code, to check if a given router
supports fetching the extra-info document, into a common macro called
SKIP_MISSING_TRUSTED_EXTRAINFO.
This patch generalizes the two functions
router_is_already_dir_fetching_rs and router_is_already_dir_fetching_ds
into a single function, router_is_already_dir_fetching_, by lifting the
passing of the IPv4 & IPv6 addresses and the directory port number to
the caller.
So far, the TTLs for both A and AAAA records were not initialised,
resulting in exit relays sending back the value 60 to Tor clients. This
also impacts exit relays' DNS cache -- the expiry time for all domains
is set to 60.
This fixes <https://bugs.torproject.org/19025>.
The server-side clipping now clamps to one of two values, both
for what to report, and how long to cache.
Additionally, we move some defines to dns.h, and give them better
names.
An operator couldn't set the number of introduction point below the default
value which is 3. With this commit, from 0 to the hardcoded maximum is now
allowed.
Closes#21033
Signed-off-by: David Goulet <dgoulet@torproject.org>
Our config code is checking correctly at DataDirectoryGroupReadable but then
when we initialize the keys, we ignored that option ending up at setting back
the DataDirectory to 0700 instead of 0750. Patch by "redfish".
Fixes#19953
Signed-off-by: David Goulet <dgoulet@torproject.org>
In the past, when we exhausted all guards in our sampled set, we just
waited there till we mark a guard for retry again (usually takes 10 mins
for a primary guard, 1 hour for a non-primary guard). This patch marks
all guards as maybe-reachable when we exhaust all guards (this can
happen when network is down for some time).
Let A = UseBridges
Let B = ClientUseIPv4
Then firewall_is_fascist_impl expands and simplifies to:
B || (!(A || ...) && A)
B || (!A && ... && A)
B || 0
B
The microdesc consensus does not contain any IPv6 addresses.
When a client has a microdesc consensus but no microdescriptor, make it
use the hard-coded IPv6 address for the node (if available).
(Hard-coded addresses can come from authorities, fallback directories,
or configured bridges.)
If there is no hard-coded address, log a BUG message, and fail the
connection attempt. (All existing code checks for a hard-coded address
before choosing a node address.)
Fixes 20996, fix on b167e82 from 19608 in 0.2.8.5-alpha.
It is no longer possible for the IPv6 preference options to differ from the
IPv6 usage: preferring IPv6 implies possibly using IPv6.
Also remove the corresponding unit test warning message checks.
(But keep the unit tests themselves - they now run without warnings.)
In order to help an HS operator knowing if the application configured behind
it is not working properly, add a log at warning level for the connection
refused or timeout case. This log will only be printed if a client connection
fails and is rate limited.
Closes#21019
Signed-off-by: David Goulet <dgoulet@torproject.org>
In 8a0ea3ee43 we added a
temp_service_list local variable to rend_config_services, but we
didn't add a corresponding "free" for it to all of the exit paths.
Fixes bug 20987; bugfix on 0.3.0.1-alpha.
This commit adds 3 unit tests which validates a wrong signature length, a
wrong authentication key length and a wrong MAC in the cell.
Closes#20992
Signed-off-by: David Goulet <dgoulet@torproject.org>
Add the "sr/current" and "sr/previous" keys for the GETINFO command in order
to get through the control port the shared random values from the consensus.
Closes#19925
Signed-off-by: David Goulet <dgoulet@torproject.org>
Some DNS NXDOMAIN hijackers hijack truly ridiculous domains, like
"invalid-stuff!!" or "1.2.3.4.5". This would provoke unit test
failures where we used addresses like that to force
tor_addr_lookup() to fail. The fix, for testing, is to mock
tor_addr_lookup() with a variant that always fails when it gets
a name with a !.
Fixes bugs 20862 and 20863.
These relays need to be contacted over their ORPorts using a begindir
connection, and relays try not to use begindir connections.
Fixes bug 20711; bugfix on 0.2.8.2-alpha.
Because <unset> makes more sense than AAAAAAAAAAAAAAAAAAA...
(I have indeed verified that ed25519_fmt() is only used for
logging. This patch also clarifies the intention that ed25519_fmt()
is only for logging.
Closes ticket 21037.
We switched these to be "if (1) " a while back, so we could keep
the indentation and avoid merge conflicts. But it's nice to clean
up from time to time.
Previously we were marking directory guards up in
..._process_inbuf(), but that's wrong: we call that function on
close as well as on success. Instead, we're marking the dirguard up
only after we parse the HTTP headers. Closes 20974.
The abort handler masks the exit status of the backtrace generator by
capturing the abort signal from the backtrace handler and exiting with
zero. Because the output of the backtrace generator is meant to be piped
to `bt_test.py`, its exit status is unimportant and is currently
ignored.
The abort handler calls `exit(3)` which is not asynchronous-signal-safe
and calling it in this context is undefined behavior [0].
Closes ticket 21026.
[0] https://www.securecoding.cert.org/confluence/x/34At
When marking for close a circuit, the reason value, a integer, was assigned to
a uint16_t converting any negative reasons (internal) to the wrong value. On
the HS side, this was causing the client to flag introduction points to be
unreachable as the internal reason was wrongfully converted to a positive
16bit value leading to flag 2 out of 3 intro points to be unreachable.
Fixes#20307 and partially fixes#21056
Signed-off-by: David Goulet <dgoulet@torproject.org>
First, this commit moves the code used to prune the service list when
reloading Tor (HUP signal for instance) to a function from
rend_config_services().
Second, fix bug #21054, improve the code by using the newly added
circuit_get_next_service_intro_circ() function instead of poking at the global
list directly and add _many_ more comments.
Fixes#21054.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This came up on #21035, where somebody tried to build on a linux
system with kernel headers including CLOCK_MONOTONIC_COARSE, then
run on a kernel that didn't support it.
I've adopted a belt-and-suspenders approach here: we detect failures
at initialization time, and we also detect (loudly) failures later on.
Fixes bug 21035; bugfix on 0.2.9.1-alpha when we started using
monotonic time.
This helps protect against bugs where any part of a buf_t's memory
is passed to a function that expects a NUL-terminated input.
It also closes TROVE-2016-10-001 (aka bug 20384).
Replace the 81 remaining fallbacks of the 100 originally introduced
in Tor 0.2.8.3-alpha in March 2016, with a list of 177 fallbacks
(123 new, 54 existing, 27 removed) generated in December 2016.
Resolves ticket 20170.
In get_token(), we could read one byte past the end of the
region. This is only a big problem in the case where the region
itself is (a) potentially hostile, and (b) not explicitly
nul-terminated.
This patch fixes the underlying bug, and also makes sure that the
one remaining case of not-NUL-terminated potentially hostile data
gets NUL-terminated.
Fix for bug 21018, TROVE-2016-12-002, and CVE-2016-1254
They broke stem, and breaking application compatibility is usually a
bad idea.
This reverts commit 6e10130e18,
commit 78a13df158, and
commit 62f52a888a.
We might re-apply this later, if all the downstream tools can handle
it, and it turns out to be useful for some reason.
I got confused when I saw my Tor saying it was opening a file
that doesn't exist. It turns out it isn't opening it, it's just
calling open() on it and then moving on when it's not there.
Since both the client and service will use that data structure to store the
descriptor decoded data, only the public keys are common to both.
Fixes#20572.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The "sig_len" fields was moved below the "end_sig_fields" in the trunnel
specification so when signing the cell content, the function generating such a
cell needed to be adjust.
Closes#20991
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, we had NumEntryGuards kind of hardwired to 1. Now we
have the code (but not the configuarability) to choose randomly from
among the first N primary guards that would work, where N defaults
to 1.
Part of 20831 support for making NumEntryGuards work again.
Since we already had a separate function for getting the universe of
possible guards, all we had to do was tweak it to handle very the
GS_TYPE_RESTRICTED case.
asn found while testing that this function can be reached with
GUARD_STATE_COMPLETE circuits; I believe this happens when
cannibalization occurs.
The added complexity of handling one more state made it reasonable
to turn the main logic here into a switch statement.
Letting the maximum sample size grow proportionally to the number of
guards defeats its purpose to a certain extent. Noted by asn during
code review.
Fixes bug 20920; bug not in any released (or merged) version of Tor.
- Correctly maintain the previous guard selection in choose_guard_selection().
- Print bridge identifier instead of nothing in entry_guard_describe()._
If a complete circuit C2 doesn't obey the restrictions of C1, then
C2 cannot block C1.
The patch here is a little big-ish, since we can no longer look
through all the complete circuits and all the waiting circuits on a
single pass: we have to find the best waiting circuit first.
This is an important thing I hadn't considered when writing prop271:
sometimes you have to restrict what guard you use for a particular
circuit. Most frequently, that would be because you plan to use a
certain node as your exit, and so you can't choose that for your
guard.
This change means that the upgrade-waiting-circuits algorithm needs
a slight tweak too: circuit A cannot block circuit B from upgrading
if circuit B needs to follow a restriction that circuit A does not
follow.
I had been asking myself, "hey, doesn't the new code need to look at
this "info" parameter? The old code did!" But it turns out that the
old code hasn't, since 05f7336624.
So instead of "support this!" the comment now says "we can remove
this!"
George pointed out that (-1,0,1) for (never usable, maybe usable
later, usable right now) was a pretty rotten convention that made
the code harder to read.
Here we handle most (all?) of the remaining tasks, and fix some
bugs, in the prop271 bridge implementation.
* We record bridge identities as we learn them.
* We only call deprecated functions from bridges.c when the
deprecated guard algorithm is in use.
* We update any_bridge_descriptors_known() and
num_bridges_usable() to work correctly with the new backend
code. (Previously, they called into the guard selection logic.
* We update bridge directory fetches to work with the new
guard code.
* We remove some erroneous assertions where we assumed that we'd
never load a guard that wasn't for the current selection.
Also, we fix a couple of typos.
Still missing is functionality for picking bridges when we don't
know a descriptor for them yet, and functionality for learning a
bridge ID.
Everything else remains (basically) the same. Neat!
This includes:
* making bridge_info_t exposed but opaque
* allowing guards where we don't know an identity
* making it possible to learn the identity of a guard
* creating a guard that lacks a node_t
* remembering a guard's address and port.
* Looking up a guard by address and port.
* Only enforcing the rule that we need a live consensus to update
the "listed" status for guards when we are not using bridges.
This is safe, because no entry_guard_t ever outlives its
guard_selection_t.
I want this because now that multiple guard selections can be active
during one tor session, we should make sure that any information we
register about guards is with respect to the selection that they came
from.
Currently, this code doesn't actually have the contexts behave
differently, (except for the legacy context), but it does switch
back and forth between them nicely.
If a guard becomes primary as a result of confirming it, consider
the circuit through that guard as a primary circuit.
Also, note open questions on behavior when confirming nonprimary guards
Some of these will get torrc options to override them too; this
is just the mechanical conversion.
Also, add documentation for a couple of undocumented (but now used)
parameters.
Act I.
" But that I am forbid
To tell the secrets of my prison-house,
I could a tale unfold..."
Here's the bug: sometimes, rend_cache/store_v2_desc_as_client would
say:
"Dec 15 08:31:26.147 [warn] rend_cache_store_v2_desc_as_client():
Bug: Couldn't decode base32 [scrubbed] for descriptor id. (on Tor
0.3.0.0-alpha-dev 4098bfa260)"
When we merged ade5005853 back in 0.2.8.1-alpha, we added that
test: it mangles the hidden service ID for a hidden service, and
ensures that when the descriptor ID doesn't match the descriptor's
key, we don't store the descriptor.
How did it mangle the descriptor ID? By doing
desc_id_base32[0]++;
So, if the hidden service ID started with z or 7, we'd wind up with an
invalid base32 string, and get the warning. And if it started with
any other character, we wouldn't.
That there is part 1 of the bug: in 2/32 cases, we'd get a BUG
warning. But we wouldn't display it, since warnings weren't shown
from the unit tests.
Act II.
"Our indiscretion sometime serves us well,
When our deep plots do pall"
Part two: in 0.2.9.3-alpha, for part of #19999, we turned on BUG
warnings in the unit tests, so that we'd actually start seeing them.
At this point we also began to consider each BUG warning that made
it through the unit tests to be an actual bug. So before this
point, we wouldn't actually notice anything happening in those 2/32
cases.
So, at this point it was a nice random _visible_ bug.
Act III.
"Our thoughts are ours, their ends none of our own"
In acbb60cd63, which was part of my prop220 work, I
changed how RSA key generation worked in the unit tests. While
previously we'd use pre-made RSA keys in some cases, this change
made us use a set of pregenerated RSA keys for _all_ 1024 or 2048
keys, and to return them in a rotation when Tor tried to generate a
key.
And now we had the heisenbug: anything that affected the number of
pregenerated keys that we had yielded before reaching
rend_cache/store_v2_desc_as_client would make us return a different
key, which would give us a different base32 ID, which would make the
bug occur, or not. So as we added or removed test cases, the bug
might or might not happen.
So yeah. Don't mangle a base32 ID like that. Do it this way instead.
The new HS circuitmap API replaces old public functions as follows:
circuit_clear_rend_token -> hs_circuitmap_remove_circuit
circuit_get_rendezvous -> hs_circuitmap_get_rend_circ
circuit_get_intro_point -> hs_circuitmap_get_intro_circ_v2
circuit_set_rendezvous_cookie -> hs_circuitmap_register_rend_circ
circuit_set_intro_point_digest -> hs_circuitmap_register_intro_circ_v2
This commit also removes the old rendinfo code that is now unused.
It also fixes the broken rendinfo unittests.
The HS circuitmap is a hash table that maps introduction and rendezvous
tokens to specific circuits such that given a token it's easy to find
the corresponding circuit. It supports rend circuits and v2/v3 intro
circuits.
It will be used by the prop224 ESTABLISH_INTRO code to register and
lookup v3 introduction circuits.
The next commit after this removes the old code and fixes the unittests.
Please consult both commits while reviewing functionality differences
between the old and new code. Let me know if you want this rebased
differently :)
WRT architectural differences, this commit removes the rendinfo pointer
from or_circuit_t. It then adds an hs_token_t pointer and a hashtable
node for the HS circuitmap. IIUC, this adds another pointer to the
weight of or_circuit_t. Let me know if you don't like this, or if you
have suggestions on improving it.
Back when Roger had do do most of our testing on the moria host, we
needed a higher limit for the number of relays running on a single
IP address when that limit was shared with an authority. Nowadays,
the idea is pretty obsolete.
Also remove the router_addr_is_trusted_dir() function, which served
no other purpose.
Closes ticket 20960.
In c35fad2bde, merged in
0.2.4.7-alpha, we removed the code to parse v1 directory
objects. When we did so, we removed everything that could set the
CST_CHECK_AUTHORITY flag for check_signature_token().
So in this code, we remove the flag itself, the code to handle the
flag, and a function that only existed to handle the flag.
Circuit object wasn't freed correctly. Also, the cpath build state object
needed to be zeroed else we were freeing garbage pointers.
Closes#20936
Signed-off-by: David Goulet <dgoulet@torproject.org>
The signed_descriptor_move() was not releasing memory inside the destination
object before overwriting it with the source object. This commit adds a reset
function that free that memory inside a signed descriptor object and zero it.
Closes#20715.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This was breaking the build on debian precise, since it thought that
using a 'const int' to dimension an array made that array
variable-size, and made us not get protection.
Bug not in any released version of Tor.
I will insist that this one wasn't my fault.
"Variables won't. Constants aren't." -- Osborn's Law
If a node can prove its Ed25519 identity, don't consider connections
to it canonical unless they match both identities.
Includes link handshake changes needed to avoid crashing with bug
warnings, since the tests now reach more parts of the code.
Closes ticket 20355
(Only run the connection_or_group_set_badness_() function on groups
of channels that have the same RSA and Ed25519 identities.)
There's a possible opportunity here where we might want to set a
channel to "bad" if it has no ed25519 identity and some other
channel has some. Also there's an opportunity to add a warning if
we ever have an Ed mismatch on open connections with the same RSA
ID.
This function has never gotten testing for the case where an
identity had been set, and then got set to something else. Rather
than make it handle those cases, we forbid them.
If there is some horrible bug in our ed25519 link authentication
code that causes us to label every single ed25519-having node as
non-running, we'll be glad we had this. Otherwise we can remove it
later.
This patch makes two absolutely critical changes:
- If an ed25519 identity is not as expected when creating a channel,
we call that channel unsuccessful and close it.
- When a client creating a channel or an extend cell for a circuit, we
only include the ed25519 identity if we believe that the node on
the other side supports ed25519 link authentication (from
#15055). Otherwise we will insist on nodes without the right
link protocol authenticating themselves.
- When deciding to extend to another relay, we only upgrade the
extend to extend by ed25519 ID when we know the ed25519 ID _and_
we know that the other side can authenticate.
This patch also tells directory servers, when probing nodes, to
try to check their ed25519 identities too (if they can authenticate
by ed25519 identity).
Also, handle the case where we connect by RSA Id, and learn the
ED25519 ID for the node in doing so.
I need to be able to turn on Ed25519 support in client generation
of extend cells so I can test it, but leave it off-by-default until
enough clients support it for us to turn it on for a bunch at once.
This is part of #15056 / prop#220.
- forbid extending to the previous hop by Ed25519 ID.
- If we know the Ed25519 ID for the next hop and the client doesn't,
insist on the one from the consensus.
Right now, there's only a mechanism to look for a channel where the
RSA ID matches *and* the ED ID matches. We can add a separate map
later if we want.
They added clock_gettime(), but with tv_nsec as a long, whereas
tv_usec is a __darwin_suseconds_t (a.k.a. 'int'). Now, why would
they do that? Are they preparing for a world where there are more
than 2 billion nanoseconds per second? Are they planning for having
int be less than 32 bits again? Or are they just not paying
attention to the Darwin API?
Also, they forgot to mark clock_gettime() as Sierra-only, so even
if we fixed the issue here, we'd still be stick with portability
breakage like we were for 0.2.9.
So, just disable clock_gettime() on apple.
