Page MenuHomePhabricator

Security: "Show Raw File" in Differential generated files with overbroad permissions
Closed, ResolvedPublic

Description

Overview

Until D17504, the "Show Raw File (Left)" and "Show Raw File (Right)" options in Differential generated an internal cache in Files which used default permissions (usually, "All Users"). Instead, these files should use the same permissions as the associated revision, which might be narrower.

Additionally, these files generated as permanent files. This is not a security concern, but other similar caches use temporary files.

After D17504, these files will generate with narrow permissions ("No One", plus a binding to the underlying diff), and generate as temporary files. That change also destroys all existing caches. This closes the hole, but will also make it impossible to audit potential disclosure impact because it destroys the evidence. See below for guidance on auditing.


Attack Scenarios

This issue was reported to us via HackerOne by a researcher. We have no evidence that it has been exploited, but a potential active attack scenario would look like this:

  • Alice, a reviewer with access to privileged changes, is the victim.
  • Eve, a user with access to an account but no access to privileged changes, is the attacker.
  • Eve instructs Alice to navigate to a privileged revision in Differential and choose "Show Raw File (Right)" on a file she wishes to learn the content of. Eve can't see the revision, but might trick Alice into doing through social engineering, like asking Alice "Hey, does 'Show Raw File' work for you on 'secret_information.txt' in D123?". Alice may not realize that Eve does not have permission to take this action.
  • After Alice clicks the link to generate the cache, Eve locates the generated cache in the main Files list and examines the file contents.

A passive attack would look like this:

  • Eve, a user with access to an account but no accessed to privileged changes, is the attacker.
  • Eve exhaustively searches Files to find caches which were disclosed incidentally during normal use of "Show Raw File".

In both cases, Eve must already control an account. This issue only allows Eve to escalate from control of an account to access to information which that account does not have permission to access.

The other three similar mechanisms ("Download Raw Patch" in Differential and Diffusion, "Show Raw File" in Diffusion) already use the correct permission model and use temporary files. It is likely that the vulnerable code predates significant portions of the Files and permissions systems, and was just overlooked as these other systems upgraded and gained more powerful policy and permissions capabilities.


Auditing and Response

If you want to assess the potential impact of this disclosure, you can run this script to get a list of file caches that exist. Only files on this list could have been disclosed. Save it as list-cached-files.php in phabricator/, then run it like this:

phabricator/ $ php -f list-cached-files.php
F123
F234
...

Here is the script:

list-cached-files.php
<?php

require_once 'scripts/__init_script__.php';

$table = new DifferentialChangeset();
$conn = $table->establishConnection('w');
$viewer = PhabricatorUser::getOmnipotentUser();

$iterator = new LiskRawMigrationIterator(
  $conn,
  $table->getTableName());

$keys = array(
  'raw:new:phid',
  'raw:old:phid',
);

foreach ($iterator as $changeset) {
  try {
    $metadata = phutil_json_decode($changeset['metadata']);

    $phids = array();
    foreach ($keys as $key) {
      if (isset($metadata[$key])) {
        $phids[] = $metadata[$key];
      }
    }

    foreach ($phids as $phid) {
      $file = id(new PhabricatorFileQuery())
        ->setViewer($viewer)
        ->withPHIDs(array($phid))
        ->executeOne();
      if ($file) {
        echo "F".$file->getID()."\n";
      }
    }
  } catch (Exception $ex) {
    // Ignore.
  }
}

You can use this script in conjunction with bin/files cat to show file content instead of file monograms:

phabricator/ $ php -f list-cached-files.php | xargs -n1 ./bin/files cat

Note that you must run this script before you upgrade to D17504, since D17504 will destroy all these files and make it impossible to audit them.

If additional tools would be helpful in auditing and assessing impact, let us know what you need.

(Phabricator doesn't retain a detailed record of file access outside of any access log written by log.access.path, so we can't do much to distinguish between potential disclosure and actual disclosure, although you may be able to correlate list-cached-files.php with any access logs you maintain.)

Event Timeline

The fix is now available on master (rP7626ec0c) and stable (rP6f879559). I've upgraded this install without incident. Per above, note that upgrading destroys evidence, so you should plan any audit or response actions you want to take before upgrading.

chad awarded a token.Mar 16 2017, 5:21 PM

It is likely that the vulnerable code predates significant portions of the Files and permissions systems, and was just overlooked as these other systems upgraded and gained more powerful policy and permissions capabilities.

Specifically, the vulnerable code was written in February 2012 (D1607). I believe that change was itself addressing a Flash vulnerability where plain text files could be interpreted as .swf files and executed in some contexts. At the time, all revisions were visible to all users.

Differential began to gain policy controls about 18 months later, in September 2013 (D7133) and files became bindable to other objects in October 2013 (D7182). This workflow should have been updated around that time, but was overlooked in December 2013 (D7849 / T4270) when other relevant workflows were addressed.

We ran the script provided above to get an audit of at-risk files. Afterwards we upgraded our instance and the upgrade succeeded however its attempts to delete the affected files failed. The failure is due to using a local file store which is accessible to our web service account but not the phabricator phd services account (T4752). After correcting the file permissions so both accounts have appropriate access, running upgrade again doesn't seem to remove the files.

Is there a way to re-run the path which will remove all affected files? It doesn't look like ./bin/files has a remove/delete command, and I'm not sure if ./bin/remove would work properly with the output of list-cached-files.php and/or each item would be prompted.

You can re-run the migration explicitly with:

phabricator/ $ ./bin/storage upgrade --apply phabricator:20170316.rawfiles.01.php

(You can use bin/storage status to get a list of valid patch names, in the general case.)

This particular patch is safe to re-apply as many times as you want. We try to make patches safe to re-apply where possible, but bin/storage upgrade --apply ... isn't always safe, so be cautious about using it in other situations.

(You could also pipe the list into bin/remove destroy --force, equivalently.)

epriestley closed this task as Resolved.Mar 20 2017, 2:16 PM
epriestley claimed this task.

We don't plan to take any other upstream actions here, but let us know if anyone has further questions.