#2535 closed defect (fixed)

Magic Folder: permissions of downloaded files should be set in a more failure-safe way

Reported by: dawuud Owned by: daira
Priority: normal Milestone: undecided
Component: code-frontend-magic-folder Version: 1.10.1
Keywords: magic-folder permissions security usability unix docs-needed Cc:
Launchpad Bug:

Description (last modified by daira)

During the demo with Zooko, Daira, Warner and Meejah last night we found yet another bug: the magic-folder downloader writes files with the original uploader's permissions intact as if the umask were 0077, but it should probably use the downloader's user's umask instead. If we do it like that then the user downloading files will have consistent permissions for all downloaded files.

Change History (31)

comment:1 Changed at 2015-10-14T06:51:42Z by dawuud

  • Description modified (diff)

comment:2 Changed at 2015-10-14T11:01:21Z by daira

I don't think it uses the original uploader's permissions. It uses fileutil.write (here), which uses whatever the default is for open(path, "wb") (here). But it does this as the daemonised node process, which might not have the same umask as the user's shell (ISTR that twistd changes the umask).

Last edited at 2015-10-14T11:13:27Z by daira (previous) (diff)

comment:3 Changed at 2015-10-14T11:11:41Z by daira

twistd sets umask to 0077 by default, unless overridden by --umask.

The correct approach here seems to be to save the original umask for later use by _write_downloaded_file, before passing control to twistd in tahoe start/run. (We don't want to change the umask of the node from 0077 using --umask because that would have difficult-to-review security consequences.)

comment:4 Changed at 2015-10-14T11:14:31Z by daira

  • Keywords security usability added

comment:5 Changed at 2015-10-14T11:18:01Z by daira

  • Summary changed from magic-folder permissions are inconsistent to Magic Folder: permissions of downloaded files are not set according to the user's umask

comment:6 Changed at 2015-10-14T11:19:36Z by daira

  • Keywords unix added

This bug is Unix-specific.

comment:7 Changed at 2015-10-14T11:21:06Z by daira

  • Description modified (diff)

comment:8 Changed at 2015-10-14T11:32:42Z by daira

Alternatively, or as well, we can have a [magic_folder]download.umask entry in tahoe.cfg. (For good reason, daemons don't normally just use the default umask.)

Last edited at 2015-10-14T11:33:16Z by daira (previous) (diff)

comment:9 Changed at 2015-10-25T11:25:13Z by daira

Note that the umask (or [magic_folder]download.umask entry) should only be used when writing a new file or directory. When replacing a file, docs/proposed/magic-folder/remote-to-local-sync.rst describes what should happen:

The implementation of file replacement differs between Unix and Windows. On Unix, it can be implemented as follows:

  • 4a. Set the permissions of the replacement file to be the same as the replaced file, bitwise-or'd with octal 600 (rw-------).

comment:10 Changed at 2015-12-03T20:41:25Z by daira

This does not block merging Magic Folder to trunk, but should preferably be fixed before a subsequent release.

comment:11 Changed at 2015-12-07T15:56:35Z by daira

  • Owner changed from daira to dawuud

comment:13 Changed at 2015-12-10T06:59:44Z by dawuud

  • Owner changed from dawuud to daira

comment:14 Changed at 2015-12-14T21:48:08Z by daira

Merged, but this also needs documentation in docs/configuration.rst and docs/frontends/magic-folder.rst.

comment:15 Changed at 2015-12-14T21:48:32Z by daira

  • Keywords needs-docs added
  • Owner changed from daira to dawuud

comment:16 Changed at 2015-12-15T02:41:39Z by daira

  • Keywords docs-needed added; needs-docs removed

comment:17 Changed at 2015-12-17T11:42:38Z by dawuud

  • Owner changed from dawuud to daira

i added a note about the umask to magic-folder.rst

configuration.rst doesn't have a magic-folder section.

comment:18 Changed at 2015-12-21T20:35:06Z by dawuud

what's next? review?

comment:19 Changed at 2015-12-21T22:21:03Z by daira

  • Resolution set to fixed
  • Status changed from new to closed

Pushed to 2438.magic-folder-stable.5 .

comment:20 Changed at 2016-01-04T13:43:12Z by daira

The current approach is undesirable because the os.umask in the finally block could fail, leaving an unsafe umask for the whole node process.

Using os.open with a mode argument wouldn't work because the umask specified by twistd (0077) would still be masked off, both when creating the file and when creating any ancestor directories that do not already exist. It appears that the only safe way for a Unix process to atomically set an arbitrary umask when creating a file, without setting its own process-wide umask, is to create a subprocess that will have the desired umask.

Here, we don't need atomicity for setting the umask of the replacement file, so we could use chmod to relax the permissions from the ones that result from the twistd umask. However we would also need to chmod any created directories. Reopening as a reminder to change it to use this method.

comment:21 Changed at 2016-01-04T13:44:04Z by daira

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:22 Changed at 2016-01-14T15:15:29Z by daira

  • Summary changed from Magic Folder: permissions of downloaded files are not set according to the user's umask to Magic Folder: permissions of downloaded files should be set in a more failure-safe way

comment:23 Changed at 2016-01-19T20:11:34Z by dawuud

  • Status changed from reopened to new

ok i got rid of the call to umask and instead use chmod as daira suggested; in my dev branch here: https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.0

please review

comment:24 Changed at 2016-01-19T20:31:57Z by meejah

Looks good to me. Two questions and a comment:

  • where does self._local_path_u come from (i.e. should that be local_path_u instead?)
  • i would maybe call the local_path_u arg to _write_downloaded_file workdir_u or similar to match the arg name at the callsite (and also to avoid confusion to self._local_path_u if that isn't a typo)
  • while loops make me nervous ;) but that one looks fine
Last edited at 2016-01-19T20:32:15Z by meejah (previous) (diff)

comment:25 Changed at 2016-01-21T13:05:37Z by dawuud

The callsite passes self._local_path_u which is the Magic-Folder directory... and _write_downloaded_file needs it to ensure that we can chmod all created directories without chmod'ing outside the Magic-Folder.

I guess I should change the argument name to magicfolder_dir_u...

Last edited at 2016-01-21T15:39:30Z by daira (previous) (diff)

comment:26 Changed at 2016-01-21T17:16:16Z by dawuud

the current plan is to break out the chmod while loop into a separate function in fileutils.py and write unit tests for it.

comment:27 Changed at 2016-01-21T22:42:32Z by dawuud

corrections made and unit test added: https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.0

please review

comment:28 Changed at 2016-02-11T15:11:22Z by daira

Review comments need addressing.

comment:29 Changed at 2016-02-15T12:25:45Z by dawuud

i've fixed the things daira said to fix in her review comments. i also noticed this branch doesn't merge cleanly onto stable.12 so i'll have to make another branch using stable.12 and then perhaps cherry-pick several times.

comment:30 Changed at 2016-02-15T13:36:38Z by dawuud

new dev branch here... cherry-picked and awaiting review https://github.com/david415/tahoe-lafs/tree/2535.chmod-not-umask.1

comment:31 Changed at 2016-02-16T15:21:35Z by daira

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.