Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:48:49Zhttps://gitlab.torproject.org/legacy/trac/-/issues/32666BUG: Non-fatal assertion info failed in onion_extend_cpath at src/core/or/cir...2020-06-13T15:48:49ZcypherpunksBUG: Non-fatal assertion info failed in onion_extend_cpath at src/core/or/circuitbuild.c:2663. (Stack trace not available) (on Tor 0.4.1.5 439ca48989ece545)Got this in orbot After startup.
```
NOTICE: Bootstrapped 100% (done): Done
Ignoring start request, already started.
WARN: tor_bug_occurred_: Bug: src/core/or/circuitbuild.c:2663: onion_extend_cpath: Non-fatal assertion info failed. (on...Got this in orbot After startup.
```
NOTICE: Bootstrapped 100% (done): Done
Ignoring start request, already started.
WARN: tor_bug_occurred_: Bug: src/core/or/circuitbuild.c:2663: onion_extend_cpath: Non-fatal assertion info failed. (on Tor 0.4.1.5 439ca48989ece545)
WARN: Bug: Non-fatal assertion info failed in onion_extend_cpath at src/core/or/circuitbuild.c:2663. (Stack trace not available) (on Tor 0.4.1.5 439ca48989ece545)
WARN: Failed to find node for hop #2 of our path. Discarding this circuit.
NOTICE: Our circuit 0 (id: 15) died due to an invalid selected path, purpose General-purpose client. This may be a torrc configuration issue, or a bug.
```
Orbot 16.1.2rc2Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/32614hs-v3: Consider flagging an intro point as bad if rendezvous fails multiple t...2020-06-13T15:48:37ZDavid Gouletdgoulet@torproject.orghs-v3: Consider flagging an intro point as bad if rendezvous fails multiple timesThis comes from #25882.
Should a client claim that an IP is failing if the RP never connected? In other words, handling the `CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED` purpose when cleaning up a circuit.
I would think that having a maxi...This comes from #25882.
Should a client claim that an IP is failing if the RP never connected? In other words, handling the `CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED` purpose when cleaning up a circuit.
I would think that having a maximum amount of retries let say (arbitrarily) after 2 times that you introduce, the IP sends back the ACK but then you end up timing out the RP circuit, we would flag the IP in the failure cache and move on to a new IP.
This would not be a "catch all" issues with connectivity but would at least zone out buggy or malicious IP that do not relay the introduction.Tor: 0.4.4.x-finalNeel Chauhanneel@neelc.orgNeel Chauhanneel@neelc.orghttps://gitlab.torproject.org/legacy/trac/-/issues/32343circ: Add a HANDLE() to circuit_t2020-06-13T15:47:41ZDavid Gouletdgoulet@torproject.orgcirc: Add a HANDLE() to circuit_tOverall better to start using handles instead of map lookup by GID which only works for origin circuit.
A HANDLE in the `circuit_t` object would allow us for instance to put in the circuit reference into a pubsub message instead of its ...Overall better to start using handles instead of map lookup by GID which only works for origin circuit.
A HANDLE in the `circuit_t` object would allow us for instance to put in the circuit reference into a pubsub message instead of its raw pointer that could be freed during a mainloop event preceding the pubsub callback(s).Tor: 0.4.3.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/32021hs-v3: Handle rendezvous client circuit build expire properly2020-06-13T15:46:26ZDavid Gouletdgoulet@torproject.orghs-v3: Handle rendezvous client circuit build expire properlyThis is a subtask of the bigger larger problem in #25882.
In `circuit_expire_building()`, we have this code path:
```
if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {
switch (victim->purpose) {
case CIRCUIT_PU...This is a subtask of the bigger larger problem in #25882.
In `circuit_expire_building()`, we have this code path:
```
if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {
switch (victim->purpose) {
case CIRCUIT_PURPOSE_C_REND_READY:
/* We only want to spare a rend circ if it has been specified in
* an INTRODUCE1 cell sent to a hidden service. A circ's
* pending_final_cpath field is non-NULL iff it is a rend circ
* and we have tried to send an INTRODUCE1 cell specifying it.
* Thus, if the pending_final_cpath field *is* NULL, then we
* want to not spare it. */
if (TO_ORIGIN_CIRCUIT(victim)->build_state &&
TO_ORIGIN_CIRCUIT(victim)->build_state->pending_final_cpath ==
NULL)
break;
```
Basically, this `pending_final_cpath` is only used by v2 which means v3 is not handle in that case.
And that case is: if we want to expire a rendezvous client circuit that is ready but has been waiting for a while on the introduction circuit as in its cookie has been sent in the `INTRODUCE1`, we want to spare it until the intro point client circuit collapses.
Because v3 is not handled in the above, rendezvous circuit will be tagged as timed out with the general cutoff instead of being kept until the intro circuit is ready or times out. And we time out intro circuit being established much later than an established rendezvous circuit for which the `general_cutoff` will be applied on.
Bottom line is that we need a flag within the rendezvous client circuit (probably hs_ident_t?) that its cookie was put in the INTRO1 cell and that we are waiting on the intro side signalling the `circuit_expire_building()` that it should wait more on that circuit.Tor: 0.4.3.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/31652hs-v3: Service circuit retry limit should not close a valid circuit2020-06-13T15:45:19ZDavid Gouletdgoulet@torproject.orghs-v3: Service circuit retry limit should not close a valid circuitContext: Lets say a service has 3 intro circuits opened and stable.
Now, imagine one circuit collapses, like for instance the intro point restarted "tor" after an update. Our code is designed to retry 3 times that is once every second a...Context: Lets say a service has 3 intro circuits opened and stable.
Now, imagine one circuit collapses, like for instance the intro point restarted "tor" after an update. Our code is designed to retry 3 times that is once every second and then give up on the intro point.
What it looks like:
Every second, `run_build_circuit_event()` is run and launches intro circuit(s) if we are missing any. For each IP, it will increment the `circuit_retries` counter but does not actually look at it to decide to launch or not.
Before that event, also every 1 second, `cleanup_intro_points()` checks that every intro point has not expired, fell off the consensus or that `circuit_retries` is greater than (>) `MAX_INTRO_POINT_CIRCUIT_RETRIES = 3`.
Putting this together, imagine now that the first 3 attempts failed for whatever reasons and then we launch a 4th one because `circuit_retries = 3`, it does pass validation of `> MAX_INTRO_POINT_CIRCUIT_RETRIES` so then a circuit is launched and that very one succeeds.
Because `circuit_retries` is now 4 then the next second, `cleanup_intro_points()` removes the IP and closes the valid open established circuit...
I've observed the above a surprising amount of time because booting a tor relay takes some seconds mostly due to the diff-cache parsing.
In a nutshell, we should NOT launch a circuit if we reached the max retries for an intro point. This back and forth of opening a circuit and then deciding that we went over the limit makes no sense in the first place.Tor: 0.4.2.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/31609Make CIRCUIT_IS_ORIGIN() look at the base magic number2020-06-13T15:45:01ZDavid Gouletdgoulet@torproject.orgMake CIRCUIT_IS_ORIGIN() look at the base magic numberCurrently, `CIRCUIT_IS_ORIGIN()` actually looks at the purpose, not the base magic number:
```
#define CIRCUIT_IS_ORIGIN(c) (CIRCUIT_PURPOSE_IS_ORIGIN((c)->purpose))
```
We should move it to look at the `magic` like `CIRCUIT_IS_ORCIRC(...Currently, `CIRCUIT_IS_ORIGIN()` actually looks at the purpose, not the base magic number:
```
#define CIRCUIT_IS_ORIGIN(c) (CIRCUIT_PURPOSE_IS_ORIGIN((c)->purpose))
```
We should move it to look at the `magic` like `CIRCUIT_IS_ORCIRC()` is doing.
The reason is because I was adding tracing events to the circuit subsystem and I kept having state transition event with a circuit global identifier of 0 which can't be because that value is set just after allocation.
But at that point, the purpose has not been set so `CIRCUIT_IS_ORIGIN()` wasn't returning true.
Furthermore, this made me discover another issue documented in #31608 where if we do make this change, we _must_ fix this ticket else we have a NULL deref.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/31608circuit_state_publish() never triggers when a new origin circuit is created2020-06-13T15:45:01ZDavid Gouletdgoulet@torproject.orgcircuit_state_publish() never triggers when a new origin circuit is createdIn `origin_circuit_init()`, we change the circuit state before allocating the `build_state` but also before a purpose is set.
This means that `circuit_state_publish()` located in `circuit_set_state()` is never called for a new circuit b...In `origin_circuit_init()`, we change the circuit state before allocating the `build_state` but also before a purpose is set.
This means that `circuit_state_publish()` located in `circuit_set_state()` is never called for a new circuit because `CIRCUIT_IS_ORIGIN()` doesn't return true.
Which in turn, by chance I believe, made this NULL deref on `build_state` to never happen.
This should be fixed regardless.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/30428sendme: Failure to validate authenticated SENDMEs client side2020-06-13T15:41:23ZDavid Gouletdgoulet@torproject.orgsendme: Failure to validate authenticated SENDMEs client sideTurns out that we have two issues with `sendme_is_valid()` (new authenticated SENDMEs).
1. We can not fallback onto version 0 if the version of the cell is unrecognized. Right now, if let say we have a minimum version (from consensus) o...Turns out that we have two issues with `sendme_is_valid()` (new authenticated SENDMEs).
1. We can not fallback onto version 0 if the version of the cell is unrecognized. Right now, if let say we have a minimum version (from consensus) of 1 and then we support version 3 but we get version 4, then ultimately we will end up in defaulting to version 0. Not good.
There needs to be a strong check on what we can minimally support (from consensus) and the upper bound of what we support. Anything outside of that range, the circuit has to be closed.
2. This one is a bit more bad. Basically, `sendme_process_circuit_level()` needs to validate the SENDME for both client and service. SENDMEs authenticate both ways and thus can not only be on service side like it is right now.
In other words, we need to call `sendme_is_valid()` in both cases that is if we are origin circuit or not.
Now that we have the unit test predictable fast prng feature, we should really add a tests that makes sure this entire logic works by sending 100 cells and expecting a SENDME validation.
Thanks to armadev's review for spotting those big issues!Tor: 0.4.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/29196circ: Remove p_mux and n_mux from circuit_t and or_circuit_t2020-06-13T15:37:14ZDavid Gouletdgoulet@torproject.orgcirc: Remove p_mux and n_mux from circuit_t and or_circuit_tThey are simply not used apart from assigning a pointer and asserting on the pointer depending on direction.
Complexity that is not needed.They are simply not used apart from assigning a pointer and asserting on the pointer depending on direction.
Complexity that is not needed.Tor: 0.4.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/24011Attempt to open a stream on first hop of circuit2020-06-13T15:26:24ZteorAttempt to open a stream on first hop of circuitI'm seeing these warnings in chutney on recent tor releases.
They seem to have appeared in the last week or two.
Did we merge something that tries to block single-hop exits recently?
Or any changes to circuit creation code?
Because I t...I'm seeing these warnings in chutney on recent tor releases.
They seem to have appeared in the last week or two.
Did we merge something that tries to block single-hop exits recently?
Or any changes to circuit creation code?
Because I think it might be buggy.
```
Warning: Attempt by 127.0.0.1:59488 to open a stream on first hop of circuit. Closing. Number: 1
Warning: circuit_receive_relay_cell (backward) failed. Closing. Number: 1
```Tor: 0.3.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/26259Don't count 0-length RELAY_DATA cell as valid2020-06-13T15:26:09ZMike PerryDon't count 0-length RELAY_DATA cell as validPointed out by Roger: The CIRC_BW OVERHEAD field currently counts a DATA cell on a valid stream but with 0 length as valid but 100% overhead. Since this can be injected arbitrarily by an end point, and should not happen in normal operati...Pointed out by Roger: The CIRC_BW OVERHEAD field currently counts a DATA cell on a valid stream but with 0 length as valid but 100% overhead. Since this can be injected arbitrarily by an end point, and should not happen in normal operation, it should not be counted as valid circuit data.Tor: 0.3.4.x-finalMike PerryMike Perryhttps://gitlab.torproject.org/legacy/trac/-/issues/26214Check stream SENDME against max2020-06-13T15:26:01ZMike PerryCheck stream SENDME against maxIn connection_edge_process_relay_cell() under the RELAY_COMMAND_SENDME handling, we check the circuit-level sendme against the window START_MAX, but we do not check the stream level SENDME against any max
This means that an attacker can...In connection_edge_process_relay_cell() under the RELAY_COMMAND_SENDME handling, we check the circuit-level sendme against the window START_MAX, but we do not check the stream level SENDME against any max
This means that an attacker can send as many stream-level sendme's on a circuit as they like, inflating the stream window as large as they like. This might be a serious OOM bug, but the circuit level SENDME check should prevent that, I think.
Even so, it would be nice to fix this in 0.3.4, so that the vanguards script's detection of invalid/dropped circuit data is more accurate.Tor: 0.3.4.x-finalMike PerryMike Perryhttps://gitlab.torproject.org/legacy/trac/-/issues/25657prop249: make sure that we incorporate fragmented extend[ed]2 cells in our OO...2020-06-13T15:23:49ZNick Mathewsonprop249: make sure that we incorporate fragmented extend[ed]2 cells in our OOM codeTor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25656Fuzzing code for prop249 (wide creates)2020-06-13T15:23:49ZNick MathewsonFuzzing code for prop249 (wide creates)We should make sure that all our wide create and fragmented extend logic is designed in such a way as to be fuzzable, and then we should fuzz it.We should make sure that all our wide create and fragmented extend logic is designed in such a way as to be fuzzable, and then we should fuzz it.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25655Integration testing of prop2492020-06-13T15:23:48ZNick MathewsonIntegration testing of prop249Once prop249 is implemented, our Chutney tests and our test network should use it so that we can find out if it has bugs.Once prop249 is implemented, our Chutney tests and our test network should use it so that we can find out if it has bugs.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25654Create a testing-only handshake for shaking the bugs out of wide create cells...2020-06-13T15:23:47ZNick MathewsonCreate a testing-only handshake for shaking the bugs out of wide create cells (prop249)We need a handshake that doesn't fit in a single CREATE cell so that we can make sure that prop249 is implemented properly.
My current recommendation is "nnttoorr", which is the same as ntor, but every byte is repeated twice. ;)We need a handshake that doesn't fit in a single CREATE cell so that we can make sure that prop249 is implemented properly.
My current recommendation is "nnttoorr", which is the same as ntor, but every byte is repeated twice. ;)Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25653prop249: advertise support correctly in protover subsystem; only use when pro...2020-06-13T15:23:47ZNick Mathewsonprop249: advertise support correctly in protover subsystem; only use when protover support advertisedIn the link handshake and in the protover list, we should advertise support for CREATE2V and CREATED2V.
In the protover list, we should advertise support for fragmented EXTEND cells.
In the protover list, we should advertise support fo...In the link handshake and in the protover list, we should advertise support for CREATE2V and CREATED2V.
In the protover list, we should advertise support for fragmented EXTEND cells.
In the protover list, we should advertise support for our temporary testing handshake.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25652Prop249: set RELAY_EARLY bit correctly on fragmented EXTEND cells; enforce it...2020-06-13T15:23:46ZNick MathewsonProp249: set RELAY_EARLY bit correctly on fragmented EXTEND cells; enforce it correctly.The RELAY_EARLY bit gets a tiny bit more complicated with prop249; let's make sure we implement it correctly.The RELAY_EARLY bit gets a tiny bit more complicated with prop249; let's make sure we implement it correctly.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25651Handle incoming extend2/extended2 fragmented requests/replies. (prop249)2020-06-13T15:23:46ZNick MathewsonHandle incoming extend2/extended2 fragmented requests/replies. (prop249)As part of prop249, we need to handle large create cells when they arrive fragmented across multiple extend cells (and similarly for created cells fragmented across multiple extended cells).
Important notes:
* The proposal says that t...As part of prop249, we need to handle large create cells when they arrive fragmented across multiple extend cells (and similarly for created cells fragmented across multiple extended cells).
Important notes:
* The proposal says that there must be no intervening cells on the same circuit. We should enforce this and test it.
* We should probably use a buf_t object to record these handshakes as they are being assembled.
* We should count these handshakes against the memory usage of a circuit and age of the oldest queued data, so that they will participate correctly in the OOM system.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/25650Handle incoming create2v / created2v cells (wide create cells)2020-06-13T15:23:45ZNick MathewsonHandle incoming create2v / created2v cells (wide create cells)We need to receive create2v/created2v cells, and respond to them by performing or finishing the corresponding onion handshake.We need to receive create2v/created2v cells, and respond to them by performing or finishing the corresponding onion handshake.Tor: unspecified