prop224: Pad RENDEZVOUS1 cell to match legacy cell length

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

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor milestone::Tor: 0.3.2.x-final owner::dgoulet points::1 priority::medium prop224 prop224-extra resolution::implemented severity::normal status::closed tor-hs type::defect labels

Trac:
Owner: N/A to dgoulet
Status: new to accepted

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

to

We should do what proposal 224 section 4.3 says and pad rend cells to 168 bytes so that they all look the same.
Summary: prop224: Pad legacy rendezvous cells to match v3 cells to prop224: Pad RENDEZVOUS1 cell to match legacy cell length

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

Trac:
Status: accepted to needs_review

Awaiting inputs from more people: https://lists.torproject.org/pipermail/tor-dev/2017-September/012451.html

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.

So, IIUC, the "algebraic structure" in question here is just that real values of g^y^ will always be between 2 and p-1 (inclusive). But since p is very close to 2^1024^, 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.

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?

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.

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.

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?

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.

Trac:
Status: needs_review to needs_revision

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!

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to merge_ready

lgtm; tests pass. merging!

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed

changed time estimate to 8h

moved to tpo/core/tor#23420 (closed)