Fixed issue #17776: New security settings (#2198)

* Fixed issue #17776: New security settings - Add 'disablePluginUpload' config * Fixed issue #17776: New security settings - Check plugin whitelist config on plugin install, update and load (it was only checked on Scan Files) * Fixed issue #17776: New security settings - Fix default config * Fixed issue #17776: New security settings - Added comments - Make the Core plugin list static * Fixed issue #17776: New security settings - Update Core plugin list * Fixed issue #17776: New security settings - Simplify upload button display condition Co-authored-by: encuestabizdevgit <devgit@encuesta.biz>
LimeSurvey · Jan 31, 2022 · 94c6b65 · 94c6b65
1 parent f72cf44
commit 94c6b65
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 20 deletions.
diff --git a/application/config/config-defaults.php b/application/config/config-defaults.php
@@ -788,22 +788,19 @@
 // This is useful when developing a theme, so changes to XML files are immediately applied without the need to uninstall and reinstall the theme.
 $config['force_xmlsettings_for_survey_rendering'] = false;
 
+/**
+ * When this setting is true, plugins that are not in the white list (see 'pluginWhitelist') cannot be installed nor loaded. This may disable
+ * already installed plugins.
+ * Core plugins are not affected by this setting.
+ */
 $config['usePluginWhitelist'] = false;
 
-$config['pluginCoreList'] = [
-    'AuditLog',
-    'ExportR',
-    'ExportSTATAxml',
-    'ExportSPSSsav',
-    'extendedStartPage',
-    'oldUrlCompat',
-    'AuthLDAP',
-    'Authdb',
-    'Authwebserver'
-];
-
+// List of plugin names allowed to be installed and loaded when 'usePluginWhitelist' is true. Core plugins are implicitly whitelisted.
 $config['pluginWhitelist'] = [];
 
+// When this setting is true, the "Plugin Upload" feature is disabled.
+$config['disablePluginUpload'] = false;
+
 /* replaced in generated application/config/security.php if exist */
 $config['encryptionkeypair'] = '';
 $config['encryptionpublickey'] = '';

diff --git a/application/controllers/admin/PluginManagerController.php b/application/controllers/admin/PluginManagerController.php
@@ -85,6 +85,7 @@ public function index()
             'scanFiles' => [
                 'url' => $scanFilesUrl,
             ],
+            'showUpload' => !Yii::app()->getConfig('demoMode') && !Yii::app()->getConfig('disablePluginUpload'),
         ];
 
         $this->_renderWrappedTemplate('pluginmanager', 'index', $aData);
@@ -514,6 +515,8 @@ public function uninstallPlugin()
      */
     public function upload()
     {
+        $this->checkUploadEnabled();
+
         $this->checkUpdatePermission();
 
         // Redirect back if demo mode is set.
@@ -546,6 +549,8 @@ public function upload()
      */
     public function uploadConfirm()
     {
+        $this->checkUploadEnabled();
+
         $this->checkUpdatePermission();
 
         /** @var PluginInstaller */
@@ -561,6 +566,11 @@ public function uploadConfirm()
                 $this->errorAndRedirect(gT('Could not read plugin configuration file.'));
             }
 
+            if (!$installer->isWhitelisted()) {
+                $installer->abort();
+                $this->errorAndRedirect(gT('The plugin is not in the plugin whitelist.'));
+            }
+
             if (!$config->isCompatible()) {
                 $installer->abort();
                 $this->errorAndRedirect(gT('The plugin is not compatible with your version of LimeSurvey.'));
@@ -593,6 +603,8 @@ public function uploadConfirm()
      */
     public function installUploadedPlugin()
     {
+        $this->checkUploadEnabled();
+
         $this->checkUpdatePermission();
 
         /** @var LSHttpRequest */
@@ -749,6 +761,18 @@ protected function checkUpdatePermission()
         }
     }
 
+    /**
+     * Blocks action if plugin upload is disabled.
+     * @return void
+     */
+    protected function checkUploadEnabled()
+    {
+        if (Yii::app()->getConfig('disablePluginUpload')) {
+            Yii::app()->setFlashMessage(gT('Plugin upload is disabled'), 'error');
+            $this->getController()->redirect($this->getPluginManagerUrl());
+        }
+    }
+
     /**
      * Renders template(s) wrapped in header and footer
      *

diff --git a/application/libraries/ExtensionInstaller/PluginInstaller.php b/application/libraries/ExtensionInstaller/PluginInstaller.php
@@ -29,6 +29,10 @@ public function install()
             throw new InvalidArgumentException('fileFetcher is not set');
         }
 
+        if (!$this->isWhitelisted()) {
+            throw new Exception('The plugin is not in the plugin whitelist.');
+        }
+
         $config = $this->getConfig();
         $pluginManager = App()->getPluginManager();
         $destdir = $pluginManager->getPluginFolder($config, $this->pluginType);
@@ -57,6 +61,10 @@ public function update()
             throw new InvalidArgumentException('fileFetcher is not set');
         }
 
+        if (!$this->isWhitelisted()) {
+            throw new Exception('The plugin is not in the plugin whitelist.');
+        }
+
         $config = $this->getConfig();
         $plugin = \Plugin::model()->find('name = :name', [':name' => $config->getName()]);
 
@@ -91,4 +99,21 @@ public function setPluginType($pluginType)
     {
         $this->pluginType = $pluginType;
     }
+
+    /**
+     * Returns true if the plugin name is whitelisted or the whitelist is disabled.
+     * @return boolean
+     */
+    public function isWhitelisted()
+    {
+        if (empty($this->fileFetcher)) {
+            throw new InvalidArgumentException('fileFetcher is not set');
+        }
+
+        $config = $this->getConfig();
+        $pluginName = $config->getName();
+        $pluginManager = App()->getPluginManager();
+
+        return $pluginManager->isWhitelisted($pluginName);
+    }
 }
diff --git a/application/libraries/PluginManager/PluginManager.php b/application/libraries/PluginManager/PluginManager.php
@@ -137,6 +137,10 @@ public function installPlugin(\ExtensionConfig $extensionConfig, $pluginType)
         }
 
         $newName = (string) $extensionConfig->xml->metadata->name;
+        if (!$this->isWhitelisted($newName)) {
+            return [false, gT('The plugin is not in the plugin whitelist.')];
+        }
+
         $otherPlugin = Plugin::model()->findAllByAttributes(['name' => $newName]);
         if (!empty($otherPlugin)) {
             return [false, sprintf(gT('Extension "%s" is already installed.'), $newName)];
@@ -298,7 +302,7 @@ public function scanPlugins($includeInstalledPlugins = false)
                             empty($plugin)
                             || ($includeInstalledPlugins && $plugin->load_error == 0)
                         ) {
-                            if (file_exists($file) && $this->_checkWhitelist($pluginName)) {
+                            if (file_exists($file) && $this->isWhitelisted($pluginName)) {
                                 try {
                                     $result[$pluginName] = $this->getPluginInfo($pluginName, $pluginDir);
                                     // getPluginInfo returns false instead of an array when config is not found.
@@ -437,7 +441,7 @@ public function loadPlugin($pluginName, $id = null)
                 }
             } else {
                 if (!isset($this->plugins[$id]) || get_class($this->plugins[$id]) !== $pluginName) {
-                    if ($this->getPluginInfo($pluginName) !== false) {
+                    if ($this->isWhitelisted($pluginName) && $this->getPluginInfo($pluginName) !== false) {
                         if (class_exists($pluginName)) {
                             $this->plugins[$id] = new $pluginName($this, $id);
                             if (method_exists($this->plugins[$id], 'init')) {
@@ -634,17 +638,47 @@ protected function getPluginName(string $class, \ExtensionConfig $extensionConfi
     }
 
     /**
+     * Returns true if the plugin name is whitelisted or the whitelist is disabled.
      * @param string $pluginName
      * @return boolean
      */
-    private function _checkWhitelist($pluginName)
+    public function isWhitelisted($pluginName)
     {
         if (App()->getConfig('usePluginWhitelist')) {
             $whiteList = App()->getConfig('pluginWhitelist');
-            $coreList = App()->getConfig('pluginCoreList');
+            $coreList = self::getCorePluginList();
             $allowedPlugins =  array_merge($coreList, $whiteList);
             return array_search($pluginName, $allowedPlugins) !== false;
         }
         return true;
     }
+
+    /**
+     * Return the core plugin list
+     * No way to update by php or DB
+     * @return string[]
+     */
+    private static function getCorePluginList()
+    {
+        return [
+            'AuditLog',
+            'Authdb',
+            'AuthLDAP',
+            'Authwebserver',
+            'ComfortUpdateChecker',
+            'customToken',
+            'ExportR',
+            'ExportSPSSsav',
+            'ExportSTATAxml',
+            'expressionFixedDbVar',
+            'expressionQuestionForAll',
+            'expressionQuestionHelp',
+            'mailSenderToFrom',
+            'oldUrlCompat',
+            'PasswordRequirement',
+            'statFunctions',
+            'TwoFactorAdminLogin',
+            'UpdateCheck'
+        ];
+    }
 }
diff --git a/application/views/admin/super/fullpagebar_view.php b/application/views/admin/super/fullpagebar_view.php
@@ -15,10 +15,7 @@
             <!-- Plugin Manager -->
             <?php if (isset($fullpagebar['pluginManager'])) : ?>
                 <!-- Install Plugin Zip -->
-                <?php if (
-                    isset($fullpagebar['pluginManager']['buttons']['installPluginZipModal']['hasConfigDemoMode']) &&
-                    !$fullpagebar['pluginManager']['buttons']['installPluginZipModal']['hasConfigDemoMode']
-                ) : ?>
+                <?php if (!empty($fullpagebar['pluginManager']['buttons']['showUpload'])) : ?>
                     <a
                         href=''
                         class='btn btn-default'