Couple things happen in this commit. First, we do not re-queue a cell back in
the circuit queue if the write packed cell failed. Currently, it is close to
impossible to have it failed but just in case, the channel is mark as closed
and we move on.
The second thing is that the channel_write_packed_cell() always took ownership
of the cell whatever the outcome. This means, on success or failure, it needs
to free it.
It turns out that that we were using the wrong free function in one case and
not freeing it in an other possible code path. So, this commit makes sure we
only free it in one place that is at the very end of
channel_write_packed_cell() which is the top layer of the channel abstraction.
This makes also channel_tls_write_packed_cell_method() return a negative value
on error.
Two unit tests had to be fixed (quite trivial) due to a double free of the
packed cell in the test since now we do free it in all cases correctly.
Part of #23709
Signed-off-by: David Goulet <dgoulet@torproject.org>
Split hs_circuitmap_get_rend_circ_client_side(). One returns only established
circuits (hs_circuitmap_get_established_rend_circ_client_side()) and the other
returns all kinds of circuits.
Fixes#23459
Signed-off-by: Fernando Fernandez Mancera <ffernandezmancera@gmail.com>
Previously, circuit_stream_is_being_handled incorrectly reported
that (1) an exit port was "handled" by a circuit regardless of
whether the circuit was already isolated in some way, and
(2) that a stream could be "handled" by a circuit even if their
isolation settings were incompatible.
As a result of (1), in Tor Browser, circuit_get_unhandled_ports was
reporting that all ports were handled even though all non-internal
circuits had already been isolated by a SOCKS username+password.
Therefore, circuit_predict_and_launch_new was declining to launch
new exit circuits. Then, when the user visited a new site in Tor
Browser, a stream with new SOCKS credentials would be initiated,
and the stream would have to wait while a new circuit with those
credentials could be built. That wait was making the
time-to-first-byte longer than it needed to be.
Now, clean, not-yet-isolated circuit(s) will be automatically
launched ahead of time and be ready for use whenever a new stream
with new SOCKS credentials (or other isolation criteria) is
initiated.
Fixes bug 18859. Thanks to Nick Mathewson for improvements.
This makes sure that a non opened channel is never put back in the channel
pending list and that its state is consistent with what we expect that is
IDLE.
Test the fixes in #24502.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch adds a check for the return value of `hs_parse_address()` in
`hs_control_hspost_command()`. Since it should not be possible for
`hs_parse_address()` to fail in this context we wrap the error check
with the `BUG()` macro.
See: https://bugs.torproject.org/24543
This patch is a result of auditing all of our uses of
get_datadir_fname() and its kin, and dividing them into cache vs
keys vs other data.
The new get_keydir_fname() and get_cachedir_fname() functions don't
actually do anything new yet.
This introduces the test_hs_control.c file which at this commit contains basic
unit test for the HS_DESC event.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is removed for two reasons. First, HSDir accepts descriptor even though
they don't think they are in fact an HSDir. This is to avoid consensus desync
between client/service and directories.
Second, our malicious HSDir scanner uses the HSPOST command to post on all
relays in order to test them before they could become HSDir. We had to remove
that check from the tor code that the scanner uses.
Thus, this check should not be enforced by the control port for the above use
cases. It is also a bit more complex with v3 support for which not all HSDir
support it so basically irrelevant check.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is groundwork for the HSPOST control port command that needs a way in the
HS subsystem to upload a service descriptor to a specific HSDir.
To do so, we add a public function that takes a series of parameters including
a fully encoded descriptor and initiate a directory request to a specific
routerstatut_t object.
It is for now not used but should be, in future commit, by the HSPOST command.
This commit has no behavior change, only refactoring.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This makes the REPLICA= field optional for the control port event. A v2
service will always pass it and v3 is ignored.
Signed-off-by: David Goulet <dgoulet@torproject.org>
A new v3 specific function has been added named
control_event_hsv3_descriptor_failed().
The HS v3 subsystem now uses it.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This changes the control_event_hs_descriptor_requested() call to add the hsdir
index optional value. v2 passes NULL all the time.
This commit creates hs_control.{c|h} that contains wrappers for the HS
subsystem to interact with the control port subsystem.
The descriptor REQUESTED event is implemented following proposal 284 extension
for v3.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Make control_event_hs_descriptor_received() and
control_event_hs_descriptor_failed() v2 specific because they take a
rend_data_t object and v3 will need to pass a different object.
No behavior change.
Signed-off-by: David Goulet <dgoulet@torproject.org>
First, rename and make that function static because it is internal to
control.c and called by two HS_DESC events.
Second, make it take more basic parameters and thus not a rend_data_t object
so we can still use the function for v3 HS that doesn't use that object.
Third, move the descriptor ID lookup to the two specific events (yes little
code duplication there) because they get a rend_data_t object which won't be
the case for v3.
Finally, through this refactoring, change the pointer check to BUG() and
change some parameter names to reflect what they really are.
No behavior change at this commit.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a naming refactor mostly _except_ for a the events' function that take
a rend_data_t which will require much more refactoring.
No behavior change at this commit, cleanup and renaming stuff to not be only
v2 specific.
Signed-off-by: David Goulet <dgoulet@torproject.org>
When an intro circuit has closed, do not warn anymore when we can't find the
service. It is possible to hit that condition if the service is removed before
the circuits were fully closed. This happens in the case of deleting an
ephemeral service.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The functions are now used by the ADD_ONION/DEL_ONION control port command as
well. This commits makes them fully functionnal with hidden service v3.
Part of #20699
Signed-off-by: David Goulet <dgoulet@torproject.org>
Instead of using the cwd to specify the location of Cargo.toml, we
use the --manifest-path option to specify its location explicitly.
This works around the bug that isis diagnosed on our jenkins builds.
Making errno error log more useful for getrandom() call. Adding if statement to
make difference between ENOSYS and other errors.
Fixes#24500
Signed-off-by: Fernando Fernandez Mancera <ffernandezmancera@gmail.com>
First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for
close() because the HS subsystem needs to learn when an intro point is
actually not established anymore as soon as possible. There is a time window
between a close and a free.
Second, when we mark for close, we also remove it from the circuitmap because
between the close and the free, a service can launch an new circuit to that
same intro point and thus register it which only succeeds if the intro point
authentication key is not already in the map.
However, we still do a remove from the circuitmap in circuit_free() in order
to also cleanup the circuit if it wasn't marked for close prior to the free.
Fixes#23603
Signed-off-by: David Goulet <dgoulet@torproject.org>
The hs_service_intro_circ_has_closed() was removing intro point objects if too
many retries.
We shouldn't cleanup those objects in that function at all but rather let
cleanup_intro_points() do its job and clean it properly.
This was causing an issue in #23603.
Furthermore, this moves the logic of remembering failing intro points in the
cleanup_intro_points() function which should really be the only function to
know when to cleanup and thus when an introduction point should be remembered
as a failed one.
Fixes#23603
Signed-off-by: David Goulet <dgoulet@torproject.org>
In the KIST main loop, if the channel happens to be not opened, set its state
to IDLE so we can release it properly later on. Prior to this fix, the channel
was in PENDING state, removed from the channel pending list and then kept in
that state because it is not opened.
This bug was introduced in commit dcabf801e5 for
which we made the scheduler loop not consider unopened channel.
This has no consequences on tor except for an annoying but harmless BUG()
warning.
Fixes#24502
Signed-off-by: David Goulet <dgoulet@torproject.org>
Some platforms don't have good monotonic time support so don't warn when the
diff between the last run of the scheduler time and now is negative. The
scheduler recovers properly from this so no need to be noisy.
Fixes#23696
Signed-off-by: David Goulet <dgoulet@torproject.org>
When creating a routerstatus (vote) from a routerinfo (descriptor),
set the IPv6 address to the unspecified IPv6 address, and explicitly
initialise the port to zero.
Also clarify the documentation for the function.
Fixes bug 24488; bugfix on 0.2.4.1-alpha.
Fortunately, use_cached_ipv4_answers was already 0, so we wouldn't
actually use this info, but it's best not to have it.
Fixes bug 24050; bugfix on 0.2.6.3-alpha
TROVE-2017-12. Severity: Medium
When choosing a random node for a circuit, directly use our router
descriptor to exclude ourself instead of the one in the global
descriptor list. That list could be empty because tor could be
downloading them which could lead to not excluding ourself.
Closes#21534
TROVE-2017-12. Severity: Medium
Thankfully, tor will close any circuits that we try to extend to
ourselves so this is not problematic but annoying.
Part of #21534.
TROVE-2017-13. Severity: High.
In the unlikely case that a hidden service could be missing intro circuit(s),
that it didn't have enough directory information to open new circuits and that
an intro point was about to expire, a use-after-free is possible because of
the intro point object being both in the retry list and expiring list at the
same time.
The intro object would get freed after the circuit failed to open and then
access a second time when cleaned up from the expiring list.
Fixes#24313
Going from 4 hours to 24 hours in order to try reduce the efficiency of guard
discovery attacks.
Closes#23856
Signed-off-by: David Goulet <dgoulet@torproject.org>
The goal here is to replace our use of msec-based timestamps with
something less precise, but easier to calculate. We're doing this
because calculating lots of msec-based timestamps requires lots of
64/32 division operations, which can be inefficient on 32-bit
platforms.
We make sure that these stamps can be calculated using only the
coarse monotonic timer and 32-bit bitwise operations.
First, that test was broken from the previous commit because the
channel_queue_cell() has been removed. This now tests the
channel_process_cell() directly.
Second, it wasn't testing much except if the channel subsystem actually went
through the cell handler. This commit adds more checks on the state of a
channel going from open, receiving a cell and closing.
Third, this and the id_map unit test are working, not the others so they've
been marked as not working and future commit will improve and fix those.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This removed code that was either never reached or irrelevant after the
incoming/outgoing queue removal such as the "timestamp_drained".
Lots of things are also removed from channel.h that do not exists anymore or
not used.
Signed-off-by: David Goulet <dgoulet@torproject.org>
If the channel layer failed to write a cell from the circuit queue, requeue it
so it can be retried on the same channel later.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The channel_write_cell() and channel_write_var_cell() can't be possibly called
nor are used by tor. We only write on the connection outbuf packed cell coming
from the scheduler that takes them from the circuit queue.
This makes channel_write_packed_cell() the only usable function. It is
simplify and now returns a code value. The reason for this is that in the next
commit(s), we'll re-queue the cell onto the circuit queue if the write fails.
Finally, channel unit tests are being removed with this commit because they do
not match the new semantic. They will be re-written in future commits.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The channel subsystem was doing a whole lot to track and try to predict the
channel queue size but they are gone due to previous commit.
Signed-off-by: David Goulet <dgoulet@torproject.org>
For the rationale, see ticket #23709.
This is a pretty massive commit. Those queues were everywhere in channel.c and
it turns out that it was used by lots of dead code.
The channel subsystem *never* handles variable size cell (var_cell_t) or
unpacked cells (cell_t). The variable ones are only handled in channeltls and
outbound cells are always packed from the circuit queue so this commit removes
code related to variable and unpacked cells.
However, inbound cells are unpacked (cell_t), that is untouched and is handled
via channel_process_cell() function.
In order to make the commit compile, test have been modified but not passing
at this commit. Also, many tests have been removed but better improved ones
get added in future commits.
This commit also adds a XXX: which indicates that the handling process of
outbound cells isn't fully working. This as well is fixed in a future commit.
Finally, at this commit, more dead code remains, it will be cleanup in future
commits.
Fixes#23709
Signed-off-by: David Goulet <dgoulet@torproject.org>
This function is part of the tor fast path so this commit adds more
documentation to it as it is critical.
Signed-off-by: David Goulet <dgoulet@torproject.org>
append_cell_to_circuit_queue() had code disabled from commit
2a95f31716
This code is 4+ years old related to bug #9072 so if we ever want to revisit
it, lets inspect/revert this commit.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This applies the changes in 23524 to num_usable_bridges(), because it has
replaced any_bridge_descriptors_known().
The original changes file still applies.
Stop checking for bridge descriptors when we actually want to know if
any bridges are usable. This avoids potential bootstrapping issues.
Fixes bug 24367; bugfix on 0.2.0.3-alpha.
Stop stalling when bridges are changed at runtime. Stop stalling when
old bridge descriptors are cached, but they are not in use.
Fixes bug 24367; bugfix on 23347 in 0.3.2.1-alpha.
We used to check whether we have enough filtered guards (guard set when
torrc is applied) but that's not good enough, since that might be bad in
some cases where many guards are not reachable (might cause overblocking
and hence reacahbility issues).
We now check if we have enough reachable filtered guards before applying
md restrictions which should prevent overblocking.
Previously, if store_multiple() reported a partial success, we would
store all the handles it gave us as if they had succeeded. But it's
possible for the diff to be only partially successful -- for
example, if LZMA failed but the other compressors succeeded.
Fixes bug 24086; bugfix on 0.3.1.1-alpha.
Move it to hs_common.h and rename it "hs_service_add_ephemeral_status_t". It
will be shared between v2 and v3 services.
Part of #20699
Signed-off-by: David Goulet <dgoulet@torproject.org>
At this commit, the key handling and generation is supported for a v3 service
(ED25519-V3). However, the service creation is not yet implemented. This only
adds the interface and code to deal with the new ED25519-V3 key type.
Tests have been updated for RSA key type but nothing yet for ED25519-v3.
Part of #20699
Signed-off-by: David Goulet <dgoulet@torproject.org>
This will be used by the control port command "GETINFO
hs/service/desc/id/<ADDR>" which returns the encoded current descriptor for
the given onion address.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit adds hs_cache_lookup_encoded_as_client() function that returns the
encoded descriptor for a given service public key. This will be needed by the
"GETINFO hs/client/desc/id/<ADDR>" control port command.
Signed-off-by: David Goulet <dgoulet@torproject.org>
If we can't read a file because of an FS issue, we say "we can't
read that" and move on. But if we can't read it because it's empty,
because it has no labels, or because its labels are misformatted, we
should remove it.
Fixes bug 24099; bugfix on 0.3.1.1-alpha.
A circuit with purpose C_INTRODUCING means that its state is opened but the
INTRODUCE1 cell hasn't been sent yet. We shouldn't consider that circuit when
looking for timing out "building circuit". We have to wait on the rendezvous
circuit to be opened before sending that cell so the intro circuit needs to be
kept alive for at least that period of time.
This patch makes that the purpose C_INTRODUCING is ignored in the
circuit_expire_building() which means that we let the circuit idle timeout
take care of it if we end up never using it.
Fixes#23681
Signed-off-by: David Goulet <dgoulet@torproject.org>
This check makes it so we can reach "done" without setting "conn",
and so the "if (conn)" check will not be redundant, and so coverity
won't complain. Fixes CID 1422205. Not actually a bug.
There are three changes here:
* We need to allow epoll_pwait.
* We need to allow PF_NETLINK sockets to be opened with SOCK_CLOEXEC.
* We need to use openat() instead of open().
Note that this fix is not complete, since the openat() change is
turned off. The next commit will make the openat() change happen
when we're running glibc 2.26 or later.
Fix for 24315.
We don't want to allow general signals to be sent, but there's no
problem sending a kill(0) to probe whether a process is there.
Fixes bug 24198; bugfix on 0.2.5.1-alpha when the seccomp2 sandbox
was introduced.
When we close a connection via connection_close_immediately, we kill
its events immediately. But if it had been blocked on bandwidth
read/write, we could try to re-add its (nonexistent) events later
from connection_bucket_refill -- if we got to that callback before
we swept the marked connections.
Fixes bug 24167. Fortunately, this hasn't been a crash bug since we
introduced connection_check_event in 0.2.9.10, and backported it.
This is a bugfix on commit 89d422914a, I believe, which
appeared in Tor 0.1.0.1-rc.
Commit 56c5e282a7 suppressed that same log
statement in directory_info_has_arrived() for microdescriptors so do the same
for the descriptors. As the commit says, we already have the bootstrap
progress for this.
Fixes#23861
Signed-off-by: David Goulet <dgoulet@torproject.org>
evdns is allowed to give us unrecognized object types; it is allowed
to give us non-IPv4 answer types, and it is (even) allowed to give
us empty answers without an error.
Closes ticket 24097.
By convention, the torrc options that the user sets are
unchangeable. If we need to change them, we should be using a copy
that's stored in another field
To avoid trouble, I'm keeping DataDirectory as the name for the
field that the rest of Tor uses, and using DataDirectory_option for
the confparse-controlled field.
This commit also modernizes some older string handling code in the
DataDirectory normalization function.
Due to #23662 this can happen under natural causes and does not disturb
the functionality of the service. This is a simple 0.3.2 fix for now,
and we plan to fix this properly in 0.3.3.
Clients add rendezvous point IPv6 addresses to introduce cell link specifiers,
when the node has a valid IPv6 address.
Also check the node's IPv4 address is valid before adding any link specifiers.
Implements #23577.
This function -- a mock replacement used only for fuzzing -- would
have a buffer overflow if it got an RSA key whose modulus was under
20 bytes long.
Fortunately, Tor itself does not appear to have a bug here.
Fixes bug 24247; bugfix on 0.3.0.3-alpha when fuzzing was
introduced. Found by OSS-Fuzz; this is OSS-Fuzz issue 4177.
On failure to upload, the HS_DESC event would report "UPLOAD_FAILED" as the
Action but it should have reported "FAILED" according to the spec.
Fixes#24230
Signed-off-by: David Goulet <dgoulet@torproject.org>
DisableNetwork is a subset of net_is_disabled(), which is (now) a
subset of should_delay_dir_fetches().
Some of these changes are redundant with others higher or lower in
the call stack. The ones that I think are behavior-relevant are
listed in the changes file. I've also added comments in a few
places where the behavior is subtle.
Fixes bug 12062; bugfix on various versions.
Commit e67f4441eb introduced a safeguard against
using an uninitialized voting schedule object. However, the dirvote_act() code
was looking roughly at the same thing to know if it had to compute the timings
before voting with this condition:
if (!voting_schedule.voting_starts) {
...
dirvote_recalculate_timing(options, now);
}
The sr_init() function is called very early and goes through the safeguard
thus the voting schedule is always initilized before the first vote.
That first vote is a crucial one because we need to have our voting schedule
aligned to the "now" time we are about to use for voting. Then, the schedule
is updated when we publish our consensus or/and when we set a new consensus.
From that point on, we only want to update the voting schedule through that
code flow.
This "created_on_demand" is indicating that the timings have been recalculated
on demand by another subsystem so if it is flagged, we know that we need to
ignore its values before voting.
Fixes#24186
Signed-off-by: David Goulet <dgoulet@torproject.org>
When we have fewer than 15 descriptors to fetch, we will delay the
fetch for a little while. That's fine, if we can go ahead and build
circuits... but if not, it's a poor choice indeed.
Fixes bug 23985; bugfix on 0.1.1.11-alpha.
In 0.3.0.3-alpha, when we made primary guard descriptors necessary
for circuit building, this situation got worse.
When calculating the fraction of nodes that have descriptors, and all
all nodes in the network have zero bandwidths, count the number of nodes
instead.
Fixes bug 23318; bugfix on 0.2.4.10-alpha.
Back in 0.2.4.3-alpha (e106812a77), when we switched from using
double to using uint64 for selecting by bandwidth, I got the math
wrong: I should have used llround(x), or (uint64_t)(x+0.5), but
instead I wrote llround(x+0.5). That means we would always round
up, rather than rounding to the closest integer
Fixes bug 23318; bugfix on 0.2.4.3-alpha.
The flush cells process can close a channel if the connection write fails but
still return that it flushed at least one cell. This is due because the error
is not propagated up the call stack so there is no way of knowing if the flush
actually was successful or not.
Because this would require an important refactoring touching multiple
subsystems, this patch is a bandaid to avoid the KIST scheduler to handle
closed channel in its loop.
Bandaid on #23751.
Signed-off-by: David Goulet <dgoulet@torproject.org>
dirvote_get_next_valid_after_time() is the only public function that uses the
voting schedule outside of the dirvote subsystem so if it is zeroed,
recalculate its timing if we can that is if a consensus exists.
Part of #24161
Signed-off-by: David Goulet <dgoulet@torproject.org>
Because the HS and SR subsystems can use the voting schedule early (with the
changes in #23623 making the SR subsystem using the static voting schedule
object), we need to recalculate the schedule very early when setting the new
consensus.
Fixes#24161
Signed-off-by: David Goulet <dgoulet@torproject.org>
If it decrypts something that turns out to start with a NUL byte,
then decrypt_desc_layer() will return 0 to indicate the length of
its result. But 0 also indicates an error, which causes the result
not to be freed by decrypt_desc_layer()'s callers.
Since we're trying to stabilize 0.3.2.x, I've opted for the simpler
possible fix here and made it so that an empty decrypted string will
also count as an error.
Fixes bug 24150 and OSS-Fuzz issue 3994.
The original bug was present but unreachable in 0.3.1.1-alpha. I'm
calling this a bugfix on 0.3.2.1-alpha since that's the first version
where you could actually try to decrypt these descriptors.
The node_get_ed25519_id() warning can actually be triggered by a relay flagged
with NoEdConsensus so instead of triggering a warning on all relays of the
network, downgrade it to protocol warning.
Fixes#24025
Signed-off-by: David Goulet <dgoulet@torproject.org>
When a BUG() occurs, this macro will print extra information about the state
of the scheduler and the given channel if any. This will help us greatly to
fix future bugs in the scheduler especially when they occur rarely.
Fixes#23753
Signed-off-by: David Goulet <dgoulet@torproject.org>
When running "make test-network-all", test that IPv6-only clients can use
microdescriptors. IPv6-only microdescriptor client support was fixed in
tor 0.3.0.1-alpha.
Requires chutney master 61c28b9 or later.
Closes ticket 24109.
They are not yet implemented: they will upload descriptors, but won't be
able to rendezvous, because IPv6 addresses in link specifiers are ignored.
Part of #23820.
The previous version of this function had the following issues:
* it didn't check if the extend_info contained an IPv6 address,
* it didn't check if the ed25519 identity key was valid.
But we can't add IPv6 support in a bugfix release.
Instead, BUG() if the address is an IPv6 address, so we always put IPv4
addresses in link specifiers. And ignore missing ed25519 identifiers,
rather than generating an all-zero link specifier.
This supports v3 hidden services on IPv4, dual-stack, and IPv6, and
v3 single onion services on IPv4 only.
Part of 23820, bugfix on 0.3.2.1-alpha.
