This implements support for enforcing and setting policies in Phragment.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4205: Implement Phragment
- Commits
- Restricted Diffusion Commit
rP86ec4d602137: Implement policies in Phragment
Set policies and ensured they were enforced successfully.
Diff Detail
Diff Detail
- Branch
- phragment-policy
- Lint
Lint Warnings Severity Location Code Message Warning webroot/rsrc/js/application/files/behavior-files-download-redirect.js:1 JAVELIN5 `javelinsymbols` Not In Path - Unit
Tests Passed
Event Timeline
Comment Actions
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).
Comment Actions
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 | ||
| 44 | Can we use the existing download flag for this instead of introducing a new flag? | |
Comment Actions
I think I fixed everything, but I got distracted in IRC so many times I can't be sure
Comment Actions
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 | Use PhabricatorEnv::isValidLocalWebResource(). | |
| src/applications/phragment/controller/PhragmentController.php | ||
| 91 | 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 | ||
| 83–86 | 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 | ||
| 23 | Do this in the query. | |
| src/view/layout/PhabricatorActionView.php | ||
| 14 | Can't we get rid of this and just use $this->download instead? | |
Comment Actions
Fix issues mentioned in code review
| src/applications/files/controller/PhabricatorFileDialogController.php | ||
|---|---|---|
| 30–36 | 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 | I updated this timeout to be 2 seconds to allow the download to start before redirecting. | |
Comment Actions
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?
Comment Actions
Require users to configure the security.alternate-file-domain option to use Phragment
| src/applications/files/controller/PhabricatorFileDataController.php | ||
|---|---|---|
| 69 ↗ | (On Diff #17558) | Users will already be on the alt domain if it's present due to the redirect above. |
Comment Actions
Cool, this seems reasonable.
| webroot/rsrc/js/application/files/behavior-files-download-redirect.js | ||
|---|---|---|
| 8 | Do we still need this? | |