Opened 16 months ago

Last modified 8 months ago

#27198 new defect

protover doesn't forbid version zero

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.9.4-alpha
Severity: Normal Keywords: doc, fast-fix, reviewer-was-teor-20190422
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

dir-spec.txt says:

Int = NON_ZERO_DIGIT
Int = Int DIGIT

But the code in protover.c allows Foo=0.

Child Tickets

TicketStatusOwnerSummaryComponent
#27201needs_informationrust/protover doesn't forbid version zeroCore Tor/Tor

Change History (9)

comment:1 Changed 16 months ago by cyberpunks

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

comment:2 Changed 16 months ago by nickm

Milestone: Tor: 0.3.5.x-final
Status: newneeds_review

comment:3 Changed 16 months ago by teor

Cc: nickm added

I think we might want to fix the spec to permit 0 instead.
What do you think, nickm?

Also, I think that it's ok to accept "01" and treat it as "1", but authorities should not put leading zeroes in the consensus.

comment:4 Changed 16 months ago by nickm

I agree with you on both counts.

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

Problem: it looks like there's already code that depends on there being no version 0, in protover_all_supported().

comment:6 in reply to:  5 ; Changed 16 months ago by teor

Status: needs_reviewneeds_information

Replying to cyberpunks:

Problem: it looks like there's already code that depends on there being no version 0, in protover_all_supported().

Can you be more specific?

Which code allows 0 versions, and which code doesn't?
And what happens in the vote and the consensus when a 0 version is specified by a relay?

comment:7 in reply to:  6 Changed 14 months ago by teor

Reviewer: teor

Replying to teor:

Replying to cyberpunks:

Problem: it looks like there's already code that depends on there being no version 0, in protover_all_supported().

Can you be more specific?

Which code allows 0 versions, and which code doesn't?
And what happens in the vote and the consensus when a 0 version is specified by a relay?

We can't resolve the spec issues, #27201, and #27198 until these questions are answered.

Also, this branch seems to have a lot of unrelated changes in it.
Can you please cherry-pick the "zero" changes on top of master?

If you can't do these things, just let us know, and we will do them eventually.

comment:8 Changed 9 months ago by teor

Keywords: doc fast-fix added
Milestone: Tor: 0.3.5.x-finalTor: unspecified
Status: needs_informationnew

We should fix this issue by updating the spec to forbid a protocol version of zero.

And maybe we should check that there are tests that make sure zeroes fail.

comment:9 Changed 8 months ago by teor

Keywords: reviewer-was-teor-20190422 added
Reviewer: teor

If these tickets go back in to needs_review, and I am on leave, they will need another reviewer.

Note: See TracTickets for help on using tickets.