Opened 3 years ago

Closed 2 years ago

#21779 closed defect (fixed)

Non-admin users can't access Tor Browser on macOS

Reported by: teor Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201705R
Cc: brade, mcs, gk, boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The TorBrowser.app directory is distributed in the macOS disk image with permissions set to 0750. When it is copied to /Applications, the Finder sets the user to the admin user that did the copy, and the group to 'admin'.

This means that non-admin users can't read or execute Tor Browser.

Historically, this was a feature, because potentially sensitive Tor Browser data was stored in the application bundle. But now that data is stored in ~/Library/Application Support or next to the app, it's a bug.

Child Tickets

Change History (13)

comment:1 Changed 2 years ago by mcs

Cc: brade mcs added

Kathy and I were able to reproduce this bug. As you point out, it should be fine to allow rx permission to "other." Doing something like:

chmod -R o+rX TorBrowser.app

during our packaging should fix this problem.

comment:2 in reply to:  1 Changed 2 years ago by gk

Replying to mcs:

Kathy and I were able to reproduce this bug. As you point out, it should be fine to allow rx permission to "other." Doing something like:

chmod -R o+rX TorBrowser.app

during our packaging should fix this problem.

I am fine taking a fix for the next alpha (alpha because I'd like to test it with an update first and see what is happening).

comment:3 Changed 2 years ago by gk

Keywords: TorBrowserTeam201704 added

comment:4 Changed 2 years ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201704 removed

Moving our tickets to May 2017.

comment:5 Changed 2 years ago by mcs

Cc: gk added
Owner: changed from tbb-team to mcs
Status: newassigned

comment:6 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201705R added; TorBrowserTeam201705 removed
Status: assignedneeds_review

Here is a patch:
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug21779-01&id=1472ef020fb709226bda7cf234a42f3b928c08ed

Unfortunately, we could not reproduce the original problem in the test builds that Kathy and I made. We are not sure why not. Is it possible that a repackaging step that occurs after the initial dmg is produced is changing file permissions somehow, e.g., during OSX code signing?

comment:7 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Hm, maybe. The patch looks good to me, though, and I took it for master (commit f27d2cab4ed80a4c7b4f594b593b6b90f6148a82). I'll close the ticket for now and keep an eye on this while signing. I doubt I get to a test signature before building the alphas. If there is indeed an issue with the signing setup I hope I can fix it ad hoc.

comment:8 Changed 2 years ago by mcs

Resolution: fixed
Status: closedreopened

Unfortunately, the file permissions in the final 7.0a4 disk images are still incorrect:

ls -ld /Volumes/Tor\ Browser/TorBrowser.app
drwxr-x---  3 user  staff  2048 Apr 19 05:15 /Volumes/Tor Browser/TorBrowser.app/

ls -l /Volumes//Tor\ Browser/TorBrowser.app/Contents/MacOS/firefox
-rwxr-x---  1 user  staff  28256 May 15 04:27 /Volumes//Tor Browser/TorBrowser.app/Contents/MacOS/firefox*

Interestingly, the files within the complete mar do have the correct mode, e.g., 755 for Contents/MacOS/firefox. Where are the post-gitian build release procedures documented? I cannot find the page.

comment:9 Changed 2 years ago by gk

Do you mean https://gitweb.torproject.org/tor-browser-spec.git/tree/processes/ReleaseProcess?
I doubt this matters. What looks more promising is our ddmg.sh script: https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/ddmg.sh. See:

if [ "z$DATA_OUTSIDE_APP_DIR" = "z1" ]; then
  EXE_MODE=0755
  OTHER_MODE=0644
else
  EXE_MODE=0750
  OTHER_MODE=0640
fi
find $@ -executable -exec chmod $EXE_MODE {} \;
find $@ ! -executable -exec chmod $OTHER_MODE {} \;

for instance. I should have thought about that earlier. :(

comment:10 Changed 2 years ago by mcs

As I implied in comment:6, files within the disk images produced by my own gitian-based build have the correct mode. But maybe you are suggesting that ddmg.sh is run a second time (e.g., during the code signing process) without DATA_OUTSIDE_APP_DIR=1.

comment:11 in reply to:  10 ; Changed 2 years ago by gk

Cc: boklm added
Status: reopenedneeds_information

Replying to mcs:

As I implied in comment:6, files within the disk images produced by my own gitian-based build have the correct mode. But maybe you are suggesting that ddmg.sh is run a second time (e.g., during the code signing process) without DATA_OUTSIDE_APP_DIR=1.

That's indeed what is happening and the culprit here. I'll fix that in my signing script. Do we want to keep the tor-browser-bundle patch in, though?

comment:12 in reply to:  11 Changed 2 years ago by mcs

Status: needs_informationneeds_revision

Replying to gk:

That's indeed what is happening and the culprit here. I'll fix that in my signing script. Do we want to keep the tor-browser-bundle patch in, though?

My vote is to remove it because it will be redundant. But if that is too much work it is of course okay to leave it in place.

comment:13 Changed 2 years ago by gk

Resolution: fixed
Status: needs_revisionclosed

Done with commit 082738a4bd83943d97e084fa04045e481772b998 on master. The problem was I only did export DATA_OUTSIDE_APP_DIR. :( boklm: if you need to adapt your scripts, now would be a properly fine time to do so. :)

Note: See TracTickets for help on using tickets.