Page MenuHomePhabricator

Improve granluarity and defaults of `security.allow-outbound-http`
ClosedPublic

Authored by epriestley on Mar 23 2015, 3:54 PM.
Tags
None
Referenced Files
F15459635: D12136.id29165.diff
Mon, Mar 31, 2:49 PM
F15421990: D12136.id29178.diff
Sat, Mar 22, 4:15 AM
F15421692: D12136.id29165.diff
Sat, Mar 22, 1:38 AM
F15419384: D12136.id29165.diff
Fri, Mar 21, 5:51 AM
F15418293: D12136.id29165.diff
Thu, Mar 20, 10:20 PM
F15417082: D12136.id.diff
Thu, Mar 20, 3:08 PM
F15412036: D12136.id.diff
Wed, Mar 19, 10:58 AM
F15405262: D12136.id29178.diff
Tue, Mar 18, 10:56 AM

Details

Summary

Ref T6755. This is a partial fix, but:

  • Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
  • Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
  • Explain the risks better.
  • Improve the errors rasied by Macro when failing.
  • Removed security.allow-outbound-http, as it is superseded by this setting and is somewhat misleading.
    • We still make outbound HTTP requests to OAuth.
    • We still make outbound HTTP requests for repositories.

From a technical perspective:

  • Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
  • Add the default blacklist.
  • Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.

Additionally:

  • I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
  • The future implementation of T4190 needs to perform the fetch check.
Test Plan
  • Fetched a valid macro.
  • Fetched a non-image, verified it didn't result in a viewable file.
  • Fetched a private-ip-space image, got an error.
  • Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
  • Fetched a bad protocol, got an error.
  • Linked to a local resource, a phriction page, a valid remote site, all worked.
  • Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
  • Added and executed unit tests.

Diff Detail

Repository
rP Phabricator
Branch
blacklist
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 4951
Build 4969: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Improve granluarity and defaults of `security.allow-outbound-http`.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
src/applications/macro/controller/PhabricatorMacroEditController.php
75

Note that newFromFileDownload() does the URI check for us.

btrahan edited edge metadata.
This revision is now accepted and ready to land.Mar 23 2015, 5:34 PM
This revision was automatically updated to reflect the committed changes.

The protection can be bypassed using HTTP redirects (untested) and DNS re-binding. Cf the HackerOne ticket for details https://hackerone.com/reports/53088

See T6755 for some additional discussion.