Support group readable hidden service directories

If a Tor users wishes to use a system Tor with a user service they may encounter problems with not being able to utilize the hidden service due to directory permissions.

An option "HiddenServiceDirGroupReadable" similar to "ControlPortFileGroupReadable" and "CookieAuthFileGroupReadable" would resolve this issue.

Trac:
Username: anon

changed milestone to %Tor: 0.2.6.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.6.x-final in Legacy / Trac priority::medium in Legacy / Trac reporter::anon in Legacy / Trac resolution::implemented in Legacy / Trac status::closed in Legacy / Trac tor-hs in Legacy / Trac type::enhancement in Legacy / Trac labels

Some notes: check_private_dir() knows only of user privacy, not group and is used extensively to verify the permissions on hidden service directories and files.

A naive approach of changing the permissions after created/updated gets clobbered during other start-up process, and thus is not even a workable quick hack. See rend_config_services().

Trac:
Username: anon

Things still wrong with that patch: overriding perms after call to check_private_dir(), the CPD_GROUP_READ vs. CPD_GROUP_OK not elegantness, and the way check perms only checks for need to make more restrictive, rather than set to specific desired value if not just testing. Will provide a better patch before asking for review... once brain re-charged... nom nom brains.

Trac:
Username: anon

Trac:
Username: anon

test-11291-group-readable-hsdirs.patch

Trac:
Username: anon

Changed option name to remove mention of "Dir" as this also makes the hostname readable by group members. (But not the private_key).

default permissions for HS creation

drwx------ 2 anon anon 2014-03-24 04:19 hs_test -rw------- 1 anon anon 2014-03-24 04:19 hs_test/hostname -rw------- 1 anon anon 2014-03-24 04:19 hs_test/private_key

set HiddenServiceGroupReadable 1 and you get:

drwxr-x--- 2 anon anon 2014-03-24 04:19 hs_test -rw-r----- 1 anon anon 2014-03-24 04:19 hs_test/hostname -rw------- 1 anon anon 2014-03-24 04:19 hs_test/private_key

Trac:
Username: anon

The code looks plausible at first glance, and is on-schedule for possible inclusion early in 0.2.5.x. It really needs a test-case, if possible.

Trac:
Milestone: N/A to Tor: 0.2.6.x-final
Status: new to needs_review
Keywords: N/A deleted, tor-hs added

Yes, I will add a test case. Thanks Nick!

Trac:
Username: anon

Almost done with new test_checkdir.c to verify this and other options. (No prior test coverage for check_private_dir())

I still need to sync up with ioerror and confirm this fixes his gstreamer system Tor configuration. [or reproduce same myself, and confirm. TBD.]

Trac:
Username: anon

(r, sorry, I meant "early in 0.2.6.x". I hope that won't be too long now.)

Trac:
Username: anon

test-11291-group-readable-hsdirs-wtests.patch

Updated patch with tests and cleaned up constants.

Trac:
Username: anon

NOTE: Still cleaning up this patch. Do not merge the -wtests.patch.

Trac:
Username: anon

Trac:
Cc: N/A to meejah@meejah.ca

Trac:
Username: anon

test-11291-group-redable-hsdirs-wtests-may6.patch

Rebased on current master, fixed broken linkage to new checkdir tests, onward!

Trac:
Username: anon

Trac:
Username: anon

test-11291-group-redable-hsdirs-wtests-may8.patch

o/~ "What do the tests say?" - OK ... o/~

Trac:
Username: anon

This patch is now doing what it should, and the tests confirm. It would be nice to replace other existing magic stat constants (0700, etc.) with the new defines (STAT_RWXO, etc.) however that refactoring will go in another patch...

Trac:
Username: anon

Background: meejah and ioerror, others have issues with system Tor on Debian and Debian based distributions like Tails. The nature of this problem is that Hidden Service directories on the filesystem and hostname files themselves (but NOT private key files!)

The specific use case was to provide dissidents like Snowden a more privacy enhanced video streaming mechanism than Skype or other "trusted" third parties.

The hidden service directories and hostnames files are not group-read-able like the other options "ControlPortFileGroupReadable" and "CookieAuthFileGroupReadable" which can be used to launch and serve media, file archives, and other services via hidden service as the desktop or other user with Tor group membership.

The user only private key file behavior requires a unit test be implemented as of when this comment was written. In other words this patch is not yet ready to be merged until further tests are added.

Trac:
Username: anon

Quick review:

• Will the tests pass on windows? (Will they compile on windows?)
• Are symbolic constants really necessary here for the STAT_* stuff? (In theory, we already have standard symbolic constants as S_IRUSR, S_IRUSR, S_IXUSR.... but, does having them really make the code cleaner? Is STAT_RWXU|STAT_RGRP|STAT_XGRP really easier to read than 0750 ?)
• Our coding style is to write '} else {' on one line.
• The style changes at the start of rendservice.c don't have anything to do with the rest of the patch.
• The comment opening /** is only for doxygen comments.
• Should HiddenServiceGroupReadable be HiddenServiceDirGroupReadable ?
• check_private_dir() ensures that the directory has bits 0700 if CPD_CHECK_MODE_ONLY is not set. Shouldn't it also ensure that the directory has bits 0050 if CPD_CHECK_MODE_ONLY is not set, and CPD_GROUP_READ is set?
• Does it make sense for HiddenServiceGroupReadable to be a per-hidden-service option?

Trac:
Status: needs_review to needs_revision

From a controller's perspective, whether it's per-hidden-service or not, the controller needs to determine:

1. does Tor support the option?
2. are we in the right group?
3. is it turned on? (if it's per-hidserv we can just always do this while doing SETCONF)

Replying to anon:

The specific use case was to provide dissidents like Snowden a more privacy enhanced video streaming mechanism than Skype or other "trusted" third parties.

Another specific use case: so that would-be dissidents like Snowden can anonymously leak documents to anonymously hosted file storage infrastructure that has geographically distributed data redundancy and verified end to end crypto.

In other words: native Tor integration with Tahoe-LAFS:

https://tahoe-lafs.org/trac/tahoe-lafs/ticket/517 http://foolscap.lothar.com/trac/ticket/203

Is the resolution of this ticket blocked on anything in particular? I'd like to help out if something needs doing.

See the code review above: somebody needs to resolve those issues before the patch can be applied.

I made some progress cleaning up anon's patch: https://github.com/david415/tor/commit/aaf979e95d6de23bd777d9b6c9245d958a42f846

I'll try to work on this some more soon.

I think it does make sense for HiddenServiceDirGroupReadable to be a per-hidden-service option.

Greetings,

I believe my branch https://github.com/david415/tor/commits/david-tortrac-11291-fu1 satisfies nickm's code review except for these two issues:

• Will the tests pass on windows? (Will they compile on windows?)
• Does it make sense for HiddenServiceGroupReadable to be a per-hidden-service option?

I will look into making HiddenServiceDirGroupReadable a per-hidden-service option... unless someone speaks up and explains why this should not be done.

I would like someone to help with the windows compatibility; I don't do windows.

If I understand things correctly HiddenServiceDirGroupReadable would have to change to CONFIG_TYPE_LINELIST or CONFIG_TYPE_LINELIST_S so that it can be listed more than once... that is what I got from looking at confparse.c... however I think that is only for torrc config files. For the control port interface i don't yet know...

Is the above patch + windows compatibility good enough?

I (believe I) have made the option per-hidden-service and fixed a couple minor typos. The unit-tests appear to run, and even have some plausible coverage. I didn't write a unit-test for the config-parsing logic.

See https://github.com/meejah/tor/tree/ticket-11291-david-rebase

Is it easier if I attach a patch to this ticket? I haven't really used git-format-patch but can certainly try!

On windows: there's no need to support windows with this: our other group-permission stuff doesn't do windows either. So it's okay to make test_checkdir_perms() only happen on non-Windows.

Other notes: It looks like CPD_GROUP_OK no longer does what it used to? Previously, CPD_CHECK_MODE_ONLY|CPD_GROUP_OK would complain if the permissions didn't match 0027. Now it complains if they're not in 0077. There's a similar issue with CPD_GROUP_OK without CPD_CHECK_MODE_ONLY.

The indentation in rend_config_services doesn't match the rest of Tor.

There are some misc whitespace-rules violations: 'make check-spaces' will tell you where.

Oh, one more thing: HiddenServiceDirGroupReadable has no more reason to exist in or_options_t.

OK... I have tried to address the regression, whitespace-rules other things from the code review.

Changes here: https://github.com/david415/tor/tree/ticket-11291-david-rebase

Trac:
Status: needs_revision to needs_review

Identified a couple of issues on irc, most prominent one being an error where we're always trying to change permissions on the directory if CPD_CHECK_MODE_ONLY is not set.

Trac:
Status: needs_review to needs_revision

OK... I think I fixed it. Got rid of the convoluted code.

https://github.com/david415/tor/tree/ticket-11291-david-rebase

Trac:
Status: needs_revision to needs_review

Last request, I hope: could the unit tests please be made to test the changes from 72030408 and b59fd2ef ? If they didn't catch this error in the first place, it would be great to make sure that they can catch it now.

Trac:
Status: needs_review to needs_revision

David asked me to review branch ticket-11291-extra-utests. It looks good to me.

I would maybe document a bit more the mask stuff in check_private_dir() because it was not obvious to me what the mask is for and why it was not needed before:

  if (check & (CPD_GROUP_OK|CPD_GROUP_READ)) {
    mask = 0027;
  } else {
    mask = 0077;
  }

Oh! There are unit tests now! This probably doesn't belong in needs_revision.

Trac:
Status: needs_revision to needs_review

(Apparently the unit tests are in meejah's repository.)

I added one test to the ones that dawuud had made. This test covered at least one case that failed in 72030408 but not in b59fd2ef

https://github.com/meejah/tor/tree/ticket-11291-extra-utests

I don't know why the mask is there either.

Merged and added a cleanup commit. thanks, everyone!

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

But see legacy/trac#13678 (moved) -- the tests fail for me.

closed

mentioned in issue legacy/trac#13678 (moved)

moved from legacy/trac#11291 (moved)

added Feature label and removed 1 deleted label

added 1 deleted label and removed 1 deleted label