Alexander Hansen Færøy authored 2 years ago and David Goulet committed 1 year ago

See: #33669.

Alexander Hansen Færøy authored 3 years ago and David Goulet committed 1 year ago

This patch forces a PT reconfigure of infant PT processes as part of the
PT process' exit handler.

See: #33669

Alexander Hansen Færøy authored 3 years ago and David Goulet committed 1 year ago

See: #33669

Alexander Hansen Færøy authored 3 years ago and David Goulet committed 1 year ago

See: #33669

Alexander Hansen Færøy authored 3 years ago and David Goulet committed 1 year ago

This patch ensures that we can figure out which PT that terminated in
the PT exit handler.

See: #33669

Alexander Hansen Færøy authored 3 years ago and David Goulet committed 1 year ago

This patch makes Tor log state transitions within the PT layer at the
info log-level. This should make it easier to figure out if Tor ends up
in a strange state.

See: #33669

Signed-off-by: David Goulet <dgoulet@torproject.org>

Signed-off-by: David Goulet <dgoulet@torproject.org>

Micah Elizabeth Scott authored 1 year ago and David Goulet committed 1 year ago

This started as a response to ticket #40792 where Coverity is
complaining about a potential year 2038 bug where we cast time_t from
approx_time() to uint32_t for use in token_bucket_ctr.

There was a larger can of worms though, since token_bucket really
doesn't want to be using wallclock time here. I audited the call sites
for approx_time() and changed any that used a 32-bit cast or made
inappropriate use of wallclock time. Things like certificate lifetime,
consensus intervals, etc. need wallclock time. Measurements of rates
over time, however, are better served with a monotonic timer that does
not try and sync with wallclock ever.

Looking closer at token_bucket, its design is a bit odd because it was
initially intended for use with tick units but later forked into
token_bucket_rw which uses ticks to count bytes per second, and
token_bucket_ctr which uses seconds to count slower events. The rates
represented by either token bucket can't be lower than 1 per second, so
the slower timer in 'ctr' is necessary to represent the slower rates of
things like connections or introduction packets or rendezvous attempts.

I considered modifying token_bucket to use 64-bit timestamps overall
instead of 32-bit, but that seemed like an unnecessarily invasive change
that would grant some peace of mind but probably not help much. I was
more interested in removing the dependency on wallclock time. The
token_bucket_rw timer already uses monotonic time. This patch converts
token_bucket_ctr to use monotonic time as well. It introduces a new
monotime_coarse_absolute_sec(), which is currently the same as nsec
divided by a billion but could be optimized easily if we ever need to.

This patch also might fix a rollover bug.. I haven't tested this
extensively but I don't think the previous version of the rollover code
on either token bucket was correct, and I would expect it to get stuck
after the first rollover.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

Micah Elizabeth Scott authored 1 year ago and David Goulet committed 1 year ago

This adds a bit more to hs_descriptor/test_decode_descriptor, mostly
testing pow-params and triggering the tor_assert() in issue #40793.

There was no mechanism for adding arbitrary test strings to the
encrypted portion of the desc without duplicating encode logic. One
option might be to publicize get_inner_encrypted_layer_plaintext enough
to add a mock implementation. In this patch I opt for what seems like
the simplest solution, at the cost of a small amount of #ifdef noise.
The unpacked descriptor grows a new test-only member that's used for
dropping arbitrary data in at encode time.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

agowa338 authored 1 year ago and David Goulet committed 1 year ago

Fixes #40785

Signed-off-by: David Goulet <dgoulet@torproject.org>

The MR was using an old function definition so the code fix is for that.

Closes #40546

Signed-off-by: David Goulet <dgoulet@torproject.org>

The descriptor validation table had an out of date minimum length
for pow-params (3) whereas the spec and the current code expect at
least 4 parameters. This was an opportunity for a malicious service
to cause an assert failure in clients which attempted to parse its
descriptor.

Addresses issue #40793

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

This should fix one of the warnings in issue #40792.

I was sloppy with freeing memory in the failure cases for
test_crypto_hashx. ASAN didn't notice but coverity did. Okay, I'll eat
my vegetables and put hashx_ctx's deinit in an upper scope and use
'goto done' correctly like a properly diligent C programmer.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

This addresses one of the warnings in issue #40792. As far as I can tell
this is a false positive, since the use of "ctx->type" in hashx_free()
can only be hit after the unioned code/program pointer is non-NULL. It's
no big deal to zero this value explicitly to silence the warning though.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

Switch rate limiting will likely be helpful for limiting OOQ, but according to
shadow it was the cause of slower performance in Hong Kong endpoints.

So let's disable it, and then optimize for OOQ later.

Maxrate had slower throughput than lowrtt in Shadow, which is not too
surprising. We just wanted to test it.

This is a protocol breaking change that implements nickm's
changes to prop 327 to add an algorithm personalization string
and blinded HS id to the EquiX challenge string for our onion
service client puzzle.

This corresponds with the spec changes in torspec!130,
and it fixes a proposed vulnerability documented in
ticket tor#40789.

Clients and services prior to this patch will no longer
be compatible with the proposed "v1" proof-of-work protocol.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

This lets controller apps see the outgoing PoW effort on client
circuits, and the validated effort received on an incoming service
circuit.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

This error path with the "PoW cpuworker returned with no solution.
Will retry soon." message was usually lying. It's concerning
now because we expect to always find a solution no matter how
long it takes, rather than re-enter the solver repeatedly, so any
exit without a solution is a sign of a problem.

In fact when this error path gets hit, we are usually missing a
circuit instead because the request is quite old and the circuits
have been destroyed. This is not an emergency, it's just a sign
of client-side overload.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>