Opened 8 years ago

Closed 6 years ago

#4341 closed defect (implemented)

MyFamily Option Requires a Dollar Sign "$"

Reported by: marlowe Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.34
Severity: Keywords: easy configuration tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The MyFamily option requires a dollar sign "$" for the fingerpint.. Tor should not require this for the fingerprint.

To duplicate, place a valid fingerprint without the preceding dollar sign. Attempt to start Tor. Tor will fail to start, complaining of an invalid configuration option.

Child Tickets

Change History (13)

comment:1 Changed 8 years ago by nickm

Keywords: easy configuration added
Milestone: Tor: unspecified

comment:2 Changed 8 years ago by Sebastian

Status: newneeds_review

Branch bug4341 in my repo

comment:3 Changed 8 years ago by arma

Looks good to me.

When somebody merges, they should be sure to fix https://www.torproject.org/docs/faq#MultipleRelays too.

comment:4 Changed 8 years ago by nickm

Ooh, dangerous!

is_legal_nickname_or_hexdigest gets called from a lot of places. One is in config.c in check_nickname_list, which seems okay. Another is when validating MyFamily in router.c, which is also fine.

But it's also used in rendservice.c to handle nicknames from introduce2 cells, and in routerparse.c to validate family lines there!

We don't want to change the behavior of parsing family lines in introduce2 cells, routers, or microdescriptors. If we did that, you'd be able to make descriptors that new Tors would accept as valid, but older Tors wouldn't.

Also, I don't see anything that transforms fingerprints without a "$" into ones with a "$" before adding them to the Family line in router.c. That makes for trouble, since older Tors don't know how to handle such fingerprints when they appear on the Family line.

What needs to happen here is that the new looser validation logic can only apply in config.c and router.c, where we're checking the user-supplied input. The other users of is_legal_nickname_or_hexdigest() need to stay unchanged. Any digests provided without a "$" need to get a "$" added to them before adding them to the family line.

comment:5 Changed 8 years ago by Sebastian

Owner: set to Sebastian
Status: needs_reviewaccepted

oho!

comment:6 Changed 8 years ago by Sebastian

Not sure if I'll get to this. But I can't also seem to un-accept/un-assign it. Boo. If someone wants to take this, please do

comment:7 in reply to:  description Changed 8 years ago by rransom

Owner: Sebastian deleted
Status: acceptedassigned

Replying to marlowe:

The MyFamily option requires a dollar sign "$" for the fingerpint.. Tor should not require this for the fingerprint.

Why should Tor not require the leading “$” in a fingerprint? That's part of the syntax for specifying a relay using its fingerprint.

comment:8 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:9 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:10 Changed 6 years ago by Ry

Check my branch below to see if the fix suits. I'm 'fixing' the MyFamily line as a side effect of options_validate/check_nickname_list so other areas that depend on it being well formed should not need to be changed. (e.g. router.c reparses the line straight from the active options so it picks up the fixed value)

https://github.com/Ryman/tor/tree/bug4341

comment:11 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final
Status: assignedneeds_revision

Quick notes:

  • We never use strcat or strcpy. Instead, in this case, I'd recommend tor_asprintf().
  • Is the cast to (char * *) still needed after the changed function signature?
  • For loops this long, I recommend SMARTLIST_FOREACH_BEGIN and SMARTLIST_FOREACH_END.

Otherwise, this looks okay. (As for the earlier question, IMO it doesn't matter much here whether we change the behavior or improve the error message in this case.)

comment:12 Changed 6 years ago by Ry

Commit diff:
https://github.com/Ryman/tor/commit/db318dc77f47157d685fdd9e2035c96574b283db

Branch:
https://github.com/Ryman/tor/tree/bug4341

-1: Noted.
-2: Yep, it's superfluous from before changing the sig.
-3: Done.

comment:13 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Great; merging this (after some compilation warning fixes). Thanks again!

Note: See TracTickets for help on using tickets.