Recent Python3 versions seem to require this on Windows.
Fixes bug 26535; bug introduced in f4be34f70d, which
was apparently intended itself as a Python3 workaround.
For HSv3, the HSADDRESS= wasn't properly parsed for the HSPOST command. It now
correctly use it and furthermore sends back a "200 OK" in case the command is
successful for a v3 descriptor.
Fixes#26523
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch fixes a memory leak in disk_state_parse_commits() where if
commit is NULL, we continue the internal loop, but without ever freeing
the args variable.
See: Coverity CID 1437441.
This patch fixes a memory leak in frac_nodes_with_descriptors() where
we might return without free'ing the bandwidths variable.
See: Coverity CID 1437451.
This patch fixes a memory leak in new_establish_intro_cell() that could
happen if a test assertion fails and the *cell_out value isn't properly
free'd.
See: Coverity CID 1437445
This patch fixes a memory leak in decode_link_specifiers() where the
hs_spec variable might leak if the default label is taken in the
switch/case expression.
See: Coverity CID 1437437.
This patches fixes a memory leak in client_likes_consensus() where if
consensus_cache_entry_get_voter_id_digests() would fail we would return
without having free'd the voters list.
See: Coverity CID 1437447
This patch fixes a memory leak in hs_helper_build_hs_desc_impl() where
if a test assertion would fail we would leak the storage that `desc`
points to.
See: Coverity CID 1437448
This patch fixes a memory leak in pick_hsdir_v3() where we might return
early, but forgot to free the responsible_hsdirs variable. We solve this
by not allocating storage for responsible_hsdirs until it's actually
needed.
See: Coverity CID 1437449
This patch fixes a potential memory leak in test_hs_auth_cookies() if a
test-case fails and we goto the done label where no memory clean up is
done.
See: Coverity CID 1437453
This patch fixes a potential memory leak in
hs_helper_build_intro_point() where a `goto done` is called before the
`intro_point` variable have been assigned to the value of the `ip`
variable.
See: Coverity CID 1437460
See: Coverity CID 1437456
Out-of-tree builds could fail to run the rust tests if built in
offline mode. cargo expects CARGO_HOME to point to the .cargo
directory, not the directory containing .cargo.
Fixes bug 26455; bug not in any released tor.
Fix a memory leak where directory authorities would leak a chunk of
memory for every router descriptor every time they considered voting.
This bug was taking down directory authorities in the live network due
to out-of-memory issues.
Fixes bug 26435; bugfix on 0.3.3.6.
Also make sure that we're actually running the test from within the right
cwd, like we do when we're building. This seems necessary to avoid
an error when running offline.
Amusingly, it appears that we had this bug before: we just weren't
noticing it, because of bug 26258.
Some versions of GCC complain that the bfn_mock_node_get_by_id
function might return NULL, but we're assuming that it won't.
(We're assuming it won't return NULL because we know in the tests
that we're passing it valid IDs.)
To make GCC happy, tt_assert() that each node_t is set before using
it.
Fixes a second case of bug26269; bugfix on 0.3.0.1-alpha.
There are a few reasons that relays might be uploading desciptors
without saying X-Desc-Gen-Reason:
1. They are running an old version of our software, before 0.3.2.stable.
2. They are not running our software, but they are claiming they
are.
3. They are uploading through a proxy that strips X-Desc-Gen-Reason.
4. They somehow had a bug in their software.
According to the 25686 data, 1 is the most common reason. This
ticket is an attempt to diagnose case 4, or prove that case 4
doesn't actually happen.
* REFACTORS `UnvalidatedProtoEntry::from_str` to place the bulk of the
splitting/parsing logic in to a new
`UnvalidatedProtoEntry::parse_protocol_and_version_str()` method (so that
both `from_str()` and `from_str_any_len()` can call it.)
* ADD a new `UnvalidatedProtoEntry::from_str_any_len()` method in order to
maintain compatibility with consensus methods older than 29.
* ADD a limit on the number of characters in a protocol name.
* FIXES part of #25517: https://bugs.torproject.org/25517
In protover.c, the `expand_protocol_list()` function expands a `smartlist_t` of
`proto_entry_t`s to their protocol name concatenated with each version number.
For example, given a `proto_entry_t` like so:
proto_entry_t *proto = tor_malloc(sizeof(proto_entry_t));
proto_range_t *range = tor_malloc_zero(sizeof(proto_range_t));
proto->name = tor_strdup("DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa");
proto->ranges = smartlist_new();
range->low = 1;
range->high = 65536;
smartlist_add(proto->ranges, range);
(Where `[19KB]` is roughly 19KB of `"a"` bytes.) This would expand in
`expand_protocol_list()` to a `smartlist_t` containing 65536 copies of the
string, e.g.:
"DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=1"
"DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=2"
[…]
"DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=65535"
Thus constituting a potential resource exhaustion attack.
The Rust implementation is not subject to this attack, because it instead
expands the above string into a `HashMap<String, HashSet<u32>` prior to #24031,
and a `HashMap<UnvalidatedProtocol, ProtoSet>` after). Neither Rust version is
subject to this attack, because it only stores the `String` once per protocol.
(Although a related, but apparently of too minor impact to be usable, DoS bug
has been fixed in #24031. [0])
[0]: https://bugs.torproject.org/24031
* ADDS hard limit on protocol name lengths in protover.c and checks in
parse_single_entry() and expand_protocol_list().
* ADDS tests to ensure the bug is caught.
* FIXES#25517: https://bugs.torproject.org/25517
The specification describes the signature token to be right after a newline
(\n) then the token "signature" and then a white-space followed by the encoded
signature.
This commit makes sure that when we parse the signature from the descriptor,
we are always looking for that extra white-space at the end of the token.
It will allow us also to support future fields that might start with
"signature".
Fixes#26069
Signed-off-by: David Goulet <dgoulet@torproject.org>
The any_client_port_set() returns true if the ControlPort is set which is
wrong because we can have that port open but still not behave as a tor client
(like many relays for instance).
Fixes#26062
Signed-off-by: David Goulet <dgoulet@torproject.org>
When directory authorities read a zero-byte bandwidth file, they log
a warning with the contents of an uninitialised buffer. Log a warning
about the empty file instead.
Fixes bug 26007; bugfix on 0.2.2.1-alpha.
Remove v3 optimization which made Tor not detect disabling services.
This optimization is not so needed because we only call that function after HUP
anyway.
Fixes bug #25761.
During service configuration, rend_service_prune_list_impl_() sets
rend_service_staging_list to NULL, which blocked pruning after a HUP.
This patch initializes rend_service_staging_list when needed, so that HUP can
detect disabled onion services.
Fixes bug #25761.
When directory authorities read a zero-byte bandwidth file, they log
a warning with the contents of an uninitialised buffer. Log a warning
about the empty file instead.
Fixes bug 26007; bugfix on 0.2.2.1-alpha.
LibreSSL, despite not having the OpenSSL 1.1 API, does define
OPENSSL_VERSION in crypto.h. Additionally, it apparently annotates
some functions as returning NULL, so that our unit tests need to be
more careful about checking for NULL so they don't get compilation
warnings.
Closes ticket 26006.
Prior to #23100, we were not counting HS circuit build times in our
calculation of the timeout. This could lead to a condition where our timeout
was set too low, based on non HS circuit build times, and then we would
abandon all HS circuits, storing no valid timeouts in the histogram.
This commit avoids the assert.
It tried to pick nodes for which only routerinfo_t items are set,
but without setting UseMicroDescriptors to 0. This won't work any
more, now that we're strict about using the right descriptor types
due to 25691/25692/25213.
In order to fix 25691 and 25692, we need to pass the "direct_conn"
flag to more places -- particularly when choosing single-hop
tunnels. The right way to do this involves having a couple more
functions accept router_crn_flags_t, rather than a big list of
boolean arguments.
This commit also makes sure that choose_good_exit_server_general()
honors the direct_conn flag, to fix 25691 and 25692.
In router_add_running_nodes_to_smartlist(), we had an inline
implementation of the logic from node_has_descriptor(), which should
be changed to node_has_preferred_descriptor().
This patch adds a new node_has_preferred_descriptor() function, and
replaces most users of node_has_descriptor() with it. That's an
important change, since as of d1874b4339 (our fix for #25213),
we are willing to say that a node has _some_ descriptor, but not the
_right_ descriptor for a particular use case.
Part of a fix for 25691 and 25692.
Now that we allow cpuworkers for dirport-only hosts (to fix 23693),
we need to allow dup_onion_keys() to succeed for them.
The change to construct_ntor_key_map() is for correctness,
but is not strictly necessary.
We recently merged a circuit cell queue size safeguard. This commit adds the
number of killed circuits that have reached the limit to the DoS heartbeat. It
now looks like this:
[notice] DoS mitigation since startup: 0 circuits killed with too many
cells. 0 circuits rejected, 0 marked addresses. 0 connections closed. 0
single hop clients refused.
Second thing that this patch does. It makes tor always print the DoS
mitigation heartbeat line (for a relay) even though no DoS mitigation have
been enabled. The reason is because we now kill circuits that have too many
cells regardless on if it is enabled or not but also it will give the operator
a chance to learn what is enabled with the heartbeat instead of suddenly
appearing when it is enabled by let say the consensus.
Fixes#25824
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit introduces the consensus parameter "circ_max_cell_queue_size"
which controls the maximum number of cells a circuit queue should have.
The default value is currently 50000 cells which is above what should be
expected but keeps us a margin of error for padding cells.
Related to this is #9072. Back in 0.2.4.14-alpha, we've removed that limit due
to a Guard discovery attack. Ticket #25226 details why we are putting back the
limit due to the memory pressure issue on relays.
Fixes#25226
Signed-off-by: David Goulet <dgoulet@torproject.org>
In d1874b4339, we adjusted this check so that we insist on
using routerinfos for bridges. That's almost correct... but if we
have a bridge that is also a regular relay, then we should use
insist on its routerinfo when connecting to it as a bridge
(directly), and be willing to use its microdescriptor when
connecting to it elsewhere in our circuits.
This bug is a likely cause of some (all?) of the (exit_ei == NULL)
failures we've been seeing.
Fixes bug 25691; bugfix on 0.3.3.4-alpha
When size_t is 32 bits, the unit tests can't fit anything more than
4GB-1 into a size_t.
Additionally, tt_int_op() uses "long" -- we need tt_u64_op() to
safely test uint64_t values for equality.
Bug caused by tests for #24782 fix; not in any released Tor.
This patch changes the algorithm of compute_real_max_mem_in_queues() to
use 0.4 * RAM iff the system has more than or equal to 8 GB of RAM, but
will continue to use the old value of 0.75 * RAM if the system have less
than * GB of RAM available.
This patch also adds tests for compute_real_max_mem_in_queues().
See: https://bugs.torproject.org/24782
This patch makes get_total_system_memory mockable, which allows us to
alter the return value of the function in tests.
See: https://bugs.torproject.org/24782
This one happens if for some reason you start with DirPort enabled
but server mode turned off entirely.
Fixes a case of bug 23693; bugfix on 0.3.1.1-alpha.
This patch lifts the list of default directory authorities from config.c
into their own auth_dirs.inc file, which is then included in config.c
using the C preprocessor.
Patch by beastr0.
See: https://bugs.torproject.org/24854
Next commit is addressing the circuit queue cell limit so cleanup before doing
anything else.
Part of #25226
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied
in Rust to the maximum number of version (total, for all subprotocols).
Whereas in C, it was being applied to the number of subprotocols that were
allowed. This changes the Rust to match C's behaviour.
The behaviours still do not match, unsurprisingly, but now we know where a
primary difference is: the Rust is validating version ranges more than the C,
so in the C it's possible to call protover_all_supported on a ridiculous
version range like "Sleen=0-4294967294" because the C uses
MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust
uses it to count the total number of *versions* of all subprotocols.
There's now no difference in these tests w.r.t. the C or Rust: both
fail miserably (well, Rust fails with nice descriptive errors, and C
gives you a traceback, because, well, C).
The DoS potential is slightly higher in C now due to some differences to the
Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs.
Also, the comment about "failing at the splitting stage" in Rust wasn't true,
since when we split, we ignore empty chunks (e.g. "1--1" parses into
"(1,None),(None,1)" and "None" can't be parsed into an integer).
Finally, the comment about "Rust seems to experience an internal error" is only
true in debug mode, where u32s are bounds-checked at runtime. In release mode,
code expressing the equivalent of this test will error with
`Err(ProtoverError::Unparseable)` because 4294967295 is too large.
Previously, if "Link=1-5" was supported, and you asked protover_all_supported()
(or protover::all_supported() in Rust) if it supported "Link=3-999", the C
version would return "Link=3-999" and the Rust would return "Link=6-999". These
both behave the same now, i.e. both return "Link=6-999".
During code review and discussion with Chelsea Komlo, she pointed out
that protover::compute_for_old_tor() was a public function whose
return type was `&'static CStr`. We both agree that C-like parts of
APIs should:
1. not be exposed publicly (to other Rust crates),
2. only be called in the appropriate FFI code,
3. not expose types which are meant for FFI code (e.g. `*mut char`,
`CString`, `*const c_int`, etc.) to the pure-Rust code of other
crates.
4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called
from pure-Rust, not even from other modules in its own crate
(i.e. do not call `protover::ffi::*` from anywhere in
`protover::protoset::*`, etc).
With that in mind, this commit makes the following changes:
* CHANGE `protover::compute_for_old_tor()` to be
visible only at the `pub(crate)` level.
* RENAME `protover::compute_for_old_tor()` to
`protover::compute_for_old_tor_cstr()` to reflect the last change.
* ADD a new `protover::compute_for_old_tor()` function wrapper which
is public and intended for other Rust code to use, which returns a
`&str`.
It was changed to take borrows instead of taking ownership.
* REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method
signature on `protover::is_supported_here()`.
This includes a subtle difference in behaviour to the previous Rust
implementation, where, for each vote that we're computing over, if a single one
fails to parse, we skip it. This now matches the current behaviour in the C
implementation.
* REFACTOR `protover::ffi::protover_compute_vote()` to use
new types and methods.
This includes a subtle difference in behaviour, as in 4258f1e18, where we return
(matching the C impl's return behaviour) earlier than before if parsing failed,
saving us computation in parsing the versions into a
protover::protoset::ProtoSet.
* REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use
new types and methods.
