Opened 3 months ago

Last modified 12 days ago

#31111 merge_ready defect

Properly support two padding machines per circuit

Reported by: pulls Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.3-alpha
Severity: Normal Keywords: wtf-pad circpad-researchers-want
Cc: asn, mikeperry, gaba Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

As part of circuitpadding.c, in circpad_add_matching_machines(), the macros FOR_EACH_CIRCUIT_MACHINE_BEGIN and SMARTLIST_FOREACH_REVERSE_BEGIN currently expand to a for loop each. Below a slightly cropped (...) version of circpad_add_matching_machines():

circpad_add_matching_machines(origin_circuit_t *on_circ,
                              smartlist_t *machines_sl)
{
  ...
  FOR_EACH_CIRCUIT_MACHINE_BEGIN(i) {
    ...
    SMARTLIST_FOREACH_REVERSE_BEGIN(machines_sl,
                                    circpad_machine_spec_t *,
                                    machine) {
        ...
        if (circpad_negotiate_padding(on_circ, machine->machine_num,
                                  machine->target_hopnum,
                                  CIRCPAD_COMMAND_START) < 0) {
          log_info(LD_CIRC, "Padding not negotiated. Cleaning machine");
          circpad_circuit_machineinfo_free_idx(circ, i);
          circ->padding_machine[i] = NULL;
          on_circ->padding_negotiation_failed = 1;
        } else {
          /* Success. Don't try any more machines */
          return;
        }
      }
    } SMARTLIST_FOREACH_END(machine);
  } FOR_EACH_CIRCUIT_MACHINE_END;
}

The outer loop goes over each machine index (currently 2, set by CIRCPAD_MAX_MACHINES), while the inner loop looks for a suitable machine for that index to negotiate. As soon as one is found and negotiated, currently, the function returns without looking for a machine for later indices in the outer loop. The return should be replaced by a break to continue looking for a machine for the next index.

See https://github.com/torproject/tor/pull/1168 for a PR.

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by teor

Status: newneeds_review

comment:2 Changed 3 months ago by nickm

Milestone: Tor: 0.4.2.x-final

comment:3 Changed 3 months ago by asn

I'd like for mike to take a look here. Do we want to add multiple machines in one circuit?

comment:4 Changed 2 months ago by dgoulet

Reviewer: mikeperry

comment:5 Changed 6 weeks ago by mikeperry

We do want to support negotiation of multiple machines on a single circuit. At a glance this looks ok. Are we sure we didn't make this mistake elsewhere in loops like this?

Tobias: Thanks for this. Are multiple machines per circuit working ok for you with this patch applied?

comment:6 Changed 6 weeks ago by mikeperry

Keywords: wtf-pad added
Status: needs_reviewneeds_information

comment:7 Changed 3 weeks ago by pulls

Sorry for the delay. I got multiple machines per circuit running with this fix and a proof-of-concept APE implementation in the circuit framework: https://github.com/pylls/tor/tree/circuit-padding-ape-machine. I looked at the logs from the client in detail. Currently looking at some other things that popped up, but I'm pretty sure that they're unrelated since I could reproduce without this change in place.

comment:8 Changed 12 days ago by mikeperry

Keywords: circpad-researchers-want added
Status: needs_informationmerge_ready

This looks good to go to me then.

Note: This does not affect the padding machines we deployed on the live network. This bug does not need backport, nor is it urgent. But it is a bug that researchers will encounter during development, so we should merge the fix to master.

comment:9 Changed 12 days ago by mikeperry

Summary: negotiate one machine per indexProperly support two padding machines per circuit

comment:10 Changed 12 days ago by mikeperry

Milestone: Tor: 0.4.2.x-finalTor: unspecified

comment:11 Changed 12 days ago by teor

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

Hi Mike,

Do you want this fix merged into 0.4.2 or 0.4.3 ?
For the moment, I'm going to assume 0.4.3, and put it in that milestone.

Please change if you want something different.

Note: See TracTickets for help on using tickets.