New feature #18375: [security] Some plugin settings need encryption (#…

…2645)

Co-authored-by: encuestabizdevgit <devgit@encuesta.biz>
Co-authored-by: Michael Hernandez <michael.hernandez@bairesdev.com>
Co-authored-by: lapiudevgit <devgit@lapiu.biz>

application/libraries/PluginManager/PluginBase.php

@@ -2,6 +2,8 @@
 
 namespace LimeSurvey\PluginManager;
 
+use LSActiveRecord;
+
 /**
  * Base class for plugins.
  */
@@ -72,6 +74,12 @@ abstract class PluginBase implements iPlugin
      */
     public $allowedPublicMethods = null;
 
+    /**
+     * List of settings that should be encrypted before saving.
+     * @var string[]
+     */
+    protected $encryptedSettings = [];
+
     /**
      * Constructor for the plugin
      * @todo Add proper type hint in 3.0
@@ -133,7 +141,21 @@ protected function setLocaleComponent()
      */
     protected function get($key = null, $model = null, $id = null, $default = null)
     {
-        return $this->getStore()->get($this, $key, $model, $id, $default);
+        $data = $this->getStore()->get($this, $key, $model, $id, $default);
+        // Decrypt the attribute if needed
+        // TODO: Handle decryption in storage class, as that would allow each storage to handle
+        // it on it's own way. Currently there is no good way of telling the storage which
+        // attributes should be encrypted. Adding a method to the storage interface would break
+        // backward compatibility. See https://bugs.limesurvey.org/view.php?id=18375#c72133
+        if (!empty($data) && in_array($key, $this->encryptedSettings)) {
+            try {
+                $json = LSActiveRecord::decryptSingle($data);
+                $data = !empty($json) ? json_decode($json, true) : $json;
+            } catch (\Throwable $e) {
+                // If decryption fails, just leave the value untouched (it was probably saved as plain text)
+            }
+        }
+        return $data;
     }
 
     /**
@@ -273,6 +295,15 @@ public function saveSettings($settings)
      */
     protected function set($key, $data, $model = null, $id = null)
     {
+        // Encrypt the attribute if needed
+        // TODO: Handle encryption in storage class, as that would allow each storage to handle
+        // it on it's own way. Currently there is no good way of telling the storage which
+        // attributes should be encrypted. Adding a method to the storage interface would break
+        // backward compatibility. See https://bugs.limesurvey.org/view.php?id=18375#c72133
+        if (!empty($data) && in_array($key, $this->encryptedSettings)) {
+            // Data is json encoded before encryption because it might be an array or object.
+            $data = LSActiveRecord::encryptSingle(json_encode($data));
+        }
         return $this->getStore()->set($this, $key, $data, $model, $id);
     }
 

tests/data/plugins/SettingsPlugin.php

@@ -0,0 +1,28 @@
+<?php
+
+class SettingsPlugin extends PluginBase
+{
+    protected static $description = 'Dummy plugin for testing SettingsPlugin event';
+    protected static $name = 'SettingsPlugin';
+    protected $storage = 'DbStorage';
+    protected $encryptedSettings = [];
+
+    public function init()
+    {
+    }
+
+    public function setSetting($name, $value)
+    {
+        return $this->set($name, $value);
+    }
+
+    public function getSetting($name)
+    {
+        return $this->get($name);
+    }
+
+    public function setEncriptedSettings($encryptedSettings)
+    {
+        $this->encryptedSettings = $encryptedSettings;
+    }
+}

tests/functional/backend/SettingsPluginTest.php

@@ -0,0 +1,117 @@
+<?php
+
+namespace ls\tests;
+
+use LSActiveRecord;
+
+/**
+ * @group api
+ */
+class SettingsPluginTest extends TestBaseClass
+{
+
+    protected static $plugin;
+
+    protected static $settings = [];
+
+    /**
+     * @inheritdoc
+     * Activate needed plugins
+     * Import survey in tests/surveys/.
+     */
+    public static function setUpBeforeClass(): void
+    {
+        parent::setUpBeforeClass();
+
+        require_once __DIR__."/../../data/plugins/SettingsPlugin.php";
+        $plugin = \Plugin::model()->findByAttributes(array('name'=>'SettingsPlugin'));
+        if (!$plugin) {
+            $plugin = new \Plugin();
+            $plugin->name = 'SettingsPlugin';
+            $plugin->active = 1;
+            $plugin->save();
+        } else {
+            $plugin->active = 1;
+            $plugin->save();
+        }
+
+        self::$plugin = App()->getPluginManager()->loadPlugin('SettingsPlugin', $plugin->id);
+
+        $obj = new \stdClass();
+        $obj->customProperty = 'abc';
+
+        self::$settings = [
+            'empty_1' => 0,
+            'empty_2' => null,
+            'empty_3' => false,
+            'empty_4' => "",
+            'empty_5' => [],
+            'empty_6' => (object) [],
+            'empty_7' => new \stdClass(),
+            'number' => rand(100,999),
+            'decimal' => rand(100,999) * 0.5,
+            'string' => 'abc',
+            'string_2' => 'ABC',
+            'especial_character_1' => 'ñ',
+            'especial_character_2' => 'á',
+            'especial_character_3' => 'é',
+            'especial_character_4' => 'í',
+            'especial_character_5' => 'ó',
+            'especial_character_6' => 'ú',
+            'especial_character_7' => 'Ñ',
+            'especial_character_8' => 'Á',
+            'especial_character_9' => 'É',
+            'especial_character_10' => 'Í',
+            'especial_character_11' => 'Ó',
+            'especial_character_12' => 'Ú',
+            'array_1' => ['a', 'b', 'c'],
+            'array_2' => [1,2,3],
+            'object_1' => (object) ['property' => 'Here we go'],
+            'object_2' => $obj,
+        ];
+    }
+
+    public function testGetAndSetSetting()
+    {
+        foreach (self::$settings as $key => $value) {
+            self::$plugin->setSetting($key, $value);
+
+            $setting = \PluginSetting::model()->findByAttributes([
+                'plugin_id' => self::$plugin->getId(),
+                'key' => $key
+            ]);
+
+            $this->assertNotEmpty($setting->id);
+            $this->assertEquals($setting->value, json_encode($value) );
+
+            $settingValue = self::$plugin->getSetting($key);
+            $this->assertEquals($settingValue, json_decode(json_encode($value), true));
+        }
+    }
+
+    public function testGetAndSetSettingEncripted()
+    {
+        self::$plugin->setEncriptedSettings(array_keys(self::$settings));
+        foreach (self::$settings as $key => $value) {
+            self::$plugin->setSetting($key, $value);
+
+            $setting = \PluginSetting::model()->findByAttributes([
+                'plugin_id' => self::$plugin->getId(),
+                'key' => $key
+            ]);
+
+            $this->assertNotEmpty($setting->id);
+            if (empty($value)) {
+                $this->assertEquals($setting->value, json_encode($value) );
+            } else {
+                $this->assertEquals($setting->value, 
+                    json_encode(LSActiveRecord::encryptSingle(json_encode($value)))
+                );
+            }
+
+            $settingValue = self::$plugin->getSetting($key);
+            $this->assertEquals($settingValue, json_decode(json_encode($value), true) );
+        }
+    }
+
+}