Opened 7 months ago

Last modified 11 days ago

#27194 needs_revision defect

Reject protover extra commas in protover

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.9.4-alpha
Severity: Normal Keywords: needs-consensus-method, rust, security-low, 029-backport, 034-backport, 035-backport, 040-backport, 041-backport, 032-unreached-backport, 035-deferred-20190115, 041-proposed, 033-backport-unreached
Cc: Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: ahf Sponsor:

Description

Like how it handles spaces, protover.c rejects leading commas (Link=,1-5 or Link=,) while it accepts and ignores extra commas elsewhere (Link=1-5, and Link=1,,,2-5 are valid).

The Rust version accepts and ignores all extra commas, including leading commas.

Child Tickets

TicketStatusOwnerSummaryComponent
#27197closedrust protover accepts excess commas in version stringsCore Tor/Tor

Change History (22)

comment:1 Changed 7 months ago by teor

Keywords: security-low added
Milestone: Tor: 0.3.6.x-final

I'm not sure if we should fix the rust version to match the C version, or fix the C version.

If we do fix the C version, we may need to define a new consensus method. So I'm scheduling this for merge in 0.3.6, because new consensus methods are complicated.

comment:2 in reply to:  1 Changed 7 months ago by teor

Milestone: Tor: 0.3.6.x-finalTor: 0.3.5.x-final

Replying to teor:

I'm not sure if we should fix the rust version to match the C version, or fix the C version.

If we do fix the C version, we may need to define a new consensus method. So I'm scheduling this for merge in 0.3.6, because new consensus methods are complicated.

Oh, hang on, we can just reject descriptors with extra commas in the C code. We're allowed to reject descriptors as malformed without a new consensus method.

comment:3 Changed 7 months ago by teor

Keywords: 029-backport 032-backport 033-backport 034-backport added
Status: newneeds_information

Is your protocommas1 branch ready for review?

If so, we should run make test-network-all using ​chutney to make sure that standard tor clients aren't inserting extra spaces.

comment:4 Changed 7 months ago by cyberpunks

Yes, please review.

Rust protover did not exist until 0.3.3, so created the rust-protocommas1 branch for that, based on maint-0.3.3. To have separate changes files, filed #27197 for that change.

Version 1, edited 7 months ago by cyberpunks (previous) (next) (diff)

comment:5 Changed 7 months ago by teor

Status: needs_informationneeds_review

comment:6 Changed 6 months ago by dgoulet

Reviewer: ahf

comment:7 Changed 6 months ago by ahf

The patch can be found here: https://gitgud.io/onionk/tor/commit/c5879adda90c7f3be6d2408f74bb1e0fd2854c50

I think this looks reasonable, but maybe someone who has deep knowledge of the protover code can say a final yes or no to this? I'm a bit unsure of the consequences of this.

comment:8 Changed 6 months ago by ahf

Status: needs_reviewneeds_revision

Just chatted with Nick on IRC about this. Can't this patch be simplified into: s = comma + 1; and then let the outer while (s < end_of_entry) take care of whether we are done processing the entry?

comment:9 in reply to:  8 ; Changed 6 months ago by cyberpunks

[]Replying to ahf:

Just chatted with Nick on IRC about this. Can't this patch be simplified into:

It actually can't. That's exactly what the first draft of this patch was, but that would ignore a trailing comma. That's why the comment was there.

But oh actually the if condition can be safely simplified a little bit.

    // Skip the comma separator between ranges. Don't ignore a trailing comma.
    if (s < (end_of_entry - 1))
      s++;
Last edited 6 months ago by cyberpunks (previous) (diff)

comment:10 in reply to:  9 Changed 6 months ago by cyberpunks

What revision is needed here now?

comment:11 Changed 6 months ago by teor

Status: needs_revisionneeds_review

See protocommas1 at https://gitgud.io/onionk/tor.git for the original patch and a fixup.

comment:12 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

Commits can be seen here:

https://gitgud.io/onionk/tor/commits/protocommas1

I think this looks OK given the situation with trailing commas.

comment:13 Changed 5 months ago by ahf

comment:14 Changed 5 months ago by ahf

Status: merge_readyneeds_revision

Looks like this patch breaks the Rust builds:

https://travis-ci.org/torproject/tor/builds/440701286

comment:15 Changed 5 months ago by ahf

Maybe we need to merge #27197 and this at the same time?

comment:16 in reply to:  15 Changed 5 months ago by cyberpunks

The child ticket has to be merged first. Rust tests don't test C code, C tests do test Rust code.

comment:17 Changed 4 months ago by teor

Keywords: 032-unreached-backport added; 032-backport removed

0.3.2 is end of life, so 032-backport is now 032-unreached-backport.

comment:18 Changed 2 months ago by nickm

Keywords: 035-deferred-20190115 041-proposed added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

Marking a number of 0.3.5 tickets as possible, maybe even a good idea, for later. Possibly backportable, some of them. But not currently things to do as part of 0.3.5 stabilization.

comment:19 in reply to:  15 ; Changed 7 weeks ago by cypherpunks3

Replying to ahf:

Maybe we need to merge #27197 and this at the same time?

Given that this is true, why is this 'needs_revision'?

comment:20 Changed 3 weeks ago by teor

Keywords: 033-backport removed

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

comment:21 Changed 3 weeks ago by teor

Keywords: 033-backport-unreached added

Hmm, I guess they should still get 033-backport-unreached

comment:22 in reply to:  19 Changed 11 days ago by teor

Actual Points: 0.2
Keywords: needs-consensus-method 035-backport 040-backport 041-backport added
Points: 0.2
Summary: handling extra commas in protoverReject protover extra commas in protover

Here's the impact of this change:

  • protover_compute_vote() may start producing a different consensus when it encounters patters that used to be accepted, because it ignores recommended/required lines that it considers malformed. This change requires a new consensus method, or rejecting the entire vote.
  • protover_compute_vote() calls contract_protocol_list(), which calls parse_single_entry(), but only on entries that have already been parsed and rejected. So the BUG() should be impossible.
  • parse_protocol_list() starts rejecting votes and descriptors that contain patterns that used to be accepted. That's ok.

Replying to cypherpunks3:

Replying to ahf:

Maybe we need to merge #27197 and this at the same time?

Given that this is true, why is this 'needs_revision'?

The ticket was marked needs_revision because the it broke the rust builds on CI, and when that happened, it was unclear how to fix it. Then, it stayed in needs_revision because no-one created working branches for 0.2.9 and 0.3.3.

Now 0.3.3 is not maintained any more, so here are some draft branches for 0.2.9 and 0.3.4:

0.2.9, which has no rust:
https://github.com/torproject/tor/pull/768

0.3.4, including the rust fix from #27197, which is already in 0.3.5 and later:
https://github.com/torproject/tor/pull/769

This change impacts the consensus, so we need to do one of these things:

  1. define a new consensus method, create two versions of parse_single_entry(), and call the fixed version only when creating a consensus with the new consensus method, or
  2. in networkstatus_parse_vote_from_string(), reject votes that have malformed:
    • recommended/required_client/server_protocols
    • relay protocol lines, as parsed by routerstatus_parse_entry_from_string()

When we make this change, we'll be introducing a version distinguisher. That's ok for most directory users, because the authorities check their documents. But bridge clients can be probed by their bridges to see if they have this fix. We might need to accept that risk.

Note: See TracTickets for help on using tickets.