Winter CMS Modules allows a sandbox bypass in Twig templates leading to data modification and deletion

@@ -1,22 +1,25 @@
-<?php namespace Cms\Classes;
+<?php
+
+namespace Cms\Classes;
 
-use App;
-use ApplicationException;
-use Cache;
 use Cms\Models\ThemeData;
-use Config;
 use DirectoryIterator;
-use Event;
 use Exception;
-use File;
-use Lang;
+use Illuminate\Support\Facades\App;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Lang;
 use System\Models\Parameter;
-use SystemException;
-use Url;
+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 Yaml;
+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;
+use Winter\Storm\Support\Str;
 
 /**
  * This class represents the CMS theme.
@@ -682,6 +685,11 @@ public function getDatasource(): DatasourceInterface
      */
     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};
         }
@@ -694,6 +702,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);

@@ -1,11 +1,13 @@
-<?php namespace Cms\Models;
+<?php
+
+namespace Cms\Models;
 
-use Lang;
-use Model;
 use Cms\Classes\Theme as CmsTheme;
-use System\Classes\CombineAssets;
 use Exception;
+use Illuminate\Support\Facades\Lang;
+use System\Classes\CombineAssets;
 use System\Models\File;
+use Winter\Storm\Database\Model;
 
 /**
  * Customization data used by a theme

@@ -0,0 +1,269 @@
+<?php
+
+namespace System\Tests\Twig;
+
+use Cms\Classes\Controller;
+use Cms\Classes\Page;
+use Cms\Classes\Theme;
+use System\Tests\Bootstrap\TestCase;
+use Twig\Environment;
+use Winter\Storm\Filesystem\Filesystem;
+use Winter\Storm\Halcyon\Datasource\FileDatasource;
+
+class SecurityPolicyTest extends TestCase
+{
+    protected Environment $twig;
+
+    public function testCannotGetTwigInstanceFromCmsController()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set twig = this.controller.getTwig() %}
+            {{ this.controller.getTwig() }}
+        ');
+    }
+
+    public function testCannotGetTwigLoaderFromCmsController()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set loader = this.controller.getLoader() %}
+            {{ loader.load(\'/\') }}
+        ');
+    }
+
+    public function testCannotRunAPageObjectFromWithinTwig()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {{ this.controller.runPage() }}
+        ');
+    }
+
+    public function testCannotExtendAPageWithADynamicMethod()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set page = this.page.addDynamicMethod("test") %}
+        ');
+    }
+
+    public function testCannotExtendAPageWithADynamicProperty()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set page = this.page.addDynamicProperty("test", "value") %}
+        ');
+    }
+
+    public function testCannotWriteToAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.setAttribute("test", "value") %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testCanReadFromAModel()
+    {
+        $model = new \Winter\Storm\Database\Model();
+        $model->test = 'value';
+
+        $result = trim($this->renderTwigInCmsController('
+            {% set modelTest = model.getAttribute("test") %}
+            {{- modelTest -}}
+        ', [
+            'model' => $model,
+        ]));
+        $this->assertEquals('value', $result);
+    }
+
+    public function testCannotFillAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        try {
+            $model = new \Winter\Storm\Database\Model();
+            $model->addFillable('test');
+            $model->test = 'value';
+
+            $this->renderTwigInCmsController('
+                {% set modelTest = model.fill({ test: \'value2\' }) %}
+            ', [
+                'model' => new \Winter\Storm\Database\Model(),
+            ]);
+        } catch (\Twig\Sandbox\SecurityNotAllowedMethodError $e) {
+            // Ensure value hasn't changed
+            $this->assertEquals('value', $model->test);
+            throw $e;
+        }
+    }
+
+    public function testCannotSaveAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.save() %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testCannotPushAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.push() %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testCannotUpdateAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $model = new \Winter\Storm\Database\Model();
+        $model->addFillable('test');
+        $model->test = 'value';
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.update({ test: \'value2\' }) %}
+        ', [
+            'model' => $model,
+        ]);
+    }
+
+    public function testCannotDeleteAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.delete() %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testCannotForceDeleteAModel()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set modelTest = model.forceDelete() %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testCannotExtendAModelWithABehaviour()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set model = model.extendClassWith("Winter\Storm\Database\Behaviors\Encryptable") %}
+        ', [
+            'model' => new \Winter\Storm\Database\Model(),
+        ]);
+    }
+
+    public function testExtendingModelBeforePassingIntoTwigShouldStillWork()
+    {
+        $model = new \Winter\Storm\Database\Model();
+        $model->addDynamicMethod('foo', function () {
+            return 'foo';
+        });
+
+        $result = trim($this->renderTwigInCmsController('
+            {{- model.foo() -}}
+        ', [
+            'model' => $model,
+        ]));
+        $this->assertEquals('foo', $result);
+    }
+
+    public function testCannotGetDatasourceFromTheme()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set datasource = this.theme.getDatasource() %}
+        ');
+    }
+
+    // Even if someone decides to be clever and make the datasource available, you shouldn't be able to insert/delete/update
+    public function testCannotDeleteInDatasource()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set datasource = datasource.delete() %}
+        ', [
+            'datasource' => new FileDatasource(
+                base_path('modules/system/tests/fixtures/themes/test'),
+                new Filesystem()
+            ),
+        ]);
+    }
+
+    public function testCannotInsertInDatasource()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set datasource = datasource.insert() %}
+        ', [
+            'datasource' => new FileDatasource(
+                base_path('modules/system/tests/fixtures/themes/test'),
+                new Filesystem()
+            ),
+        ]);
+    }
+
+    public function testCannotUpdateInDatasource()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set datasource = datasource.update() %}
+        ', [
+            'datasource' => new FileDatasource(
+                base_path('modules/system/tests/fixtures/themes/test'),
+                new Filesystem()
+            ),
+        ]);
+    }
+
+    public function testCannotChangeThemeDirectory()
+    {
+        $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class);
+
+        $this->renderTwigInCmsController('
+            {% set theme = this.theme.setDirName("test") %}
+        ');
+    }
+
+    protected function renderTwigInCmsController(string $source, array $vars = [])
+    {
+        $controller = new Controller();
+        $twig = $controller->getTwig();
+        $template = $twig->createTemplate($source, 'test.case');
+        return $twig->render($template, [
+            'this' => [
+                'controller' => $controller,
+                'page' => new Page(),
+                'theme' => new Theme()
+            ],
+        ] + $vars);
+    }
+}

@@ -1,48 +1,115 @@
-<?php namespace System\Twig;
+<?php
 
+namespace System\Twig;
+
+use Cms\Classes\Controller;
+use Cms\Classes\Theme;
+use Illuminate\Database\Eloquent\Model as DbModel;
+use Winter\Storm\Halcyon\Model as HalcyonModel;
 use Twig\Markup;
+use Twig\Sandbox\SecurityNotAllowedFunctionError;
 use Twig\Template;
 use Twig\Sandbox\SecurityPolicyInterface;
 use Twig\Sandbox\SecurityNotAllowedMethodError;
 use Twig\Sandbox\SecurityNotAllowedPropertyError;
+use Twig\Sandbox\SecurityNotAllowedTagError;
+use Winter\Storm\Halcyon\Datasource\DatasourceInterface;
 
 /**
  * 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 = [
-        // Prevent accessing Twig itself
-        'getTwig',
-
-        // \Winter\Storm\Extension\ExtendableTrait
-        'addDynamicMethod',
-        'addDynamicProperty',
-
-        // \Winter\Storm\Support\Traits\Emitter
-        'bindEvent',
-        'bindEventOnce',
-
-        // Eloquent & Halcyon data modification
-        'insert',
-        'update',
-        'delete',
-        'write',
+        '*' => [
+            // 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'
+        ],
+    ];
+
+    /**
+     * @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);
         }
     }
 
@@ -69,6 +136,19 @@ public function checkSecurity($tags, $filters, $functions): void
      */
     public function checkPropertyAllowed($obj, $property): void
     {
+        // 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);
+            }
+        }
     }
 
     /**
@@ -85,10 +165,20 @@ public function checkMethodAllowed($obj, $method): void
             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);
+            }
+        }
     }
 }