Opened 12 months ago

Last modified 10 days ago

#27191 needs_revision defect

handling double spaces 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: rust, 029-backport, 032-unreached-backport, 035-deferred-20190115, 041-proposed, 033-unreached-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

protover.c accepts trailing spaces and extra spaces between subprotocol entries like "Link=1-4 LinkAuth=1 ", but rejects leading spaces like " Link=1-4". It has since its introduction.

The Rust implementation rejects all extra spaces in any position. It's at least consistent.

Child Tickets

Change History (15)

comment:1 Changed 12 months ago by cyberpunks

The protospaces2 branch at ​​https://gitgud.io/onionk/tor.git changes the C implementation to reject them. Nothing actually produces extra spaces like this, right?

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

Status: newneeds_review

Replying to cyberpunks:

The protospaces2 branch at ​​https://gitgud.io/onionk/tor.git changes the C implementation to reject them. Nothing actually produces extra spaces like this, right?

Extra spaces are not allowed by the protover spec. Each separator is exactly one space:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n780

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

comment:3 Changed 12 months ago by teor

Keywords: 029-backport 032-backport 033-backport 034-backport added
Milestone: Tor: 0.3.5.x-final

comment:4 Changed 12 months ago by teor

Status: needs_reviewneeds_information

We could also update the protover spec to allow any number of spaces and tabs (WS), rather than a single space (SP), and update the C and Rust implementations to match.

That would be more consistent with Tor's directory document meta-format:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n193

I'd like to get nickm's opinion before we move forward.

comment:5 Changed 12 months ago by teor

Also, I'm not sure how you're finding all these protover bugs, but you might be interested in protover fuzzing in #24265. Someone already tried in #26265, but they didn't do it using Tor's existing fuzzing infrastructure, and we needed to be able to fuzz the rust and C functions in the same process.

comment:6 Changed 12 months ago by teor

Cc: nickm added

If I cc nickm, I'm more likely to get his opinion.

comment:7 in reply to:  5 ; Changed 11 months ago by cyberpunks

Visual inspection of the code made them pretty apparent.

Also, it's been 3 weeks now. Is this gonna get merged before Friday?

Version 0, edited 11 months ago by cyberpunks (next)

comment:8 in reply to:  7 Changed 11 months ago by teor

Cc: nickm removed
Reviewer: nickm
Status: needs_informationneeds_review

Replying to cyberpunks:

Visual inspection of the code made them pretty apparent.

Also, it's been 3 weeks now. Is this gonna get reviewed before Friday?

We aim to review code within a week, but we've been busy with the 0.3.4 release:
https://lists.torproject.org/pipermail/tor-announce/2018-September/000164.html
And the 0.3.5 code freeze:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

If it helps, you're not the only person waiting for code reviews:
https://trac.torproject.org/projects/tor/query?status=needs_review&component=Core+Tor%2FTor&col=id&col=summary&col=status&col=type&col=priority&col=milestone&col=component&order=priority

I'm still waiting on nickm's opinion on spec vs code changes:

Replying to teor:

We could also update the protover spec to allow any number of spaces and tabs (WS), rather than a single space (SP), and update the C and Rust implementations to match.

That would be more consistent with Tor's directory document meta-format:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n193

I'd like to get nickm's opinion before we move forward.

I'll try setting him as reviewer on this ticket instead.

comment:9 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

Sorry for the delay -- I do think we should use the metaformat from the rest of the spec, and change the spec to WS here.

comment:10 Changed 9 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:11 Changed 7 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:12 Changed 6 months 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:13 Changed 6 months ago by teor

Keywords: 033-backport-unreached added

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

comment:14 Changed 2 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:15 Changed 10 days ago by teor

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

Fix 033-unreached-backport spelling.

Note: See TracTickets for help on using tickets.