protover.c accepts arbitrary bytes in protocol names

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.

Trac:
Username: cyberpunks

changed milestone to %Tor: 0.3.5.x-final

added 029-backport 032-backport 033-backport 034-backport component::core tor/tor milestone::Tor: 0.3.5.x-final owner::cyberpunks priority::medium protover reporter::cyberpunks resolution::fixed reviewer::teor severity::normal status::closed type::defect version::tor 0.2.9.4-alpha labels

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

Trac:
Milestone: N/A to Tor: unspecified

Trac:
Status: new to accepted
Owner: N/A to rl1987

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

Trac:
Username: cyberpunks

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

Trac:
Owner: rl1987 to cyberpunks
Status: accepted to assigned

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to teor

Can this make it to 0.3.5?

Trac:
Username: cyberpunks

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.

Trac:
Status: needs_review to needs_revision
Keywords: unicode deleted, N/A added
Milestone: Tor: unspecified to Tor: 0.3.5.x-final

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

Trac:
Username: cyberpunks

Trac:
Status: needs_revision to needs_review

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

Trac:
Status: needs_review to merge_ready

merged to 0.2.9 and forward!

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

moved to tpo/core/tor#27316 (closed)