Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#26860 closed defect (fixed)

Spec: decryption order appears to be wrongly specified

Reported by: dmr Owned by: dmr
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, prop295-assigned-sponsor-v
Cc: catalyst, asn, teor, atagar Actual Points:
Parent ID: #26869 Points:
Reviewer: Sponsor: SponsorV

Description

catalyst noted in #tor-dev that the decryption algorithm in 5.5:

  • won't work unless decryption is commutative (which it is for plain AES-CTR, but not for the proposal)
  • doesn't look like it'll work to handle a relay cell arriving at the OP that originates from an intermediate node

I had also noted the latter, so I postulated is the order of that line backwards? that is... should it be "For I=1...N?" and asked what does little-t tor do?

And asn noted:

in relay_decrypt_cell() it does:

   if (CIRCUIT_IS_ORIGIN(circ)) { /* We're at the beginning of the circuit.
                                    * We'll want to do layered decrypts. */
      crypt_path_t *thishop, *cpath = TO_ORIGIN_CIRCUIT(circ)->cpath;
      thishop = cpath;

i think this is I=1...N like you say
(for 1 being the hop closest to the OP)

So it looks like this is an error in the spec, rather than a problem with tor.

Child Tickets

TicketStatusOwnerSummaryComponent
#26859closedteorSpec: improve contextual description of EXTEND->CREATE / CREATED->EXTENDED handling and payloadsCore Tor/Tor

Change History (11)

comment:1 Changed 14 months ago by dmr

Cc: teor atagar added
Owner: set to dmr
Status: newassigned

Assigning to self - I'll take care of this right away (the patch is super-easy).

comment:2 Changed 14 months ago by dmr

Status: assignedneeds_review

PR here:
https://github.com/torproject/torspec/pull/27
branch head c551c2dc07785cc96468a079b1b9a77ddb1e17b0

comment:3 Changed 14 months ago by nickm

Milestone: Tor: 0.3.5.x-final

comment:4 Changed 14 months ago by teor

Parent ID: #26869
Status: needs_reviewmerge_ready

Looks good to me!

I'm just about to modify this section as part of our prop#295 review, so I'll cherry-pick your commit to #26869 before my commits.

comment:5 Changed 14 months ago by teor

Status: merge_readyneeds_review

Hi dmr,

Thanks for this patch.

It turns out that whole section was confusing, because it tried to handle the forward and backward directions at the same time.

Please see my branch 26860-decryption-order at https://github.com/teor2345/torspec.git , which covers this ticket and #26859, because the edits overlapped.

comment:6 Changed 14 months ago by catalyst

Status: needs_reviewmerge_ready

The reordering change looks good! I made another comment on the commit for #26859. Also updated the pull request branch at https://github.com/torproject/torspec/pull/27

comment:7 in reply to:  6 ; Changed 14 months ago by teor

Replying to catalyst:

The reordering change looks good! I made another comment on the commit for #26859.

Thanks, I pushed a fixup to my 26860-decryption-order branch.

Also updated the pull request branch at https://github.com/torproject/torspec/pull/27

That's a neat trick!
I can't work out how to add my fixup commit to dmr's pull request.
Can you point me to the documentation?
(I tried looking for it, but I couldn't find it.)

comment:8 in reply to:  7 Changed 14 months ago by teor

Replying to teor:

Replying to catalyst:

...

That's a neat trick!
I can't work out how to add my fixup commit to dmr's pull request.
Can you point me to the documentation?
(I tried looking for it, but I couldn't find it.)

Oh wow, can I just push to dmr's branch?
So that's what "allow maintainer updates" means.

comment:9 Changed 14 months ago by nickm

LGTM too; squashed and merged to torspec master. Please close or unparent the child ticket as appropriate, and then close this one?

comment:10 Changed 14 months ago by catalyst

Resolution: fixed
Status: merge_readyclosed

comment:11 Changed 14 months ago by teor

Keywords: prop295-assigned-sponsor-v added
Sponsor: SponsorV

I think all prop#295 tickets are sponsor V.

Note: See TracTickets for help on using tickets.