#2506 closed defect (fixed)

Magic Folder: enforce that downloaded files are only written below the magic folder directory

Reported by: daira Owned by: daira
Priority: major Milestone: undecided
Component: code-frontend-magic-folder Version: n/a
Keywords: otf-magic-folder-objective4 security unicode path Cc:
Launchpad Bug:

Description

https://github.com/tahoe-lafs/tahoe-lafs/blob/665c36e45cab3487f279ccae44f79f606613c8f2/src/allmydata/frontends/magic_folder.py#L539

Since name comes from another client's DMD and is an arbitrary string, it can be an absolute path, or contain ".." path elements, or start with "~", which would cause the file to be written somewhere that might not be below this client's magic folder directory.

We should use FilePath.preauthChild to prevent this, and to provide a clearer type distinction between relative and absolute paths.

Change History (15)

comment:1 Changed at 2015-09-17T07:46:33Z by daira

To be clear, this is not a security vulnerability in master or any released version, only in some magic folder development branches.

Last edited at 2015-09-17T07:59:04Z by daira (previous) (diff)

comment:2 Changed at 2015-09-17T10:07:27Z by daira

  • Keywords blocks-merge added

comment:3 Changed at 2015-09-17T13:42:02Z by dawuud

I've extended the test_alice_bob test to use an insecure path... however it is unclear where this InsecurePath? exception will surface.. and therefore I am not sure what the proper way to test for it...

I *could* make the test check for the existence of the offending file outside of the magic-folder directory... this might be sufficient to test for the bad behavior... but it would stop working when we fix our code to fire the failure containing the exception...

this dev branch here: https://github.com/david415/tahoe-lafs/tree/2506.only-write-to-download-dir.0

Last edited at 2015-09-17T13:42:27Z by dawuud (previous) (diff)

comment:4 follow-up: Changed at 2015-09-18T16:03:33Z by dawuud

Recently Daira made this dev branch so that twisted filepath objects are used more often than file path unicode strings:

https://github.com/tahoe-lafs/tahoe-lafs/tree/2506.enforce-paths.0

I was working with the code trying to fix it and I discovered that the twisted inotifier doesn't produce filepath objects in unicode mode... and this breaks our code. Here's the twisted bug regarding this issue:

https://twistedmatrix.com/trac/ticket/7928

comment:5 Changed at 2015-09-18T16:07:05Z by dawuud

just in helps here's my sloppy dev branch with some changes that helped me identify problems https://github.com/david415/tahoe-lafs/tree/2506.enforce-paths.0

comment:6 Changed at 2015-09-24T08:29:51Z by dawuud

my dev branch here shows a weird bug... and the test now fails instead of live-locking: https://github.com/david415/tahoe-lafs/tree/2506.enforce-paths.1

i made the test fail by testing for the local magic-folder db version of the file after alice rewrites it...

comment:7 in reply to: ↑ 4 Changed at 2015-09-24T16:51:59Z by daira

Replying to dawuud:

Recently Daira made this dev branch so that twisted filepath objects are used more often than file path unicode strings:

https://github.com/tahoe-lafs/tahoe-lafs/tree/2506.enforce-paths.0

That branch doesn't yet use FilePaths in all the cases I wanted to. I have a branch on my machine that does, and hopefully we can work on that during our pairing today once we've fixed the current bug (or if we think it will help with the bug).

Last edited at 2015-09-24T16:52:39Z by daira (previous) (diff)

comment:8 Changed at 2015-09-25T08:47:51Z by dawuud

Last night Daira and I paired and she fixed our design doc: https://github.com/david415/tahoe-lafs/commit/c649721318485cac7496cb37a3ff003be4d5234c

This morning I fixed the code... all tests now pass: https://github.com/david415/tahoe-lafs/tree/2506.enforce-paths.2

I think this ticket can be closed now... pending review.

comment:9 Changed at 2015-09-25T19:37:59Z by daira

Reviewed. Looks good so far, but this needs a test that InsecurePath is actually raised (instead of writing the file) when it should be.

Last edited at 2015-09-26T00:03:44Z by daira (previous) (diff)

comment:10 Changed at 2015-09-28T08:38:52Z by daira

Arrgh, FilePath.preauthChild is insecure: https://twistedmatrix.com/trac/ticket/6527

comment:11 Changed at 2015-09-29T14:14:01Z by daira

I have a fix for this, but it's entangled with other changes that cause test failures, so I haven't pushed it yet. I will try to push it tonight.

comment:12 Changed at 2015-09-30T15:49:49Z by daira

I believe this is now fixed on the https://github.com/tahoe-lafs/tahoe-lafs/commits/2506.enforce-paths.2 branch. It passes existing tests, but still lacks a specific test for the security bug.

I have also rebased to master: https://github.com/tahoe-lafs/tahoe-lafs/commits/2506.enforce-paths.4.

Last edited at 2015-10-01T00:20:57Z by daira (previous) (diff)

comment:13 Changed at 2015-09-30T15:51:31Z by daira

Note that these include the clock improvements on your branch, David.

Last edited at 2015-09-30T15:51:54Z by daira (previous) (diff)

comment:14 Changed at 2015-10-27T20:26:11Z by daira

  • Keywords test-needed blocks-merge removed
  • Resolution set to fixed
  • Status changed from new to closed

This is now tested, in the Alice_tries_to_p0wn_Bob section of test_alice_bob.

comment:15 Changed at 2016-07-21T21:32:36Z by Brian Warner <warner@…>

In cec315d/trunk:

Add test that we don't write files outside the magic folder directory. refs ticket:2506

Signed-off-by: Daira Hopwood <daira@…>

Note: See TracTickets for help on using tickets.