Opened 9 months ago

Closed 6 weeks ago

#29669 closed defect (fixed)

hs: ADD_ONION with NEW:BEST is still pinned on v2

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.5.1-alpha
Severity: Normal Keywords: tor-hs, tor-control, hs-v3, tor-spec, security, 041-deferred-20190530 042-should
Cc: Actual Points: 0.1
Parent ID: #29995 Points: 1
Reviewer: asn Sponsor: Sponsor27-must

Description

Even though the control spec says:

  (The "NEW:BEST" option obeys the HiddenServiceVersion torrc option default
  value. Since 0.3.5.1-alpha, it is 3. For Tor versions before 0.3.5.1-alpha,
  default HiddenServiceVersion is 2.)

... in control.c, the ADD_ONION command has this condition that basically pins the NEW:BEST to v2:

    if (!strcasecmp(key_type_rsa1024, key_blob) ||
        !strcasecmp(key_type_best, key_blob)) {
      /* "RSA1024", RSA 1024 bit, also currently "BEST" by default. */

Not good! NEW:BEST should obey the default version, not something hardcoded like so. This will need a spec update to mention the correct tor version.

Child Tickets

Change History (12)

comment:1 Changed 6 months ago by nickm

Keywords: security added

comment:2 Changed 6 months ago by nickm

Keywords: 041-deferred-20190530 added

Marking these tickets as deferred from 041.

comment:3 Changed 6 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

comment:4 Changed 2 months ago by nickm

Keywords: 042-should added

comment:5 Changed 2 months ago by dgoulet

Parent ID: #29995
Sponsor: Sponsor27-must

Falls under s27 for feature parity and stability.

comment:6 Changed 2 months ago by dgoulet

Actual Points: 0.1
Reviewer: asn
Status: newneeds_review

PR: https://github.com/torproject/tor/pull/1317
Branch: ticket29669_042_01

comment:7 Changed 2 months ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

comment:8 Changed 2 months ago by dgoulet

Status: acceptedneeds_review

comment:9 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Patch LGTM but the changes file ends in a weird semicolon instead of a colon.
Also we need a spec file and we should be good. Marking as needs_revision for the above.

comment:10 Changed 7 weeks ago by dgoulet

Status: needs_revisionneeds_review

Spec: ticket29669_01

I've also fixed the PR to be on latest master and remove that weird ; in the changes file.

comment:11 Changed 6 weeks ago by asn

Status: needs_reviewmerge_ready

LGTM! Thanks!

comment:12 Changed 6 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged both branches.

Note: See TracTickets for help on using tickets.