Opened 6 weeks ago

Closed 4 weeks ago

#23420 closed defect (implemented)

prop224: Pad RENDEZVOUS1 cell to match legacy cell length

Reported by: asn Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, prop224-extra, tor-hs
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description (last modified by dgoulet)

We should do what proposal 224 section 4.3 says and pad rend cells to 168 bytes so that they all look the same.

Child Tickets

Change History (15)

comment:1 Changed 5 weeks ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:2 Changed 5 weeks ago by dgoulet

Description: modified (diff)
Summary: prop224: Pad legacy rendezvous cells to match v3 cellsprop224: Pad RENDEZVOUS1 cell to match legacy cell length

comment:3 Changed 5 weeks ago by dgoulet

Status: acceptedneeds_review

ESTABLISH_RENDEZVOUS cell is exactly the same size and payload for v2 and v3.

RENDEZVOUS1 is the one that is different between v2 (168 bytes) and v3 (84 bytes).

Branch: ticket23420_032_01

Last edited 5 weeks ago by dgoulet (previous) (diff)

comment:5 Changed 4 weeks ago by asn

OK. Based on the thread from comment:4 :

  • Ian pointed out that just padding with random bytes is not the right approach since the g^y field of the legacy cell has certain algebraic structure that will be missing from the random pad. We could in theory add intelligent padding that matches the algebraic structure, but kinda overkill perhaps.
  • teor pointed out that there are other distinguishers between v2 and v3, like whether the service extends with tap vs ntor.

We could do naive padding in v3, but only with the hope that it would obfuscate future cells (v4, etc.).

Based on the above I have no strong opinions here, and I'm fine with either doing nothing, or doing naive padding.

Last edited 4 weeks ago by asn (previous) (diff)

comment:6 Changed 4 weeks ago by nickm

So, IIUC, the "algebraic structure" in question here is just that real values of gy will always be between 2 and p-1 (inclusive). But since p is very close to 21024, nearly all values will pass that test. (Random junk will fail with probability approximately 2-66.)

The tap/ntor argument is stronger.

On the other hand, just padding with random bytes costs us almost nothing.

comment:7 Changed 4 weeks ago by asn

WRT the tap/ntor argument, Roger mentioned that even legacy onion services should extend using ntor these days, since ntor has been around since 0.2.4 or so.

There might be some legacy services that still use tap, but probably not many.

This might invalidate the tap/ntor argument.

Are we right on this?

comment:8 Changed 4 weeks ago by nickm

The difficulty with using ntor is that there is no way in the legacy protocol for clients to tell services the ntor key of the RP.

Services could look up the RP's ntor key in the directory information they have, though. This would reveal two things:

  • Whether the service has the RP in its directory info
  • Whether the service is running a version that is new enough to do this.

I'm not sure how bad those two leaks are.

comment:9 Changed 4 weeks ago by dgoulet

The padding is very cheap to do although I think the current code takes the raw bytes from our PRNG so we might not want to expose that to the RP directly? The crypto_strongest_rand() is the one hashing the bytes but could be heavier?

Considering the padding will in most cases pass the test, seems a win-win to just do it.

comment:10 Changed 4 weeks ago by nickm

Don't use crypto_strongest_rand() unless you need an extra hash bytes straight from the operating system.

Personally, I believe that the "don't expose PRNG bytes to the world" wisdom is bad, but if you want to avoid it, pass 32 bytes of crypto_rand() output through SHAKE?

Last edited 4 weeks ago by nickm (previous) (diff)

comment:11 Changed 4 weeks ago by asn

Padding sounds good to me.

I'd be a bit enthusiastic if we also had unittests to ensure that cell parsing is right even tho there is padding at the end.

comment:12 Changed 4 weeks ago by nickm

Status: needs_reviewneeds_revision

comment:13 Changed 4 weeks ago by asn

Status: needs_revisionneeds_review

Please check my branch ticket23420_032_01 for David's padding feature, plus a unittest that tests the parsing of REND1/REND2 cells.

This is our first cell-parsing unittest so it's important stuff and it should increase test coverage!

Cheers!

comment:14 Changed 4 weeks ago by asn

Status: needs_reviewmerge_ready

comment:15 Changed 4 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

lgtm; tests pass. merging!

Note: See TracTickets for help on using tickets.