In hs_config.c, we do validate the permission of the hidden service directory
but we do not try to create it. So, in the event that the directory doesn't
exists, we end up in the loading key code path which checks for the
permission and possibly creates the directory. On failure, don't BUG() since
there is a perfectly valid use case for that function to fail.
Fixes#27335
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is harder than with OpenSSL, since OpenSSL counts the bytes on
its own and NSS doesn't. To fix this, we need to define a new
PRFileDesc layer that has its own byte-counting support.
Closes ticket 27289.
On GCC and Clang, there's a feature to warn you about bad
conditionals like "if (a = b)", which should be "if (a == b)".
However, they don't warn you if there are extra parentheses around
"a = b".
Unfortunately, the tor_assert() macro and all of its kin have been
passing their inputs through stuff like PREDICT_UNLIKELY(expr) or
PREDICT_UNLIKELY(!(expr)), both of which expand to stuff with more
parentheses around "expr", thus suppressing these warnings.
To fix this, this patch introduces new macros that do not wrap
expr. They're only used when GCC or Clang is enabled (both define
__GNUC__), since they require GCC's "({statement expression})"
syntax extension. They're only used when we're building the
unit-test variant of the object files, since they suppress the
branch-prediction hints.
I've confirmed that tor_assert(), tor_assert_nonfatal(),
tor_assert_nonfatal_once(), BUG(), and IF_BUG_ONCE() all now give
compiler warnings when their argument is an assignment expression.
Fixes bug 27709.
Bugfix on 0.0.6, where we first introduced the "tor_assert()" macro.
.retain() would allocating a Vec of billions of integers and check them
one at a time to separate the supported versions from the unsupported.
This leads to a memory DoS.
Closes ticket 27206. Bugfix on e6625113c9.
Before 0.3.3.1-alpha, we would exit() in this case immediately. But
now that we leave tor_main() more conventionally, we need to make
sure we restore things so as not to cause a double free.
Fixes bug 27708; bugfix on 0.3.3.1-alpha.
Since we use a 32-bit approximation for millisecond conversion here,
we can't expect so much precision.
Fixes part of bug 27139; bugfix on 0.3.4.1-alpha.
Multiply-then-divide is more accurate, but it runs into trouble when
our input is above INT32_MAX/numerator. So when our value is too
large, do divide-then-multiply instead.
Fixes part of bug 27139; bugfix on 0.3.4.1-alpha.
We use an optimized but less accurate formula for converting coarse
time differences to milliseconds on 32-bit OSX platforms, so that we
can avoid 64-bit division.
The old numbers were off by 0.4%. The new numbers are off by .006%.
This should make the unit tests a bit cleaner, and our tolerances a
bit closer.
All node_get_all_orports() does is allocate and return a smartlist
with at most two tor_addr_port_t members that match ORPort's of
node configuration. This is harmful for memory efficiency, as it
allocates the same stuff every time it is called. However,
node_is_a_configured_bridge() does not need to call it, as it
already has all the information to check if there is configured
bridge for a given node.
The new code is arranged in a way that hopefully makes each succeeding
linear search through bridge_list less likely.
We determine that a cell was dropped by inspecting CIRC_BW fields. If we did
not update the delivered or overhead fields after processing the cell, the
cell was dropped/not processed.
Also emit CIRC_BW events for cases where we decide to close the circuit in
this function, so vanguards can print messages about dropped cells in those
cases, too.
This function tells the underlying TLS object that it shouldn't
close the fd on exit. Mostly, we hope not to have to use it, since
the NSS implementation is kludgey, but it should allow us to fix
It's possible for a unit test to report success via its pipe, but to
fail as it tries to clean up and exit. Notably, this happens on a
leak sanitizer failure.
Fixes bug 27658; bugfix on 0.2.2.4-alpha when tinytest was
introduced.
This commit makes it that the authorized clients in the descriptor are in
random order instead of ordered by how they were read on disk.
Fixes#27545
Signed-off-by: David Goulet <dgoulet@torproject.org>
Existing cached directory information can cause misleadingly high
bootstrap percentages. To improve user experience, defer reporting of
directory information progress until at least one connection has
succeeded to a relay or bridge.
Closes ticket 27169.
If a tor client gets a descriptor that it can't decrypt, chances are that the
onion requires client authorization.
If a tor client is configured with client authorization for an onion but
decryption fails, it means that the configured keys aren't working anymore.
In both cases, we'll log notice the former and log warn the latter and the
rest of the decryption errors are now at info level.
Two logs statement have been removed because it was redundant and printing the
fetched descriptor in the logs when 80% of it is encrypted wat not helping.
Fixes#27550
Signed-off-by: David Goulet <dgoulet@torproject.org>
Track bootstrap phase (enumerated by bootstrap_status_t) independently
from the bootstrap progress (which can represent intermediate
progress). This allows control_event_bootstrap_problem() to avoid
doing a linear search through the bootstrap progress space to find the
current bootstrap phase.
Move the mostly-invariant part of control_event_boostrap() into a
helper control_event_bootstrap_core(). The helper doesn't modify any
state beyond doing logging and control port notifications.
Simplify control_event_bootstrap() by making it return void again. It
is currently a fairly complicated function, and it's made more
complicated by returning an int to signal whether it logged at NOTICE
or INFO.
The callers conditionally log messages at level NOTICE based on this
return value. Change the callers to unconditionally log their verbose
human-readable messages at level INFO to keep NOTICE logs less
cluttered.
This partially reverts the changes of #14950.
One HSv3 unit test used "tor_memeq()" without checking the return value. This
commit changes that to use "tt_mem_op()" to actually make the test validate
something :).
Signed-off-by: David Goulet <dgoulet@torproject.org>
>>>> CID 1439133: Null pointer dereferences (REVERSE_INULL)
>>>> Null-checking "fields" suggests that it may be null, but it
>>>> has already been dereferenced on all paths leading to the check.
>>>> CID 1439132: Null pointer dereferences (REVERSE_INULL)
>>>> Null-checking "fields" suggests that it may be null, but it
>>>> has already been dereferenced on all paths leading to the check.
This is an attempt to work around what I think may be a bug in
OSS-Fuzz, which thinks that uninitialized data might be passed to
the curve25519 functions.
There are three reasons we use a cached_dir_t to hold a consensus:
1. to serve that consensus to a client
2. to apply a consensus diff to an existing consensus
3. to send the consensus to a controller.
But case 1 is dircache-only. Case 2 and case 3 both fall back to
networkstatus_read_cached_consensus(). So there's no reason for us
to store this as a client. Avoiding this saves about 23% of our RAM
usage, according to our experiments last month.
This is, semantically, a partial revert of e5c608e535.
Fixes bug 27247; bugfix on 0.3.0.1-alpha.
We already had fallback code for "dir/status-vote/current/consensus"
to read from disk if we didn't have a cached_dir_t available. But
there's a function in networkstatus_t that does it for us, so let's
do that.
Return a newly allocated fake client authorization object instead of taking
the object as a parameter.
Signed-off-by: David Goulet <dgoulet@torproject.org>
When reloading tor, check if our the configured client authorization have
changed from what we previously had. If so, republish the updated descriptor.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, the validation by decoding a created descriptor was disabled
because the interface had to be entirely changed and not implemented at the
time.
This commit re-enabled it because it is now implemented.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Parse the client authorization section from the descriptor, use the client
private key to decrypt the auth clients, and then use the descriptor cookie to
decrypt the descriptor.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit refactors the existing decryption code to make it compatible with
a new logic for when the client authorization is enabled.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Because this secret data building logic is not only used by the descriptor
encoding process but also by the descriptor decoding, refactor the function to
take both steps into account.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The new ClientOnionAuthDir option is introduced which is where tor looks to
find the HS v3 client authorization files containing the client private key
material.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, we encrypted the descriptor without the descriptor cookie. This
commit, when the client auth is enabled, the descriptor cookie is always used.
I also removed the code that is used to generate fake auth clients because it
will not be used anymore.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit tests that the descriptor building result, when the client
authorization is enabled, includes everything that is needed.
Signed-off-by: David Goulet <dgoulet@torproject.org>
We need to generate all the related keys when building the descriptor, so that
we can encrypt the descriptor.
Signed-off-by: David Goulet <dgoulet@torproject.org>
It is not supported, and always fails. Some compilers warn about the
function pointer cast on 64-bit Windows.
Fixes bug 27461; bugfix on 0.2.2.23-alpha.
gcc 8 warns that extend_info_t.nickname might be truncated by strncpy().
But it doesn't know that nickname can either contain a hex id, or a
nicknames. hex ids are only used for general and HSDir circuits.
Fixes bug 27463; bugfix on 0.1.1.2-alpha.
GetProcAddress() returns FARPROC, which is (long long int(*)()) on
64-bit Windows:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683212(v=vs.85).aspx
But GetAdaptersAddresses() is (long unsigned int(*)()), on both 32-bit
and 64-bit Windows:
https://docs.microsoft.com/en-us/windows/desktop/api/iphlpapi/nf-iphlpapi-getadaptersaddresses
So gcc 8 issues a spurious "incompatible function pointer" warning
about the cast to GetAdaptersAddresses_fn_t.
Silence this warning by casting to a void function pointer, before
the cast to GetAdaptersAddresses_fn_t.
This issue is already fixed by 26481 in 0.3.5 and later, by removing
the lookup and cast.
Fixes bug 27465; bugfix on 0.2.3.11-alpha.
This reverts commit b5fddbd241.
The commit here was supposed to be a solution for #27451 (fd
management with NSS), but instead it caused an assertion failure.
Fixes bug 27500; but not in any released Tor.
On new glibc versions, there's an explicit_bzero(). With openssl,
there's openssl_memwipe().
When no other approach works, use memwipe() and a memory barrier.
This function was a wrapper around RSA_check_key() in openssl, which
checks for invalid RSA private keys (like those where p or q are
composite, or where d is not the inverse of e, or where n != p*q).
We don't need a function like this in NSS, since unlike OpenSSL, NSS
won't let you import a bogus private key.
I've renamed the function and changed its return type to make it
more reasonable, and added a unit test for trying to read a key
where n != p*q.
This function was supposed to implement a half-duplex mode for our
TLS connections. However, nothing in Tor actually uses it (besides
some unit tests), and the implementation looks really questionable
to me. It's probably best to remove it. We can add a tested one
later if we need one in the future.
The OpenSSL "RSA" object is currently 408 bytes compares to the ASN.1 encoding
which is 140 for a 1024 RSA key.
We save 268 bytes per descriptor (routerinfo_t) *and* microdescriptor
(microdesc_t). Scaling this to 6000 relays, and considering client usually
only have microdescriptors, we save 1.608 MB of RAM which is considerable for
mobile client.
This commit makes it that we keep the RSA onion public key (used for TAP
handshake) in ASN.1 format instead of an OpenSSL RSA object.
Changes is done in both routerinfo_t and microdesc_t.
Closes#27246
Signed-off-by: David Goulet <dgoulet@torproject.org>
TRUNCATED cells were ignored while in path bias. Now they are obeyed, and
cause us to tear down the circuit. The actual impact is minimal, since we
would just wait around for a probe that would never arrive before.
This commit changes client behavior.
We allow their CONNECTEDs, RESOLVEDs, ENDs, SENDMEs, and DATA cells to not
count as dropped until the windows are empty, or we get an END.
This commit does not change behavior. It only changes CIRC_BW event field
values.
By removing Tor2Web, there is no way a client can be non anonymous so we
remove that function and the callsites.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Because we just removed Tor2web support, the need_specific_rp is not needed
anymore when cannibalizing a circuit.
Signed-off-by: David Goulet <dgoulet@torproject.org>
No behaviour change.
A previous fix to chutney removed v3 onion services from the
mixed+hs-v23 network, so seeing "mixed+hs-v23" in tests is
confusing.
Fixes bug 27345; bugfix on 0.3.2.1-alpha.
LinkAuth method 1 is the one where we pull the TLS master secrets
out of the OpenSSL data structures and authenticate them with
RSA. Right now we list method 1 as required for clients and relays.
That's a problem, since we can't reasonably support it with NSS. So
let's remove it as a requirement and a recommendation.
As for method 3: I'd like to recommend it it, but that would make
0.2.9 start warning. Let's not do that till at least some time
after 0.3.5 (the next LTS) is stable.
Closes ticket 27286
Instead, count exits as usable if they have the exit flag, and
present if they also have a non-reject exit policy.
Requiring a threshold of usable descriptors avoids directories trickling
exit descriptors to clients to discover their ExitNodes settings.
Part of 27236.
Previously, Tor would only check the exit flag. In small networks, Tor
could bootstrap once it received a consensus with exits, without fetching
the new descriptors for those exits.
After bootstrap, Tor delays descriptor fetches, leading to failures in
fast networks like chutney.
Fixes 27236; bugfix on 0.2.6.3-alpha.
In order to switch the default HS version from 2 to 3, we need tor to be smart
and be able to decide on the version by trying to load the service keys during
configuration validation.
Part of #27215
Signed-off-by: David Goulet <dgoulet@torproject.org>
Part of #27215, we need to call the ed_key_init_from_file function during
option_validate() which is before the global_options variable is set.
This commit make ed_key_init_from_file() stop using get_options() and instead
now has a or_options_t parameter.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Bug description: For each descriptor, its revision counter is the OPE
ciphertext of the number of seconds since the start time of its SRV value.
This bug caused us to confuse the SRV start time in the middle of the lifetime
of a descriptor in some edge-cases, which caused descriptor rejects.
Bug cause: The bug occurs when we fetch a 23:00 consensus after
midnight (e.g. at 00:08 when not all dirauths have fetched the latest 00:00
consensus). In that case, the voting schedule (which was used for SRV start
time calculation) would return a valid-after past-midnight, whereas our
consensus would be pre-midnight, and that would confuse the SRV start time
computation which is used by HS revision counters (because we would reset the
start time of SRV, without rotating descriptors).
Bug fix: We now use our local consensus time to calculate the SRV start time,
instead of the voting schedule. The voting schedule does not work as originally
envisioned in this case, because it was created for voting by dirauths and not
for scheduling stuff on clients.
We used to link both libraries at once, but now that I'm working on
TLS, there's nothing left to keep OpenSSL around for when NSS is
enabled.
Note that this patch causes a couple of places that still assumed
OpenSSL to be disabled when NSS is enabled
- tor-gencert
- pbkdf2
Also, add a stubbed-out nss version of the modules. The tests won't
pass with NSS yet since the NSS modules don't do anything.
This is a good patch to read with --color-moved.
This cleans up a lot of junk from crypto_rsa_openssl, and will
save us duplicated code in crypto_rsa_nss (when it exists).
(Actually, it already exists, but I am going to use git rebase so
that this commit precedes the creation of crypto_rsa_nss.)
Unlike the old test, this test no will no longer mess around with
the forbidden internals of any openssl data structures.
Additionally, it verifies several other behaviors of
tor_tls_cert_matches_key() that we had wanted to verify, such as
the possibility of the certificate's key not matching.
Fixes bug 27226; bugfix on 0.2.5.1-alpha.
This allows us to mock our own tor_tls_get_peer_certificate()
function in order to test ..cert_matches_key(), which will in turn
allow us to simplify test_tortls_cert_matches_key() considerably.
Prep work for the fix for 27226.
Until https://github.com/rust-lang/rust/issues/52652 is fixed,
unwinding on panic is potentially unsound in a mixed C/Rust codebase.
The codebase is supposed to be panic-free already, but just to be safe.
This started mattering at commit d1820c1516.
Fixes#27199; bugfix on tor-0.3.3.1-alpha.
It's impossible for spaces to get here, since spaces are used as
separators between individual protocol entries higher up.
And it shouldn't ignore whitespace that isn't a literal space
character, because that would differ from the C implementation.
These were added in 9925d2e687.
Fixes#27177. Bugfix on 0.3.3.5-rc.
It was parsing "1-2-3" as if it were 1-2, ignoring the 2nd hyphen
and everything after.
Introduced in d1820c1516.
Fixes#27164; bugfix on 0.3.3.1-alpha.
The function takes an already validated utf-8 string, and
it never checks if the version numbers are an empty string.
That parse error happens later.
Fix on 701c2b69f5
These are the 12 stable and documented configuration options,
set to their default values.
use_small_heuristics is only stabilized in rustfmt 0.9, so maintain
support for 0.8.x for now by commenting it out.
comment_width is unstable and did nothing, since wrap_comments defaults
to false.
Default values gotten from `rustfmt --print-config default rustfmt.toml`.
e7932fa9c2/Configurations.md
Replace master .travis.yml with 034 .travis.yml.
All the changes in master have been backported to the
034 .travis.yml already.
Replace master src/test/test_rust.sh with 034
src/test/test_rust.sh, which was backported from
master. One 033/034-specific commit needs to be
reverted.
Replace 034 .travis.yml with 033 .travis.yml.
Subsequent commits will restore 034 functionality.
Replace 034 src/test/test_rust.sh with 033
src/test/test_rust.sh, which was backported from
master.
Replace 033 .travis.yml with 032 .travis.yml.
Subsequent commits will restore 033 functionality.
src/rust/tor_util/include.am is deleted in 033.
Subsequent commits will apply 032 changes to
src/rust/tor_rust/include.am.
Replace 033 src/test/test_rust.sh with 032
src/test/test_rust.sh, which was backported from
master.
At the same time, sternly warn any person thinking about relying on
any particular format too strictly. If you do this, and your
program breaks, it is your bug, not mine.
I hope that the debian clang maintainers will look at debian bug
903709 soon. But until they do, this should keep our users and our
CI happy on sid with clang.
Closes ticket 26779.
The seccomp rule for the openat syscall checks for the AT_FDCWD
constant. Because this constant is usually a negative value, a
cast to unsigned int is necessary to make sure it does not get
converted to uint64_t used by seccomp.
More info on:
https://github.com/seccomp/libseccomp/issues/69#issuecomment-273805980
When we fixed 25939 in f7633c1fca, we
introduced a call to rescan_periodic_events() from inside the onion
service logic. But this meant that we could rescan the event list --
thereby running event callbacks! -- from inside the hidden service code.
This could cause us to run some of our event callbacks from an
inconsistent state, if we were in the middle of changing options.
A related bug (#25761) prevented us from rescanning our periodic
events as appropriate, but when we fixed THAT one, this bug reared
its ugly head.
The fix here is that "enabling" an event should cause us to run it
from the event loop, but not immediately from the point where we
enable it.
Fixes bug 27003; bugfix on 0.3.4.1-alpha.
We need this so that the tor_api user can specify some arguments,
while the tor_api implementation adds others.
This implementation detail should not be visible to tor_api users.
This change also makes tor_ersatz_socketpair() follow the same
interface as socketpair() rather than tor_socketpair(), so it now
needs to be wrapped in the same code as socketpair() does.
This is comparatively straightforward too, except for a couple of
twists:
* For as long as we're building with two crypto libraries, we
want to seed _both_ their RNGs, and use _both_ their RNGs to
improve the output of crypto_strongest_rand()
* The NSS prng will sometimes refuse to generate huge outputs.
When it does, we stretch the output with SHAKE. We only need
this for the tests.
We would usually call it through tor_cleanup(), but in some code
paths, we wouldn't. These paths would break restart-in-process,
since leaving fields uncleared would cause assertion failures on
restart.
Fixes bug 26948; bugfix on 0.3.3.1-alpha
Conditionalize the pragma that temporarily disables
-Wunused-const-variable. Some versions of gcc don't support it. We
need to do this because of an apparent bug in some libzstd headers.
Fixes bug 26785; bugfix on 0.3.2.11.
Instead, log a protocol warning when single onion services or
Tor2web clients fail to authenticate direct connections to relays.
Fixes bug 26924; bugfix on 0.2.9.1-alpha.
Stop putting ed25519 link specifiers in v3 onion service descriptors,
when the intro point doesn't support ed25519 link authentication.
Fixes bug 26627; bugfix on 0.3.2.4-alpha.
Stop sending ed25519 link specifiers in v3 onion service introduce
cells, when the rendezvous point doesn't support ed25519 link
authentication.
Fixes bug 26627; bugfix on 0.3.2.4-alpha.
Our previous definition implied that code would never keep running
if a BUG occurred (which it does), and that BUG(x) might be true
even if x was false (which it can't be).
Closes ticket 26890. Bugfix on 0.3.1.4-alpha.
This is another attempt to fix 1437668. The assertion here should
be safe, since the rules of networkstatus_get_param() keep the value
it returns in range.
The following bug was causing many issues for this branch in chutney:
In sr_state_get_start_time_of_current_protocol_run() we were using the
consensus valid-after to calculate beginning_of_current_round, but we were
using time(NULL) to calculate the current_round slot. This was causing time
sync issues when the consensus valid-after and time(NULL) were disagreeing on
what the current round is. Our fix is to use the consensus valid-after in both
places.
This also means that we are not using 'now' (aka time(NULL)) anymore in that
function, and hence we can remove that argument from the function (and its
callers). I'll do this in the next commit so that we keep things separated.
Furthermore, we fix a unittest that broke.
We only build a descriptor once, and we just re-encode it (and change its intro
points if needed) before uploading.
Hence we should set the revision counter before uploading, not during building.
The OPE cipher is tied to the current blinded key which is tied to the current
time period. Hence create the OPE cipher structure when we create a new
descriptor (and build its blinded key).
Now that the rev counter depends on the local time, we need to be more careful
in the unittests. Some unittests were breaking because they were using
consensus values from 1985, but they were not updating the local time
appropriately. That was causing the OPE module to complain that it was trying
to encrypt insanely large values.
To do so for a given descriptor, we use the "seconds since the SR protocol run"
started, for the SRV that is relevant to this descriptor. This is guaranteed to
be a positive value (since we need an SRV to be able to build a descriptor),
and it's also guaranteed to be a small value (since SRVs stop being listed on a
consensus after 48 hours).
We cannot use the "seconds since the time period started", because for the next
descriptor we use the next time period, so the timestamp would end up negative.
See [SERVICEUPLOAD] from rend-spec-v3.txt for more details.
To do so, we have to introduce a new `is_current` argument to a bunch of
functions, because to use "seconds since the SR protocol run" we need to know
if we are building the current or the next descriptor, since we use a different
SRV for each descriptor.
This is meant for use when encrypting the current time within the
period in order to get a monotonically increasing revision counter
without actually revealing our view of the time.
This scheme is far from the most state-of-the-art: don't use it for
anything else without careful analysis by somebody much smarter than
I am.
See ticket #25552 for some rationale for this logic.
If an authority is not configured with a V3BandwidthsFile, this line
SHOULD NOT appear in its vote.
If an authority is configured with a V3BandwidthsFile, but parsing
fails, this line SHOULD appear in its vote, but without any headers.
Part of 3723, implements the spec in 26799.
