Opened 4 months ago

Last modified 4 weeks ago

#27194 needs_revision defect

handling extra commas in protover

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.4-alpha
Severity: Normal Keywords: rust, security-low, 029-backport, 033-backport, 034-backport, 032-unreached-backport
Cc: Actual Points:
Parent ID: Points:
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
#27197merge_readyrust protover accepts excess commas in version stringsCore Tor/Tor

Change History (17)

comment:1 Changed 4 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 4 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 4 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 4 months ago by cyberpunks

Yes, please review.

Rust protover did not exist until 0.3.3, so filed #27197 for that change.

Last edited 4 months ago by cyberpunks (previous) (diff)

comment:5 Changed 4 months ago by teor

Status: needs_informationneeds_review

comment:6 Changed 3 months ago by dgoulet

Reviewer: ahf

comment:7 Changed 3 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 3 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 3 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 3 months ago by cyberpunks (previous) (diff)

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

What revision is needed here now?

comment:11 Changed 3 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 2 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 2 months ago by ahf

comment:14 Changed 2 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 2 months ago by ahf

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

comment:16 in reply to:  15 Changed 2 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 weeks 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.

Note: See TracTickets for help on using tickets.