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
F14367380: D12136.id29178.diff
Fri, Dec 20, 3:49 PM
F14360946: D12136.diff
Fri, Dec 20, 10:52 AM
Unknown Object (File)
Tue, Dec 10, 4:30 PM
Unknown Object (File)
Mon, Dec 9, 3:53 PM
Unknown Object (File)
Sun, Dec 8, 11:31 AM
Unknown Object (File)
Thu, Nov 28, 9:23 AM
Unknown Object (File)
Tue, Nov 26, 8:12 PM
Unknown Object (File)
Fri, Nov 22, 3:02 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.