Opened 3 months ago

Closed 2 months ago

#27316 closed defect (fixed)

protover.c accepts arbitrary bytes in protocol names

Reported by: cyberpunks Owned by: cyberpunks
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.4-alpha
Severity: Normal Keywords: protover, 029-backport, 032-backport, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

dir-spec.txt defines a protocol name as a Keyword, and strictly limits what character set is allowed in a Keyword:

    Keyword = KeywordChar+
    KeywordChar ::= 'A' ... 'Z' | 'a' ... 'z' | '0' ... '9' | '-'

But "Foo_Bar=1", ",,,=1", and arbitrary Unicode strings like "Risqu\u00e9=1" are accepted. Bytes that aren't even valid Unicode like "\xc1=1" are also fine, as long as no bytes are the null byte, =, or the space character.

Child Tickets

TicketStatusOwnerSummaryComponent
#27687closedrust protover accepts non ASCII in protocol namesCore Tor/Tor

Change History (12)

comment:1 in reply to:  description Changed 3 months ago by teor

Milestone: Tor: unspecified

Thanks for finding this bug.

Replying to cyberpunks:

dir-spec.txt defines a protocol name as a Keyword, and strictly limits what character set is allowed in a Keyword:

    Keyword = KeywordChar+
    KeywordChar ::= 'A' ... 'Z' | 'a' ... 'z' | '0' ... '9' | '-'

But "Foo_Bar=1", ",,,=1", and arbitrary Unicode strings like "Risqu\u00e9=1" are accepted.

We can safely reject descriptors, votes, and consensuses containing non-keyword characters.

Bytes that aren't even valid Unicode like "\xc1=1" are also fine, as long as no bytes are the null byte, =, or the space character.

Tor doesn't do any Unicode checks on directory documents yet.
(More precisely, when built without Rust, tor doesn't do any Unicode checks. When built with Rust, protover does Unicode checks.)
Here's a proposal for consistently checking Unicode in directory documents:
https://gitweb.torproject.org/torspec.git/tree/proposals/285-utf-8.txt

comment:2 Changed 3 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 3 months ago by cyberpunks

See the protokeyword1 branch at https://gitgud.io/onionk/tor.git

comment:4 Changed 3 months ago by rl1987

Owner: changed from rl1987 to cyberpunks
Status: acceptedassigned

Nice progress! I'll set you as an owner of this ticket then.

comment:5 Changed 3 months ago by teor

Status: assignedneeds_review

comment:6 Changed 2 months ago by dgoulet

Reviewer: teor

comment:7 in reply to:  6 Changed 2 months ago by cyberpunks

Can this make it to 0.3.5?

comment:8 Changed 2 months ago by teor

Keywords: unicode removed
Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: needs_reviewneeds_revision

Can this make it to 0.3.5?

The 0.3.5 feature freeze is today (Friday 14 September).

But bug fixes are not features, so we review them and backport them to previous releases as needed.

I tried reviewing this branch, but it seems to contain a whole bunch of fixes. Most of the fixes have other tickets. Can you provide one branch per fix, with no other fixes?

If there are dependencies, can you say which branches depend on other branches, so that I know which branches to review first?

Also, we try not to use tor_assert() for non-fatal bugs, because it terminates the process. Instead, we use tor_assert_nonfatal(), or if(BUG()) { /* action on failure */ }.

This branch doesn't merge cleanly into master, I think because some of the fixes have already been merged to master. Can you rebase this branch on the latest maint-0.2.9?

I opened a pull request for this branch on 0.2.9 here:
https://github.com/torproject/tor/pull/332

Once the merge conflicts are fixed, we can merge to 0.3.4 to get Appveyor CI as well.

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

Oops. Rebased, was only supposed to have one commit there.

comment:10 Changed 2 months ago by nickm

Status: needs_revisionneeds_review

comment:11 Changed 2 months ago by teor

Status: needs_reviewmerge_ready

This looks fine to me, and the CI passed at:
https://github.com/torproject/tor/pull/332

comment:12 Changed 2 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to 0.2.9 and forward!

Note: See TracTickets for help on using tickets.