Opened 3 weeks ago

Closed 3 days ago

#30428 closed defect (fixed)

sendme: Failure to validate authenticated SENDMEs client side

Reported by: dgoulet Owned by: dgoulet
Priority: Very High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-circuit, sendme, 041-must, 0411-alpha, postfreeze-ok
Cc: Actual Points:
Parent ID: #26288 Points: 1
Reviewer: nickm Sponsor: SponsorV

Description

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.

  1. 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!

Child Tickets

Change History (17)

comment:1 Changed 3 weeks ago by dgoulet

Keywords: 041-must added
Status: assignedneeds_review

PR: https://github.com/torproject/tor/pull/1005
Branch: ticket30428_041_01

I spent WAY TOO much time on the big unit test I mentioned... Sending cells from one circuit to another is actually extremely hard in our unit tests framework so for now I've tabled that test.

This REALLY must go before 041 is stable.

comment:2 Changed 12 days ago by dgoulet

Reviewer: nickm

comment:3 Changed 12 days ago by nickm

The seems good to me but before we merge, let's talk about integration testing.

How do we know that it actually works on a live network? What integration tests did we _not_ do last time that would have helped us catch this?

comment:4 Changed 11 days ago by nickm

Status: needs_reviewneeds_revision

comment:5 Changed 10 days ago by dgoulet

Status: needs_revisionneeds_review

Finally pushed the revision. I had to rebase on latest master since the cpath layer was refactored to hide the relay_crypto_t object which basically made this branch _not_ work and complicated conflict to resolve. Thus the new PR.

With the chutney bidi branch from nickm, I confirm that this works properly now (the TIMEOUT = 3 needed to be changed to be able to transfer more than 5MB).

The SENDME v0 also still works properly. And I've tested with a network supporting and emitting only v1 with a client that only supports v0. And vice versa with a network only v0 with a client doing v1.

PR: https://github.com/torproject/tor/pull/1026
Branch: ticket30428_041_02

comment:6 Changed 10 days ago by nickm

Left some questions on the PR. IIUC, dgoulet is waiting for roger to comment more here. Let me know if that's wrong and I should move forward.

comment:7 Changed 9 days ago by nickm

Keywords: 0411-alpha added

comment:8 Changed 9 days ago by nickm

Roger says it's probably best not to wait for him here. dgoulet, please have a look at my comments on the PR and see what you think?

comment:9 Changed 9 days ago by nickm

Status: needs_reviewneeds_revision

comment:10 Changed 8 days ago by nickm

Keywords: postfreeze-ok added

comment:11 Changed 4 days ago by dgoulet

Status: needs_revisionneeds_review

Comments addressed.

comment:12 Changed 4 days ago by nickm

Status: needs_reviewmerge_ready

Looks good; how are the tests looking?

comment:13 in reply to:  12 Changed 4 days ago by dgoulet

Status: merge_readyneeds_review

Replying to nickm:

Looks good; how are the tests looking?

The new very small commit 4ef8470fa5480d3b actually broke things when I tested with the latest chutney bidi branch.

Turns out that we needed that minus 1 on the window. I explain why in the new commit. I've thus reverted 4ef8470fa5480d3b as well first and then new commit:

19c086365957dc93
  sendme: Clarify how sendme_circuit_cell_is_next() works [David Goulet]
6380a2f307ba8f7b
  Revert "sendme: Off by one on the SENDME window" [David Goulet]

This has been quite tested now just to find that issue that was not showing up reliably for unknown reasons on nickm's bidi chutney.

I confirm that the digests matches as expected on both sides.

comment:14 Changed 4 days ago by nickm

Thanks, I'll review.

One thing to test as we test -- are we doing this at the correct intervals? The tests prove that the intervals _match_ on both sides, but not that they match the spec. One way to verify might be to count how much data we have hashed, and make sure that it is the correct number of cells, if that makes sense.

comment:15 Changed 3 days ago by dgoulet

Ok! I've confirmed that we do always have a multiple of 509 (CELL_PAYLOAD_SIZE) and it is aligned on the window.

There is a edge case there where we record a cell digest of a non DATA cell if the window is on the SENDME limit. It is not a big problem because in that case, the window is not updated so the next DATA cell will properly get recorded. However, it makes us sometimes record a digest that we shouldn't. To fix that, we would need to pass down to the relay crypto layer the cell relay command so we ONLY record for data cells.

Two commits were added here. First one is fixing a bug that I discovered while stress testing with the chutney bidi where an Exit was not sending v1 as expected so the other end kept accumulating cell digest on the circuit. The commit takes care of removing the digest each time we get a SENDME.

Second commit adds two non fatal assert for cases that should never happen but in case they do, tor will scream loudly.

comment:16 Changed 3 days ago by nickm

LGTM. Squashed and merging!

comment:17 Changed 3 days ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.