Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11291 closed enhancement (implemented)

Support group readable hidden service directories

Reported by: anon Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: meejah@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (4)

test-11291-group-readable-hsdirs.patch (6.3 KB) - added by anon 5 years ago.
test-11291-group-readable-hsdirs-wtests.patch (7.8 KB) - added by anon 5 years ago.
Updated patch with tests and cleaned up constants.
test-11291-group-redable-hsdirs-wtests-may6.patch (13.4 KB) - added by anon 5 years ago.
Rebased on current master, fixed broken linkage to new checkdir tests, onward!
test-11291-group-redable-hsdirs-wtests-may8.patch (14.0 KB) - added by anon 5 years ago.
o/~ "What do the tests say?" - OK ... o/~

Download all attachments as: .zip

Change History (37)

comment:1 Changed 5 years ago by anon

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().

comment:2 Changed 5 years ago by 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.

Changed 5 years ago by anon

comment:3 Changed 5 years ago by 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

comment:4 Changed 5 years ago by nickm

Keywords: tor-hs added
Milestone: Tor: 0.2.6.x-final
Status: newneeds_review

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.

comment:5 Changed 5 years ago by anon

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

comment:6 Changed 5 years ago by 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.]

comment:7 Changed 5 years ago by nickm

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

Changed 5 years ago by anon

Updated patch with tests and cleaned up constants.

comment:8 Changed 5 years ago by anon

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

comment:9 Changed 5 years ago by meejah

Cc: meejah@… added

Changed 5 years ago by anon

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

Changed 5 years ago by anon

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

comment:10 Changed 5 years ago by 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...

Last edited 5 years ago by anon (previous) (diff)

comment:11 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

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?

comment:13 Changed 5 years ago by meejah

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)

comment:14 in reply to:  11 Changed 5 years ago by dawuud

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

comment:15 Changed 5 years ago by dawuud

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

comment:16 Changed 5 years ago by nickm

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

comment:17 Changed 5 years ago by dawuud

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.

comment:18 Changed 5 years ago by dawuud

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.

comment:19 Changed 5 years ago by dawuud

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?

comment:20 Changed 5 years ago by meejah

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!

Last edited 5 years ago by meejah (previous) (diff)

comment:21 Changed 5 years ago by nickm

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.

comment:22 Changed 5 years ago by nickm

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

comment:23 Changed 5 years ago by dawuud

Status: needs_revisionneeds_review

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

comment:24 Changed 5 years ago by Sebastian

Status: needs_reviewneeds_revision

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.

comment:25 Changed 5 years ago by dawuud

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

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

comment:26 Changed 5 years ago by dawuud

Status: needs_revisionneeds_review

comment:27 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

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

comment:28 Changed 5 years ago by asn

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;
  }

comment:29 Changed 5 years ago by nickm

Status: needs_revisionneeds_review

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

comment:30 Changed 5 years ago by nickm

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

comment:31 Changed 5 years ago by meejah

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

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

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

comment:32 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged and added a cleanup commit. thanks, everyone!

comment:33 Changed 5 years ago by nickm

But see #13678 -- the tests fail for me.

Note: See TracTickets for help on using tickets.