Tor does not complain if the user specifies two HSes with the same HiddenServiceDir

changed milestone to %Tor: 0.2.2.x-final

added component::core tor/tor milestone::Tor: 0.2.2.x-final owner::rransom priority::medium resolution::fixed status::closed tor-hs type::defect labels

Specifying a single ‘client name’ seems to work, but specifying two or more causes Tor to regenerate the HS's keys on every SIGHUP, which defeats the purpose entirely.

Oops.

Trac:
Summary: ‘HiddenServiceAuthorizeClient stealth foo, bar’ generates new HS keys on every SIGHUP to Tor does not complain if the user specifies two HSes with the same HiddenServiceDir
Priority: critical to normal

An idea for a fix that would do for 0.2.2.x is in branch bug3289 in my repository. It has a few issues as noted in an XXX that I added, but I feel that we shouldn't get fancy here with directory locking or something like that for 0.2.2.x.

Trac:
Status: new to needs_review

I'm okay leaving this as O(n^2) in 0.2.2.x, but having every FS be case-insensitive everywhere seems questionable to me. Even the operating systems with case-insensitive file-systems have only some of their file-systems case-insensitive.

To see whether two directories are equal ... hooboy.

On unixy systems, it's not too hard. Try to stat them. If they have st_dev and st_ino fields equal, then they are equal.

On windows, we can use use GetFileInformationByHandle, and see whether dwVolumeSerialNumber, nFileIndexHigh, and nFileIndexLow are equal.

Hm. I'm tempted to just say, "Let's check with strcmp() alone, and deal with this stuff through real locking later on."

See bug3289-022 ( git://git.torproject.org/rransom/tor.git bug3289-022 ) for a fixup commit to make the comparison case-sensitive (so it won't break existing, valid configurations with two HS directories differing only in case).

I'm not at all convinced that this is a common error, either, but it does seem worth warning about when we can detect it easily.

I don't object to this general direction. I think rransom's idea of doing an exact cmp is better, if we're not doing stat or lock files.

That said,

    SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr,
                      dupe = !strcmp(ptr->directory, service->directory));
    if (dupe) {

is only going to check if the last entry in rend_service_list matches, right?

ugh. adding fixup.

Looks fine. squashing and merging.

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

Replying to nickm:

Hm. I'm tempted to just say, "Let's check with strcmp() alone, and deal with this stuff through real locking later on."

The ‘use real locking later’ ticket is #3317 (moved).

Trac:
Keywords: N/A deleted, tor-hs added

Trac:
Component: Tor Hidden Services to Tor

closed

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