Backport fb88e6f to 1.0

* Harden theme objects, prevent certain properties from being passed through to ThemeData object * Improve and properly scope Twig security policy - Block methods that write, delete or modify records and attributes in Database/Eloquent and Halcyon models - Block access to theme datasource - Prevent extensions from being created or directly interacted with (models and properties provided to extended objects should still be OK) Refs: fb88e6f#diff-347d3e6f6f84697f5be048027169529a5ed7e782fcf2dcf62dcdbf560a0a4f77
wintercms · Oct 16, 2024 · 3532a05 · 3532a05
1 parent 5811af2
commit 3532a05
Show file tree

Hide file tree

Showing 2 changed files with 129 additions and 32 deletions.
diff --git a/modules/cms/classes/Theme.php b/modules/cms/classes/Theme.php
@@ -1,22 +1,24 @@
-<?php namespace Cms\Classes;
-
-use App;
-use Url;
-use File;
-use Yaml;
-use Lang;
-use Cache;
-use Event;
-use Config;
-use Exception;
-use SystemException;
-use DirectoryIterator;
-use ApplicationException;
+<?php
+
+namespace Cms\Classes;
+
 use Cms\Models\ThemeData;
+use DirectoryIterator;
+use Exception;
+use Illuminate\Support\Facades\App;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Lang;
 use System\Models\Parameter;
+use Winter\Storm\Exception\ApplicationException;
+use Winter\Storm\Exception\SystemException;
+use Winter\Storm\Halcyon\Datasource\DatasourceInterface;
 use Winter\Storm\Halcyon\Datasource\DbDatasource;
 use Winter\Storm\Halcyon\Datasource\FileDatasource;
-use Winter\Storm\Halcyon\Datasource\DatasourceInterface;
+use Winter\Storm\Support\Facades\Config;
+use Winter\Storm\Support\Facades\Event;
+use Winter\Storm\Support\Facades\File;
+use Winter\Storm\Support\Facades\Url;
+use Winter\Storm\Support\Facades\Yaml;
 
 /**
  * This class represents the CMS theme.
@@ -582,6 +584,11 @@ public function getDatasource()
      */
     public function __get($name)
     {
+        if (in_array(strtolower($name), ['id', 'path', 'dirname', 'config', 'formconfig', 'previewimageurl'])) {
+            $method = 'get'. ucfirst($name);
+            return $this->$method();
+        }
+
         if ($this->hasCustomData()) {
             return $this->getCustomData()->{$name};
         }
@@ -596,6 +603,10 @@ public function __get($name)
      */
     public function __isset($key)
     {
+        if (in_array(strtolower($key), ['id', 'path', 'dirname', 'config', 'formconfig', 'previewimageurl'])) {
+            return true;
+        }
+
         if ($this->hasCustomData()) {
             $theme = $this->getCustomData();
             return $theme->offsetExists($key);

diff --git a/modules/system/twig/SecurityPolicy.php b/modules/system/twig/SecurityPolicy.php
@@ -1,44 +1,107 @@
 <?php namespace System\Twig;
 
+use Cms\Classes\Controller;
+use Cms\Classes\Theme;
+use Illuminate\Database\Eloquent\Model as DbModel;
 use Twig\Markup;
 use Twig\Template;
 use Twig\Sandbox\SecurityPolicyInterface;
 use Twig\Sandbox\SecurityNotAllowedMethodError;
 use Twig\Sandbox\SecurityNotAllowedPropertyError;
+use Winter\Storm\Halcyon\Datasource\DatasourceInterface;
+use Winter\Storm\Halcyon\Model as HalcyonModel;
 
 /**
  * SecurityPolicy globally blocks accessibility of certain methods and properties.
  *
  * @package winter\wn-system-module
- * @author Alexey Bobkov, Samuel Georges, Luke Towers
+ * @author Alexey Bobkov, Samuel Georges, Luke Towers, Ben Thomson
  */
 final class SecurityPolicy implements SecurityPolicyInterface
 {
     /**
-     * @var array List of forbidden methods.
+     * @var array<string, string[]> List of forbidden methods, grouped by applicable instance.
      */
     protected $blockedMethods = [
-        // \Winter\Storm\Extension\ExtendableTrait
-        'addDynamicMethod',
-        'addDynamicProperty',
-
-        // \Winter\Storm\Support\Traits\Emitter
-        'bindEvent',
-        'bindEventOnce',
+        '*' => [
+            // Prevent accessing Twig itself
+            'getTwig',
+            // Prevent extensions of any objects
+            'addDynamicMethod',
+            'addDynamicProperty',
+            'extendClassWith',
+            'getClassExtension',
+            'extendableSet',
+            // Prevent binding to events
+            'bindEvent',
+            'bindEventOnce',
+        ],
+        // Prevent some controller methods
+        Controller::class => [
+            'runPage',
+            'renderPage',
+            'getLoader',
+        ],
+        // Prevent model data modification
+        DbModel::class => [
+            'fill',
+            'setAttribute',
+            'setRawAttributes',
+            'save',
+            'push',
+            'update',
+            'delete',
+            'forceDelete',
+        ],
+        HalcyonModel::class => [
+            'fill',
+            'setAttribute',
+            'setRawAttributes',
+            'setSettingsAttribute',
+            'setFileNameAttribute',
+            'save',
+            'push',
+            'update',
+            'delete',
+            'forceDelete',
+        ],
+        DatasourceInterface::class => [
+            'insert',
+            'update',
+            'delete',
+            'forceDelete',
+            'write',
+            'usingSource',
+            'pushToSource',
+            'removeFromSource',
+        ],
+        Theme::class => [
+            'setDirName',
+            'registerHalcyonDatasource',
+            'getDatasource'
+        ],
+    ];
 
-        // Eloquent & Halcyon data modification
-        'insert',
-        'update',
-        'delete',
+    /**
+     * @var array<string, string[]> List of forbidden properties, grouped by applicable instance.
+     */
+    protected $blockedProperties = [
+        Theme::class => [
+            'datasource',
+        ],
     ];
 
     /**
      * Constructor
      */
     public function __construct()
     {
-        foreach ($this->blockedMethods as $i => $m) {
-            $this->blockedMethods[$i] = strtolower($m);
+        foreach ($this->blockedMethods as $type => $methods) {
+            $this->blockedMethods[$type] = array_map('strtolower', $methods);
+        }
+
+        foreach ($this->blockedProperties as $type => $properties) {
+            $this->blockedProperties[$type] = array_map('strtolower', $properties);
         }
     }
 
@@ -65,6 +128,19 @@ public function checkSecurity($tags, $filters, $functions)
      */
     public function checkPropertyAllowed($obj, $property)
     {
+        // No need to check Twig internal objects
+        if ($obj instanceof Template || $obj instanceof Markup) {
+            return;
+        }
+
+        $property = strtolower($property);
+
+        foreach ($this->blockedProperties as $type => $properties) {
+            if ($obj instanceof $type && in_array($property, $properties)) {
+                $class = get_class($obj);
+                throw new SecurityNotAllowedPropertyError(sprintf('Getting "%s" property in a "%s" object is blocked.', $property, $class), $class, $property);
+            }
+        }
     }
 
     /**
@@ -81,10 +157,20 @@ public function checkMethodAllowed($obj, $method)
             return;
         }
 
-        $blockedMethod = strtolower($method);
-        if (in_array($blockedMethod, $this->blockedMethods)) {
+        $method = strtolower($method);
+
+        if (
+            in_array($method, $this->blockedMethods['*'])
+        ) {
             $class = get_class($obj);
             throw new SecurityNotAllowedMethodError(sprintf('Calling "%s" method on a "%s" object is blocked.', $method, $class), $class, $method);
         }
+
+        foreach ($this->blockedMethods as $type => $methods) {
+            if ($obj instanceof $type && in_array($method, $methods)) {
+                $class = get_class($obj);
+                throw new SecurityNotAllowedMethodError(sprintf('Calling "%s" method on a "%s" object is blocked.', $method, $class), $class, $method);
+            }
+        }
     }
 }