From 2037979142cb2e8b5edb8dc2361567c755bb7396 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 15 Dec 2014 10:41:49 -0800
Subject: [PATCH] Prevent Phame blogs from using invalid skins

Summary: Via HackerOne. An attacker with access to both Phame and the filesystem could potentially load a skin that lives outside of the configured skin directories, because we had insufficient checks on the actual skin at load time.

Test Plan: Attempted to build a blog with an invalid skin; got an exception instead of a mis-load of a sketchy skin.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10992
---
 .../phame/skins/PhameSkinSpecification.php    | 24 +++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/applications/phame/skins/PhameSkinSpecification.php b/src/applications/phame/skins/PhameSkinSpecification.php
index 7c1754f4b2..0303bdf12d 100644
--- a/src/applications/phame/skins/PhameSkinSpecification.php
+++ b/src/applications/phame/skins/PhameSkinSpecification.php
@@ -56,12 +56,32 @@ final class PhameSkinSpecification {
   }
 
   public static function loadOneSkinSpecification($name) {
+    // Only allow skins which we know to exist to load. This prevents loading
+    // skins like "../../secrets/evil/".
+    $all = self::loadAllSkinSpecifications();
+    if (empty($all[$name])) {
+      throw new Exception(
+        pht(
+          'Blog skin "%s" is not a valid skin!',
+          $name));
+    }
+
     $paths = PhabricatorEnv::getEnvConfig('phame.skins');
-    $base  = dirname(phutil_get_library_root('phabricator'));
+    $base = dirname(phutil_get_library_root('phabricator'));
     foreach ($paths as $path) {
       $path = Filesystem::resolvePath($path, $base);
       $skin_path = $path.DIRECTORY_SEPARATOR.$name;
       if (is_dir($skin_path)) {
+
+        // Double check that the skin really lives in the skin directory.
+        if (!Filesystem::isDescendant($skin_path, $path)) {
+          throw new Exception(
+            pht(
+              'Blog skin "%s" is not located in path "%s"!',
+              $name,
+              $path));
+        }
+
         $spec = self::loadSkinSpecification($skin_path);
         if ($spec) {
           $spec->setName($name);
@@ -72,7 +92,7 @@ final class PhameSkinSpecification {
     return null;
   }
 
-  public static function loadSkinSpecification($path) {
+  private static function loadSkinSpecification($path) {
 
     $config_path = $path.DIRECTORY_SEPARATOR.'skin.json';
     $config = array();