On some windows builds, time_t is 64 bits but long is not. This is
causing appveyor builds to fail.
Also, one of our uses of labs() on time_t was logically incorrect:
it was telling us to accept NETINFO cells up to three minutes
_before_ the message they were responding to, which doesn't make
sense.
This patch adds a time_abs() function that we should eventually move
to intmath.h or something. For now, though, it will make merges
easier to have it file-local in channeltls.c.
Fixes bug 31343; bugfix on 0.2.4.4-alpha.
Padding circuits were regular cells that got closed before their padding
machine could finish. This means that they can still receive regular cells from
their past life, but they have no way or reason to answer them anymore. Hence
let's ignore them before they even get to the proper subsystems.
Make origin-side messages about padding negotiation failure into
LOG_PROTOCOL_WARN.
I'm not sure I like this either.. But the negotiation refusal case might
happen naturally due to consensus drift, and is functionally no different than
a corrupted cell.
There is other code that uses this value, and some of it is
apparently reachable from inside router_dir_info_changed(), which
routerlist_free() apparently calls. (ouch!) This is a minimal fix
to try to resolve the issue without causing other problems.
Fixes bug 31003. I'm calling this a bugfix on 0.1.2.2-alpha, where
the call to router_dir_info_changed() was added to routerlist_free().
Overflowing a signed integer in C is an undefined behaviour.
It is possible to trigger this undefined behaviour in tor_asprintf on
Windows or systems lacking vasprintf.
On these systems, eiter _vscprintf or vsnprintf is called to retrieve
the required amount of bytes to hold the string. These functions can
return INT_MAX. The easiest way to recreate this is the use of a
specially crafted configuration file, e.g. containing the line:
FirewallPorts AAAAA<in total 2147483610 As>
This line triggers the needed tor_asprintf call which eventually
leads to an INT_MAX return value from _vscprintf or vsnprintf.
The needed byte for \0 is added to the result, triggering the
overflow and therefore the undefined behaviour.
Casting the value to size_t before addition fixes the behaviour.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Overflowing a signed integer in C is an undefined behaviour.
It is possible to trigger this undefined behaviour in tor_asprintf on
Windows or systems lacking vasprintf.
On these systems, eiter _vscprintf or vsnprintf is called to retrieve
the required amount of bytes to hold the string. These functions can
return INT_MAX. The easiest way to recreate this is the use of a
specially crafted configuration file, e.g. containing the line:
FirewallPorts AAAAA<in total 2147483610 As>
This line triggers the needed tor_asprintf call which eventually
leads to an INT_MAX return value from _vscprintf or vsnprintf.
The needed byte for \0 is added to the result, triggering the
overflow and therefore the undefined behaviour.
Casting the value to size_t before addition fixes the behaviour.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
When we added LD_MESG, we created a conflict with the LD_NO_MOCK
flag. We now need 64 bits for log domains in order to fix this
issue.
Fixes bug 31080; bugfix on 0.4.1.1-alpha.
Update the fallback directory mirrors by merging the current list with:
fallback_dirs_2019-06-28-08-58-39_AU_f0437a39ddbc8459.inc
Part of 28795, see that ticket for logs.
Update the fallback directory mirrors by replacing the old list with:
fallback_dirs_2019-06-25-11-49-10_AU_a37adb956fbb5cd2.inc
Part of 28795, see that ticket for logs.
Always publish bridge pluggable transport information in the extra info
descriptor, even if ExtraInfoStatistics is 0. This information is
needed by BridgeDB.
Fixes bug 30956; bugfix on 0.4.1.1-alpha.
When we consider all circuits in "waiting for guard" state to be promoted to
an "open" state, we were considering all circuits, even the one marked for
close.
This ultiamtely triggers a "circuit_has_opened()" called on the circuit that
is marked for close which then leads to possible undesirable behaviors within
a subsystem.
For instance, the HS subsystem would be unable to find the authentication key
of the introduction point circuit leading to a BUG() warning and a duplicate
mark for close on the circuit.
This commit also adds a unit test to make sure we never select marked for
close circuits when upgrading its guard state from waiting for guard to open.
Fixes#30871
Signed-off-by: David Goulet <dgoulet@torproject.org>
When we consider all circuits in "waiting for guard" state to be promoted to
an "open" state, we were considering all circuits, even the one marked for
close.
This ultiamtely triggers a "circuit_has_opened()" called on the circuit that
is marked for close which then leads to possible undesirable behaviors within
a subsystem.
For instance, the HS subsystem would be unable to find the authentication key
of the introduction point circuit leading to a BUG() warning and a duplicate
mark for close on the circuit.
This commit also adds a unit test to make sure we never select marked for
close circuits when upgrading its guard state from waiting for guard to open.
Fixes#30871
Signed-off-by: David Goulet <dgoulet@torproject.org>
Skip test_rebind when the TOR_SKIP_TEST_REBIND environmental variable
is set.
Skip test_rebind on macOS in Travis builds, because it is unreliable
on macOS on Travis.
Fixes bug 30713; bugfix on 0.3.5.1-alpha.
Leave the other rend and hs_ident data around until circuit free, since code
may still try to inspect it after marking the circuit for close. The
circuitmap is the important thing to clean up, since repurposed
intropoints must be removed from this map to ensure validity.
Make origin-side messages about padding negotiation failure into
LOG_PROTOCOL_WARN.
I'm not sure I like this either.. But the negotiation refusal case might
happen naturally due to consensus drift, and is functionally no different than
a corrupted cell.
Leave the other rend and hs_ident data around until circuit free, since code
may still try to inspect it after marking the circuit for close. The
circuitmap is the important thing to clean up, since repurposed
intropoints must be removed from this map to ensure validity.
If "Log debug ..." is not set, the decrement never happens. This lead to the
package/deliver window to be out of sync at the stream level and thus breaking
the connection after 50+ cells.
Fixes#30628
Signed-off-by: David Goulet <dgoulet@torproject.org>
When this function was implemented, it counted all the entry guards
in the bridge set. But this included previously configured bridges,
as well as currently configured ones! Instead, only count the
_filtered_ bridges (ones that are configured and possibly reachable)
as maybe usable.
Fixes bug 29875; bugfix on 0.3.0.1-alpha.
This is necessary since shutting down libevent frees some pointer
that the subsystems want to free themselves. A longer term solution
will be to turn the evloop module into a subsystem itself, but for
now it is best to do the minimal fix.
Fixes bug 30629; bugfix on 0.4.1.1-alpha.
Previously, we were looking at our global settings to see what kind
of proxy we had. But doing this would sometimes give us the wrong
results when we had ClientTransportPlugin configured but we weren't
using it for a particular connection. In several places in the
code, we had added checks to see if we were _really_ using a PT or
whether we were using a socks proxy, but we had forgotten to do so
in at least once case. Instead, since every time we call this
function we are asking about a single connection, it is probably
best just to make this function connection-specific.
Fixes bug 29670; bugfix on 0.2.6.2-alpha.
When we repurpose a hidden service circuit, we need to clean up from the HS
circuit map and any HS related data structured contained in the circuit.
This commit adds an helper function that does it when repurposing a hidden
service circuit.
Fixes#29034
Signed-off-by: David Goulet <dgoulet@torproject.org>
If tor is compiled on a system with neither vasprintf nor _vscprintf,
the fallback implementation exposes a logic flaw which prevents
proper usage of strings longer than 127 characters:
* tor_vsnprintf returns -1 if supplied buffer is not large enough,
but tor_vasprintf uses this function to retrieve required length
* the result of tor_vsnprintf is not properly checked for negative
return values
Both aspects together could in theory lead to exposure of uninitialized
stack memory in the resulting string. This requires an invalid format
string or data that exceeds integer limitations.
Fortunately tor is not even able to run with this implementation because
it runs into asserts early on during startup. Also the unit tests fail
during a "make check" run.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
[backported to 0.2.9 by nickm]
Fixes assertion failure in tests on NetBSD:
slow/prob_distr/stochastic_log_logistic: [forking] May 25 03:56:58.091 [err] tor_assertion_failed_(): Bug: src/lib/crypt_ops/crypto_rand_fast.c:184: crypto_fast_rng_new_from_seed: Assertion inherit != INHERIT_RES_KEEP failed; aborting. (on Tor 0.4.1.1-alpha-dev 29955f13e5)
May 25 03:56:58.091 [err] Bug: Assertion inherit != INHERIT_RES_KEEP failed in crypto_fast_rng_new_from_seed at src/lib/crypt_ops/crypto_rand_fast.c:184: . (Stack trace not available) (on Tor 0.4.1.1-alpha-dev 29955f13e5)
[Lost connection!]
Proposal 289 prevents SENDME-flooding by requiring the other side to
authenticate the data it has received. But this data won't actually
be random if they are downloading a known resource. "No problem",
we said, "let's fell the empty parts of our cells with some
randomness!" and we did that in #26871.
Unfortunately, if the relay data payloads are all completely full,
there won't be any empty parts for us to randomize.
Therefore, we now pick random "randomness windows" between
CIRCWINDOW_INCREMENT/2 and CIRCWINDOW_INCREMENT. We remember whether we have
sent a cell containing at least 16 bytes of randomness in that window. If we
haven't, then when the window is exhausted, we send one. (This window approach
is designed to lower the number of rng checks we have to do. The number 16 is
pulled out of a hat to change the attacker's guessing difficulty to
"impossible".)
Implements 28646.
Because it invokes the Tor mainloop, it does unpredictable things to
test coverage of a lot of code that it doesn't actually test at
all. (It is more an integration test than anything else.)
The ordinary definitions of timeradd() and timersub() contain a
branch. However, in coverage builds, this means that we get spurious
complaints about partially covered basic blocks, in a way that makes
our coverage determinism harder to check.
Ordinarily we skip calling log_fn(LOG_DEBUG,...) if debug logging is
completely disabled. However, in coverage builds, this means that
we get spurious complaints about partially covered basic blocks, in
a way that makes our coverage determinism harder to check.
Two non fatal asserts are added in this commit. First one is to see if the
SENDME digest list kept on the circuit for validation ever grows bigger than
the maximum number of expected SENDME on a circuit (currently 10).
The second one is to know if we ever send more than one SENDME at a time on a
circuit. In theory, we shouldn't but if we ever do, the v1 implementation
wouldn't work because we only keep one single cell digest (the previous cell
to the SENDME) on the circuit/cpath. Thus, sending two SENDME consecutively
will lead to a mismatch on the other side because the same cell digest would
be use and thus the circuit would collapse.
Finally, add an extra debug log in case we emit a v0 which also includes the
consensus emit version in that case.
Part of #30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
We must not accumulate digests on the circuit if the other end point is using
another SENDME version that is not using those digests like v0.
This commit makes it that we always pop the digest regardless of the version.
Part of #30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
Commit 4ef8470fa5480d3b was actually reverted before because in the end we
needed to do this minus 1 check on the window.
This commit clarifies that in the code, takes the useful comment changes from
4ef8470fa5480d3b and makes sendme_circuit_cell_is_next() private since it
behaves in a very specific way that one external caller might expect.
Part of #30428.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Turns out that we were only recording the "b_digest" but to have
bidirectionnal authenticated SENDMEs, we need to use the "f_digest" in the
forward cell situation.
Because of the cpath refactoring, this commit plays with the crypt_path_ and
relay_crypto_t API a little bit in order to respect the abstractions.
Previously, we would record the cell digest as the SENDME digest in the
decrypt cell function but to avoid code duplication (both directions needs to
record), we now do that right after iff the cell is recognized (at the edge).
It is now done in circuit_receive_relay_cell() instead.
We now also record the cell digest as the SENDME digest in both relay cell
encryption functions since they are split depending on the direction.
relay_encrypt_cell_outbound() and relay_encrypt_cell_inbound() need to
consider recording the cell digest depending on their direction (f vs b
digest).
Fixes#30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
There was a missing cell version check against our max supported version. In
other words, we do not fallback to v0 anymore in case we do know the SENDME
version.
We can either handle it or not, never fallback to the unauthenticated version
in order to avoid gaming the authenticated logic.
Add a unit tests making sure we properly test that and also test that we can
always handle the default emit and accepted versions.
Fixes#30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
The validation of the SENDME cell is now done as the very first thing when
receiving it for both client and exit. On failure to validate, the circuit is
closed as detailed in the specification.
Part of #30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
It turns out that only the exit side is validating the authenticated SENDME v1
logic and never the client side. Which means that if a client ever uploaded
data towards an exit, the authenticated SENDME logic wouldn't apply.
For this to work, we have to record the cell digest client side as well which
introduced a new function that supports both type of edges.
This also removes a test that is not valid anymore which was that we didn't
allow cell recording on an origin circuit (client).
Part of #30428
Signed-off-by: David Goulet <dgoulet@torproject.org>
We want to support parsing a cell with unknown status code so we are forward
compatible.
Part of #30454
Signed-off-by: David Goulet <dgoulet@torproject.org>
Like the previous commit about the INTRODUCE_ACK status code, change all auth
key type to use the one defined in the trunnel file.
Standardize the use of these auth type to a common ABI.
Part of #30454
Signed-off-by: David Goulet <dgoulet@torproject.org>
This enum was the exact same as hs_intro_ack_status_t that was removed at the
previous commit. It was used client side when parsing the INTRODUCE_ACK cell.
Now, the entire code dealing with the INTRODUCE_ACK cell (both sending and
receiving) have been modified to all use the same ABI defined in the trunnel
introduce1 file.
Finally, the client will default to the normal behavior when receiving an
unknown NACK status code which is to note down that we've failed and re-extend
to the next intro point. This way, unknown status code won't trigger a
different behavior client side.
Part of #30454.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Remove the hs_intro_ack_status_t enum and move the value into trunnel. Only
use these values from now on in the intro point code.
Interestingly enough, the client side also re-define these values in hs_cell.h
with the hs_cell_introd_ack_status_t enum. Next commit will fix that and force
to use the trunnel ABI.
Part of #30454
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously we purged it in 1-hour increments -- but one-hour is the
maximum TTL for the cache! Now we do it in 25%-TTL increments.
Fixes bug 29617; bugfix on 0.3.5.1-alpha.
The client side had garbage histograms and deadcode here, too. That code has
been removed.
The tests have also been updated to properly test the intro circ by sending
padding from the relay side to the client, and verifying that both shut down
when padding was up. (The tests previously erroneously tested only the client
side of intro circs, which actually were supposed to be doing nothing).
This just moves the state transition directives into the proper client/relay
side functions. It also allows us to remove some dead-code from the client
side (since the client doesn't send padding).
- Add some more useful logs for future debugging.
- Stop usage of circpad_state_to_string(). It's innacurate.
- Reduce severity and fix up log domain of some logging messages.
To ease debugging of miscount issues, attach vanguards with --loglevel DEBUG
and obtain control port logs (or use any other control port CIRC and
CIRC_MINOR event logging mechanism).
If circuit padding wants to keep a circuit open and pathbias used to ignore
it, pathbias should continue to ignore it.
This may catch other purpose-change related miscounts (such as timeout
measurement, cannibalization, onion service circuit transitions, and
vanguards).
When a circuit is marked for close, check to see if any of our padding
machines want to take ownership of it and continue padding until the machine
hits the END state.
For safety, we also ensure that machines that do not terminate are still
closed as follows: Because padding machine timers are UINT32_MAX in size, if
some sort of network event doesn't happen on a padding-only circuit within
that time, we can conclude it is deadlocked and allow
circuit_expire_old_circuits_clientside() to close it.
If too much network activity happens, then per-machine padding limits can be
used to cease padding, which will cause network cell events to cease, on the
circuit, which will cause circpad to abandon the circuit as per the above time
limit.
We need to check here because otherwise we can try to schedule padding with no
tokens left upon the receipt of a padding event when our bins just became
empty.
Our other tests tested state lengths against padding packets, and token counts
against non-padding packets. This test checks state lengths against
non-padding packets (and also padding packets too), and checks token counts
against padding packets (and also non-padding packets too).
The next three commits are needed to make this test pass (it found 3 bugs).
Yay?
Since the reproducible RNG dumps its own seed, we don't need to do
it for it. Since tinytest can tell us if the test failed, we don't
need our own test_failed booleans.
This commit moves code that updates the state length and padding limit counts
out from the callback to its own function, for clarity.
It does not change functionality.
This commit moves the padding state limit checks and the padding rate limit
checks out of the token removal codepath, and causes all three functions to
get called from a single circpad_machine_count_nonpadding_sent() function.
It does not change functionality.
The code flow in theory can end up with a layer_hint to be NULL but in
practice it should never happen because with an origin circuit, we must have
the layer_hint.
Just in case, BUG() on it if we ever end up in this situation and recover by
closing the circuit.
Fixes#30467.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fortunately, in 0.3.5.1-alpha we improved logging for various
failure cases involved with onion service client auth.
Unfortunately, for this one, we freed the file right before logging
its name.
Fortunately, tor_free() sets its pointer to NULL, so we didn't have
a use-after-free bug.
Unfortunately, passing NULL to %s is not defined.
Fortunately, GCC 9.1.1 caught the issue!
Unfortunately, nobody has actually tried building Tor with GCC 9.1.1
before. Or if they had, they didn't report the warning.
Fixes bug 30475; bugfix on 0.3.5.1-alpha.
