MAGETWO-52867: [APPSEC-1446] Sensitive server information disclosure upon specific URL requests

Author: dsikkema-magento

.htaccess

@@ -206,7 +206,7 @@
 
 ###########################################
 ## Deny access to root files to hide sensitive application information
-    RedirectMatch 404 /\.git
+    RedirectMatch 403 /\.git
 
     <Files composer.json>
         order allow,deny
@@ -281,6 +281,10 @@
             deny from all
     </Files>
 
+# For 404s and 403s that aren't handled by the application, show plain 404 response
+ErrorDocument 404 /pub/errors/404.php
+ErrorDocument 403 /pub/errors/404.php
+
 ################################
 ## If running in cluster environment, uncomment this
 ## http://developer.yahoo.com/performance/rules.html#etags

lib/internal/Magento/Framework/App/Bootstrap.php

@@ -404,15 +404,17 @@ public function getErrorCode()
      */
     public function isDeveloperMode()
     {
-        if (isset($this->server[State::PARAM_MODE]) && $this->server[State::PARAM_MODE] == State::MODE_DEVELOPER) {
-            return true;
-        }
-        /** @var \Magento\Framework\App\DeploymentConfig $deploymentConfig */
-        $deploymentConfig = $this->getObjectManager()->get('Magento\Framework\App\DeploymentConfig');
-        if ($deploymentConfig->get(State::PARAM_MODE) == State::MODE_DEVELOPER) {
-            return true;
+        $mode = 'default';
+        if (isset($this->server[State::PARAM_MODE])) {
+            $mode = $this->server[State::PARAM_MODE];
+        } else {
+            $deploymentConfig = $this->getObjectManager()->get(DeploymentConfig::class);
+            if (($configMode = $deploymentConfig->get(State::PARAM_MODE)) !== null) {
+                $mode = $configMode;
+            }
         }
-        return false;
+
+        return $mode == State::MODE_DEVELOPER;
     }
 
     /**

lib/internal/Magento/Framework/App/StaticResource.php

@@ -5,7 +5,9 @@
  */
 namespace Magento\Framework\App;
 
+use Magento\Framework\App\Filesystem\DirectoryList;
 use Magento\Framework\ObjectManager\ConfigLoaderInterface;
+use Magento\Framework\Filesystem;
 
 /**
  * Entry point for retrieving static resources like JS, CSS, images by requested public path
@@ -14,46 +16,33 @@
  */
 class StaticResource implements \Magento\Framework\AppInterface
 {
-    /**
-     * @var State
-     */
+    /** @var State */
     private $state;
 
-    /**
-     * @var \Magento\Framework\App\Response\FileInterface
-     */
+    /** @var \Magento\Framework\App\Response\FileInterface */
     private $response;
 
-    /**
-     * @var Request\Http
-     */
+    /** @var Request\Http */
     private $request;
 
-    /**
-     * @var View\Asset\Publisher
-     */
+    /** @var View\Asset\Publisher */
     private $publisher;
 
-    /**
-     * @var \Magento\Framework\View\Asset\Repository
-     */
+    /** @var \Magento\Framework\View\Asset\Repository */
     private $assetRepo;
 
-    /**
-     * @var \Magento\Framework\Module\ModuleList
-     */
+    /** @var \Magento\Framework\Module\ModuleList */
     private $moduleList;
 
-    /**
-     * @var \Magento\Framework\ObjectManagerInterface
-     */
+    /** @var \Magento\Framework\ObjectManagerInterface */
     private $objectManager;
 
-    /**
-     * @var ConfigLoaderInterface
-     */
+    /** @var ConfigLoaderInterface */
     private $configLoader;
 
+    /** @var Filesystem */
+    private $filesystem;
+
     /**
      * @param State $state
      * @param Response\FileInterface $response
@@ -116,12 +105,14 @@ public function launch()
      */
     public function catchException(Bootstrap $bootstrap, \Exception $exception)
     {
-        $this->response->setHttpResponseCode(404);
-        $this->response->setHeader('Content-Type', 'text/plain');
         if ($bootstrap->isDeveloperMode()) {
+            $this->response->setHttpResponseCode(404);
+            $this->response->setHeader('Content-Type', 'text/plain');
             $this->response->setBody($exception->getMessage() . "\n" . $exception->getTraceAsString());
+            $this->response->sendResponse();
+        } else {
+            require $this->getFilesystem()->getDirectoryRead(DirectoryList::PUB)->getAbsolutePath('errors/404.php');
         }
-        $this->response->sendResponse();
         return true;
     }
 
@@ -156,4 +147,18 @@ protected function parsePath($path)
         $result['file'] = $parts[5];
         return $result;
     }
+
+    /**
+     * Lazyload filesystem driver
+     *
+     * @deprecated
+     * @return Filesystem
+     */
+    private function getFilesystem()
+    {
+        if (!$this->filesystem) {
+            $this->filesystem = $this->objectManager->get(Filesystem::class);
+        }
+        return $this->filesystem;
+    }
 }

lib/internal/Magento/Framework/App/Test/Unit/BootstrapTest.php

@@ -162,26 +162,35 @@ public function testGetObjectManager()
         $this->assertSame($this->objectManager, $bootstrap->getObjectManager());
     }
 
-    public function testIsDeveloperMode()
+    /**
+     * @param $modeFromEnvironment
+     * @param $modeFromDeployment
+     * @param $isDeveloper
+     *
+     * @dataProvider testIsDeveloperModeDataProvider
+     */
+    public function testIsDeveloperMode($modeFromEnvironment, $modeFromDeployment, $isDeveloper)
     {
-        $bootstrap = self::createBootstrap();
-        $this->assertFalse($bootstrap->isDeveloperMode());
-        $testParams = [State::PARAM_MODE => State::MODE_DEVELOPER];
+        $testParams = [];
+        if ($modeFromEnvironment) {
+            $testParams[State::PARAM_MODE] = $modeFromEnvironment;
+        }
+        if ($modeFromDeployment) {
+            $this->deploymentConfig->method('get')->willReturn($modeFromDeployment);
+        }
         $bootstrap = self::createBootstrap($testParams);
-        $this->assertTrue($bootstrap->isDeveloperMode());
-        $this->deploymentConfig->expects($this->any())->method('get')->willReturn(State::MODE_DEVELOPER);
-        $bootstrap = self::createBootstrap();
-        $this->assertTrue($bootstrap->isDeveloperMode());
+        $this->assertEquals($isDeveloper, $bootstrap->isDeveloperMode());
     }
 
-    public function testIsDeveloperModeСontradictoryValues()
+    public function testIsDeveloperModeDataProvider()
     {
-        $this->deploymentConfig->expects($this->any())->method('get')->willReturn(State::MODE_PRODUCTION);
-        $bootstrap = self::createBootstrap();
-        $this->assertFalse($bootstrap->isDeveloperMode());
-        $testParams = [State::PARAM_MODE => State::MODE_DEVELOPER];
-        $bootstrap = self::createBootstrap($testParams);
-        $this->assertTrue($bootstrap->isDeveloperMode());
+        return [
+            [null, null, false],
+            [State::MODE_DEVELOPER, State::MODE_PRODUCTION, true],
+            [State::MODE_PRODUCTION, State::MODE_DEVELOPER, false],
+            [null, State::MODE_DEVELOPER, true],
+            [null, State::MODE_PRODUCTION, false]
+        ];
     }
 
     public function testRunNoErrors()

lib/internal/Magento/Framework/App/Test/Unit/StaticResourceTest.php

@@ -8,6 +8,9 @@
 
 namespace Magento\Framework\App\Test\Unit;
 
+use Magento\Framework\App\Bootstrap;
+use Magento\Framework\Filesystem;
+
 class StaticResourceTest extends \PHPUnit_Framework_TestCase
 {
     /**
@@ -188,17 +191,13 @@ public function testLaunchWrongPath()
         $this->object->launch();
     }
 
-    public function testCatchException()
+    public function testCatchExceptionDeveloperMode()
     {
-        $bootstrap = $this->getMock('Magento\Framework\App\Bootstrap', [], [], '', false);
-        $bootstrap->expects($this->at(0))->method('isDeveloperMode')->willReturn(false);
-        $bootstrap->expects($this->at(1))->method('isDeveloperMode')->willReturn(true);
-        $exception = new \Exception('message');
-        $this->response->expects($this->exactly(2))->method('setHttpResponseCode')->with(404);
-        $this->response->expects($this->exactly(2))->method('setHeader')->with('Content-Type', 'text/plain');
-        $this->response->expects($this->exactly(2))->method('sendResponse');
-        $this->response->expects($this->once())->method('setBody')->with($this->stringStartsWith('message'));
-        $this->assertTrue($this->object->catchException($bootstrap, $exception));
+        $bootstrap = $this->getMockBuilder(Bootstrap::class)->disableOriginalConstructor()->getMock();
+        $bootstrap->expects($this->once())->method('isDeveloperMode')->willReturn(true);
+        $exception = new \Exception('Error: nothing works');
+        $this->response->expects($this->once())->method('setHttpResponseCode')->with(404);
+        $this->response->expects($this->once())->method('sendResponse');
         $this->assertTrue($this->object->catchException($bootstrap, $exception));
     }
 }

nginx.conf.sample

@@ -26,6 +26,7 @@ root $MAGE_ROOT/pub;
 index index.php;
 autoindex off;
 charset UTF-8;
+error_page 404 403 = /errors/404.php;
 #add_header "X-UA-Compatible" "IE=Edge";
 
 location ~* ^/setup($|/) {
@@ -185,6 +186,7 @@ gzip_types
     image/svg+xml;
 gzip_vary on;
 
-location ~ \.php$ {
+# Banned locations (only reached if the earlier PHP entry point regexes don't match)
+location ~* (\.php$|\.htaccess$|\.git) {
     deny all;
 }

pub/get.php

@@ -45,13 +45,13 @@
         // Serve file if it's materialized
         if ($mediaDirectory) {
             if (!$isAllowed($relativePath, $allowedResources)) {
-                header('HTTP/1.0 404 Not Found');
+                require_once 'errors/404.php';
                 exit;
             }
             $mediaAbsPath = $mediaDirectory . '/' . $relativePath;
             if (is_readable($mediaAbsPath)) {
                 if (is_dir($mediaAbsPath)) {
-                    header('HTTP/1.0 404 Not Found');
+                    require_once 'errors/404.php';
                     exit;
                 }
                 $transfer = new \Magento\Framework\File\Transfer\Adapter\Http(