Page MenuHomePhabricator

Implement policies in Phragment
ClosedPublic

Authored by hach-que on Dec 10 2013, 6:26 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 12:59 AM
Unknown Object (File)
Wed, Mar 20, 5:18 PM
Unknown Object (File)
Mon, Mar 4, 11:41 PM
Unknown Object (File)
Feb 11 2024, 11:50 AM
Unknown Object (File)
Feb 8 2024, 9:16 PM
Unknown Object (File)
Feb 4 2024, 10:52 PM
Unknown Object (File)
Jan 27 2024, 8:28 AM
Unknown Object (File)
Jan 24 2024, 11:14 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T4205: Implement Phragment
Commits
Restricted Diffusion Commit
rP86ec4d602137: Implement policies in Phragment
Summary

This implements support for enforcing and setting policies in Phragment.

Test Plan

Set policies and ensured they were enforced successfully.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This also implements file nonce tokens. The reason for this is two-fold:

  • Unauthenticated users can not view the file info controller, regardless of the policy on the file.
  • Clicking "Download ZIP" and being redirected to the info page is a bad user experience.

By implementing use-once tokens, this allows us to redirect users safely to a raw file download, while preventing attacks like using the file in an <iframe> (because the token only lasts for 60 seconds and can only be used once).

The getNonceURI token should only be used as part of redirects and never printed to the page raw (because the user may not click the link).

hach-que updated this revision to Unknown Object (????).Dec 10 2013, 6:32 AM

Enforce expiry dates on nonce tokens

This is not safe. As an attacker, I just write an evil.com which does this:

  • When my victim loads the page, I make a request to Phabricator to generate an appropriate nonce before returning a response.
  • Now, with the nonce in hand, I return <applet src="evil.jar?nonce=abc" /> to the victim.
  • The victim loads the page with a valid, nonced resource URI, so their browser loads the applet and I can escalate from there.

How about we do this instead?

  • If the user GETs the download URI, and the alternate file domain isn't configured, we render a simplified page with just a "download file" button?
src/applications/phragment/controller/PhragmentPolicyController.php
4

Instead, we should write PhabricatorEdgeConfig::TYPE_FILE_HAS_OBJECT edges for these files, which guarantee visibility if you can see an object the file is attached to. These are also visible in the Files UI so it's easier to keep track of things.

src/applications/phragment/controller/PhragmentSnapshotPromoteController.php
55–58

Prefer to use requireCapabilities(VIEW, EDIT) when querying over explicit checks, if you can. This is a stronger behavior because we never even get objects which we don't have authority to act on, so it's more difficult to make mistakes.

src/view/layout/PhabricatorActionView.php
43

Can we use the existing download flag for this instead of introducing a new flag?

hach-que updated this revision to Unknown Object (????).Dec 10 2013, 11:55 PM

I think I fixed everything, but I got distracted in IRC so many times I can't be sure

hach-que updated this revision to Unknown Object (????).Dec 11 2013, 12:08 AM

Allow unguarded writes when attaching fragments to files

This is good to go, but isValidLocalWebResource() is vaguely security-ish so I don't want to accept-with-inlines. Some other stuff inline.

src/applications/files/controller/PhabricatorFileDialogController.php
30–36 ↗(On Diff #17536)

Use PhabricatorEnv::isValidLocalWebResource().

src/applications/phragment/controller/PhragmentController.php
96

This shouldn't be possible, since viewing the fragment should imply having permission on the file if we're writing the edge. So a better fix is to remove this and start writing the edges.

src/applications/phragment/controller/PhragmentHistoryController.php
89–92

This is no longer accurate, is it?

The request will also always have a user, it just won't always be an isLoggedIn() user.

src/applications/phragment/controller/PhragmentPatchController.php
97–100

Maybe easier to just attach it to the fragment itself.

src/applications/phragment/controller/PhragmentSnapshotDeleteController.php
26

Do this in the query.

src/view/layout/PhabricatorActionView.php
14

Can't we get rid of this and just use $this->download instead?

hach-que updated this revision to Unknown Object (????).Dec 12 2013, 6:35 AM

Fix issues mentioned in code review

src/applications/files/controller/PhabricatorFileDialogController.php
30–36 ↗(On Diff #17536)

I used PhabricatorEnv::isValidWebResource() here since I want to allow external redirects after the download completes.

webroot/rsrc/js/application/files/behavior-files-download-redirect.js
15 ↗(On Diff #17536)

I updated this timeout to be 2 seconds to allow the download to start before redirecting.

I don't want to allow open redirects, even under these relatively difficult-to-exploit circumstances. The attack here is:

  • Alice emails everyone in the company with a "Hey, here's that payroll/acquisition/salary/secret project information you asked for: http://blahblah/stuff.zip?redirect=http://phishing.com/"
  • Everyone eagerly clicks the link and downloads the ZIP.
  • After the download begins, they get redirected to a phishing site.

This isn't easy to execute, but it's much easier than any other attack in this class currently is, because we don't do untrusted client redirects anywhere else. I think the use case for this is also really weak.

For your case, why can't you just configure the alternate file domain and we'll do GET -> GET?

hach-que updated this revision to Unknown Object (????).Dec 12 2013, 10:58 PM

Require users to configure the security.alternate-file-domain option to use Phragment

src/applications/files/controller/PhabricatorFileDataController.php
69–74

Users will already be on the alt domain if it's present due to the redirect above.

Cool, this seems reasonable.

webroot/rsrc/js/application/files/behavior-files-download-redirect.js
7 ↗(On Diff #17558)

Do we still need this?

hach-que updated this revision to Unknown Object (????).Dec 13 2013, 3:41 AM

Remove old behaviour JS file