Prevent the use of file:// URIs in Diffusion

Summary:
Via HackerOne. There are two attacks here:

  - Configuring mirroring to a `file://` URI to place files on disk or overwrite another repository. This is not particularly severe.
  - Configuring cloning from a `file://` URI to read repositories you should not have access to. This is more severe.

Historically, repository creation and editing explicitly supported `file://` URIs to deal with use cases where you had something else managing repositories on the same machine. Since there were no permissions, repository management was admin-only, and you couldn't mirror, this was fine.

As we've evolved, this use case is a tiny minority use case and the security implications of `file://` URIs overwhelm the utility it provides. Prevent the use of `file://` URIs. Existing configured repositories won't stop working, you just can't add any new ones.

Also prevent `localPath` from being set via Conduit (see T4039).

Test Plan:
  - Tried to create a `file://` repository.
  - Tried to create a `file://` mirror.
  - Tried to create a `file://` repository via Conduit.
  - Created a non-`file://` repository.
  - Created a non-`file://` mirror.
  - Created a non-`file://` repository via Conduit.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9513

src/applications/diffusion/controller/DiffusionMirrorEditController.php

@@ -49,7 +49,13 @@ final class DiffusionMirrorEditController
     if ($request->isFormPost()) {
       $v_remote = $request->getStr('remoteURI');
       if (strlen($v_remote)) {
-        $e_remote = null;
+        try {
+          PhabricatorRepository::assertValidRemoteURI($v_remote);
+          $e_remote = null;
+        } catch (Exception $ex) {
+          $e_remote = pht('Invalid');
+          $errors[] = $ex->getMessage();
+        }
       } else {
         $e_remote = pht('Required');
         $errors[] = pht('You must provide a remote URI.');

src/applications/diffusion/controller/DiffusionRepositoryCreateController.php

@@ -446,7 +446,6 @@ final class DiffusionRepositoryCreateController
         "| ----------------------- |\n".
         "| `git@github.com:example/example.git` |\n".
         "| `ssh://user@host.com/git/example.git` |\n".
-        "| `file:///local/path/to/repo` |\n".
         "| `https://example.com/repository.git` |\n");
     } else if ($is_mercurial) {
       $uri_label = pht('Remote URI');
@@ -457,7 +456,7 @@ final class DiffusionRepositoryCreateController
         "| Example Mercurial Remote URIs |\n".
         "| ----------------------- |\n".
         "| `ssh://hg@bitbucket.org/example/repository` |\n".
-        "| `file:///local/path/to/repo` |\n");
+        "| `https://bitbucket.org/example/repository` |\n");
     } else if ($is_svn) {
       $uri_label = pht('Repository Root');
       $instructions = pht(
@@ -471,7 +470,6 @@ final class DiffusionRepositoryCreateController
         "| `http://svn.example.org/svnroot/` |\n".
         "| `svn+ssh://svn.example.com/svnroot/` |\n".
         "| `svn://svn.example.net/svnroot/` |\n".
-        "| `file:///local/path/to/svnroot/` |\n".
         "\n\n".
         "You **MUST** specify the root of the repository, not a ".
         "subdirectory. (If you want to import only part of a Subversion ".
@@ -494,52 +492,11 @@ final class DiffusionRepositoryCreateController
       $page->addPageError(
         pht('You must specify a URI.'));
     } else {
-      $proto = $this->getRemoteURIProtocol($v_remote);
+      try {
-
+        PhabricatorRepository::assertValidRemoteURI($v_remote);
-      if ($proto === 'file') {
+      } catch (Exception $ex) {
-        if (!preg_match('@^file:///@', $v_remote)) {
+        $c_remote->setError(pht('Invalid'));
-          $c_remote->setError(pht('Invalid'));
+        $page->addPageError($ex->getMessage());
-          $page->addPageError(
-            pht(
-              "URIs using the 'file://' protocol should have three slashes ".
-              "(e.g., 'file:///absolute/path/to/file'). You only have two. ".
-              "Add another one."));
-        }
-      }
-
-      // Catch confusion between Git/SCP-style URIs and normal URIs. See T3619
-      // for discussion. This is usually a user adding "ssh://" to an implicit
-      // SSH Git URI.
-      if ($proto == 'ssh') {
-        if (preg_match('(^[^:@]+://[^/:]+:[^\d])', $v_remote)) {
-          $c_remote->setError(pht('Invalid'));
-          $page->addPageError(
-            pht(
-              "The Remote URI is not formatted correctly. Remote URIs ".
-              "with an explicit protocol should be in the form ".
-              "'proto://domain/path', not 'proto://domain:/path'. ".
-              "The ':/path' syntax is only valid in SCP-style URIs."));
-        }
-      }
-
-      switch ($proto) {
-        case 'ssh':
-        case 'http':
-        case 'https':
-        case 'file':
-        case 'git':
-        case 'svn':
-        case 'svn+ssh':
-          break;
-        default:
-          $c_remote->setError(pht('Invalid'));
-          $page->addPageError(
-            pht(
-              "The URI protocol is unrecognized. It should begin ".
-              "'ssh://', 'http://', 'https://', 'file://', 'git://', ".
-              "'svn://', 'svn+ssh://', or be in the form ".
-              "'git@domain.com:path'."));
-          break;
       }
     }
 
@@ -573,7 +530,8 @@ final class DiffusionRepositoryCreateController
     $remote_uri = $form->getPage('remote-uri')
       ->getControl('remoteURI')
       ->getValue();
-    $proto = $this->getRemoteURIProtocol($remote_uri);
+
+    $proto = PhabricatorRepository::getRemoteURIProtocol($remote_uri);
     $remote_user = $this->getRemoteURIUser($remote_uri);
 
     $c_credential = $page->getControl('credential');
@@ -613,15 +571,6 @@ final class DiffusionRepositoryCreateController
           "you can continue to the next step.",
           $remote_uri),
         'credential');
-    } else if ($proto == 'file') {
-      $c_credential->setHidden(true);
-      $provides_type = null;
-
-      $page->addRemarkupInstructions(
-        pht(
-          'You do not need to configure any credentials for repositories '.
-          'accessed over the `file://` protocol. Continue to the next step.'),
-        'credential');
     } else {
       throw new Exception('Unknown URI protocol!');
     }
@@ -870,21 +819,6 @@ final class DiffusionRepositoryCreateController
 
 /* -(  Internal  )----------------------------------------------------------- */
 
-
-  private function getRemoteURIProtocol($raw_uri) {
-    $uri = new PhutilURI($raw_uri);
-    if ($uri->getProtocol()) {
-      return strtolower($uri->getProtocol());
-    }
-
-    $git_uri = new PhutilGitURI($raw_uri);
-    if (strlen($git_uri->getDomain()) && strlen($git_uri->getPath())) {
-      return 'ssh';
-    }
-
-    return null;
-  }
-
   private function getRemoteURIUser($raw_uri) {
     $uri = new PhutilURI($raw_uri);
     if ($uri->getUser()) {

src/applications/repository/conduit/ConduitAPI_repository_create_Method.php

@@ -30,7 +30,6 @@ final class ConduitAPI_repository_create_Method
       'tracking'            => 'optional bool',
       'uri'                 => 'optional string',
       'credentialPHID'      => 'optional string',
-      'localPath'           => 'optional string',
       'svnSubpath'          => 'optional string',
       'branchFilter'        => 'optional list<string>',
       'closeCommitsFilter'  => 'optional list<string>',
@@ -100,12 +99,14 @@ final class ConduitAPI_repository_create_Method
 
     $repository->setCredentialPHID($request->getValue('credentialPHID'));
 
+    $remote_uri = $request->getValue('uri');
+    PhabricatorRepository::assertValidRemoteURI($remote_uri);
+
     $details = array(
       'encoding'          => $request->getValue('encoding'),
       'description'       => $request->getValue('description'),
       'tracking-enabled'  => (bool)$request->getValue('tracking', true),
-      'remote-uri'        => $request->getValue('uri'),
+      'remote-uri'        => $remote_uri,
-      'local-path'        => $request->getValue('localPath'),
       'branch-filter'     => array_fill_keys(
         $request->getValue('branchFilter', array()),
         true),

src/applications/repository/editor/PhabricatorRepositoryEditor.php

@@ -318,6 +318,21 @@ final class PhabricatorRepositoryEditor
     $errors = parent::validateTransaction($object, $type, $xactions);
 
     switch ($type) {
+      case PhabricatorRepositoryTransaction::TYPE_REMOTE_URI:
+        foreach ($xactions as $xaction) {
+          $new_uri = $xaction->getNewValue();
+          try {
+            PhabricatorRepository::assertValidRemoteURI($new_uri);
+          } catch (Exception $ex) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              $ex->getMessage(),
+              $xaction);
+          }
+        }
+        break;
+
       case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
         $ok = PassphraseCredentialControl::validateTransactions(
           $this->getActor(),

src/applications/repository/storage/PhabricatorRepository.php

@@ -1257,6 +1257,68 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $this;
   }
 
+  public static function getRemoteURIProtocol($raw_uri) {
+    $uri = new PhutilURI($raw_uri);
+    if ($uri->getProtocol()) {
+      return strtolower($uri->getProtocol());
+    }
+
+    $git_uri = new PhutilGitURI($raw_uri);
+    if (strlen($git_uri->getDomain()) && strlen($git_uri->getPath())) {
+      return 'ssh';
+    }
+
+    return null;
+  }
+
+  public static function assertValidRemoteURI($uri) {
+    if (trim($uri) != $uri) {
+      throw new Exception(
+        pht(
+          'The remote URI has leading or trailing whitespace.'));
+    }
+
+    $protocol = self::getRemoteURIProtocol($uri);
+
+    // Catch confusion between Git/SCP-style URIs and normal URIs. See T3619
+    // for discussion. This is usually a user adding "ssh://" to an implicit
+    // SSH Git URI.
+    if ($protocol == 'ssh') {
+      if (preg_match('(^[^:@]+://[^/:]+:[^\d])', $uri)) {
+        throw new Exception(
+          pht(
+            "The remote URI is not formatted correctly. Remote URIs ".
+            "with an explicit protocol should be in the form ".
+            "'proto://domain/path', not 'proto://domain:/path'. ".
+            "The ':/path' syntax is only valid in SCP-style URIs."));
+      }
+    }
+
+    switch ($protocol) {
+      case 'ssh':
+      case 'http':
+      case 'https':
+      case 'git':
+      case 'svn':
+      case 'svn+ssh':
+        break;
+      default:
+        // NOTE: We're explicitly rejecting 'file://' because it can be
+        // used to clone from the working copy of another repository on disk
+        // that you don't normally have permission to access.
+
+        throw new Exception(
+          pht(
+            "The URI protocol is unrecognized. It should begin ".
+            "'ssh://', 'http://', 'https://', 'git://', 'svn://', ".
+            "'svn+ssh://', or be in the form 'git@domain.com:path'."));
+    }
+
+    return true;
+  }
+
+
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */