From fe2098376ff0a8f70aa44510cd534671bf1e50c2 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Fri, 17 Jan 2020 17:15:37 +0100 Subject: [PATCH 1/9] Prepare PostProcessingPlugin for security. --- plugins/PostProcessingPlugin/PostProcessingPlugin.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index 8be0523b65..bb1d42c42e 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -4,6 +4,7 @@ from PyQt5.QtCore import QObject, pyqtProperty, pyqtSignal, pyqtSlot from typing import Dict, Type, TYPE_CHECKING, List, Optional, cast +from UM.Trust import Trust from UM.PluginRegistry import PluginRegistry from UM.Resources import Resources from UM.Application import Application @@ -161,7 +162,12 @@ class PostProcessingPlugin(QObject, Extension): # Iterate over all scripts. if script_name not in sys.modules: try: - spec = importlib.util.spec_from_file_location(__name__ + "." + script_name, os.path.join(path, script_name + ".py")) + file_location = __name__ + "." + script_name, os.path.join(path, script_name + ".py") + trust_instance = Trust.getInstanceOrNone() + if trust_instance is not None and Trust.signatureFileExistsFor(file_location): + if not trust_instance.signedFileCheck(file_location): + raise Exception("Can't validate script {0}".format(file_location)) + spec = importlib.util.spec_from_file_location(file_location) loaded_script = importlib.util.module_from_spec(spec) if spec.loader is None: continue From 935c71eddade5b1468a61f148c989a4149b6d438 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Fri, 17 Jan 2020 17:25:00 +0100 Subject: [PATCH 2/9] Fix name/file-location oops. --- plugins/PostProcessingPlugin/PostProcessingPlugin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index bb1d42c42e..826b655988 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -162,12 +162,12 @@ class PostProcessingPlugin(QObject, Extension): # Iterate over all scripts. if script_name not in sys.modules: try: - file_location = __name__ + "." + script_name, os.path.join(path, script_name + ".py") + file_location = os.path.join(path, script_name + ".py") trust_instance = Trust.getInstanceOrNone() if trust_instance is not None and Trust.signatureFileExistsFor(file_location): if not trust_instance.signedFileCheck(file_location): raise Exception("Can't validate script {0}".format(file_location)) - spec = importlib.util.spec_from_file_location(file_location) + spec = importlib.util.spec_from_file_location(__name__ + "." + script_name, file_location) loaded_script = importlib.util.module_from_spec(spec) if spec.loader is None: continue From 5b045f89b174e63763b787e5de40868c393ecf26 Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Tue, 24 Mar 2020 16:24:24 +0100 Subject: [PATCH 3/9] Finish postprocessing script signature checking CURA-7319 --- .../PostProcessingPlugin.py | 53 ++++++++++----- .../tests/TestPostProcessingPlugin.py | 65 +++++++++++++++++++ .../PostProcessingPlugin/tests/__init__.py | 0 3 files changed, 101 insertions(+), 17 deletions(-) create mode 100644 plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py create mode 100644 plugins/PostProcessingPlugin/tests/__init__.py diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index 826b655988..8cfe730d95 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -1,24 +1,24 @@ # Copyright (c) 2018 Jaime van Kessel, Ultimaker B.V. # The PostProcessingPlugin is released under the terms of the AGPLv3 or higher. -from PyQt5.QtCore import QObject, pyqtProperty, pyqtSignal, pyqtSlot -from typing import Dict, Type, TYPE_CHECKING, List, Optional, cast - -from UM.Trust import Trust -from UM.PluginRegistry import PluginRegistry -from UM.Resources import Resources -from UM.Application import Application -from UM.Extension import Extension -from UM.Logger import Logger - import configparser # The script lists are stored in metadata as serialised config files. +import importlib.util import io # To allow configparser to write to a string. import os.path import pkgutil import sys -import importlib.util +from typing import Dict, Type, TYPE_CHECKING, List, Optional, cast +from PyQt5.QtCore import QObject, pyqtProperty, pyqtSignal, pyqtSlot + +from UM.Application import Application +from UM.Extension import Extension +from UM.Logger import Logger +from UM.PluginRegistry import PluginRegistry +from UM.Resources import Resources +from UM.Trust import Trust from UM.i18n import i18nCatalog +from cura import ApplicationMetadata from cura.CuraApplication import CuraApplication i18n_catalog = i18nCatalog("cura") @@ -162,12 +162,13 @@ class PostProcessingPlugin(QObject, Extension): # Iterate over all scripts. if script_name not in sys.modules: try: - file_location = os.path.join(path, script_name + ".py") - trust_instance = Trust.getInstanceOrNone() - if trust_instance is not None and Trust.signatureFileExistsFor(file_location): - if not trust_instance.signedFileCheck(file_location): - raise Exception("Can't validate script {0}".format(file_location)) - spec = importlib.util.spec_from_file_location(__name__ + "." + script_name, file_location) + file_path = os.path.join(path, script_name + ".py") + if not self._isScriptAllowed(file_path): + Logger.warning("Skipped loading post-processing script {}: not trusted".format(file_path)) + continue + + spec = importlib.util.spec_from_file_location(__name__ + "." + script_name, + file_path) loaded_script = importlib.util.module_from_spec(spec) if spec.loader is None: continue @@ -340,4 +341,22 @@ class PostProcessingPlugin(QObject, Extension): if global_container_stack is not None: global_container_stack.propertyChanged.emit("post_processing_plugin", "value") + @staticmethod + def _isScriptAllowed(file_path) -> bool: + """Checks whether the given file is allowed to be loaded""" + if not ApplicationMetadata.IsEnterpriseVersion: + # No signature needed + return True + + if os.path.split(file_path) == os.path.join(Resources.getStoragePath(Resources.Resources), "scripts"): + # Bundled scripts are trusted. + return True + + trust_instance = Trust.getInstanceOrNone() + if trust_instance is not None and Trust.signatureFileExistsFor(file_path): + if trust_instance.signedFileCheck(file_path): + return True + + return False # Default verdict should be False, being the most secure fallback + diff --git a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py new file mode 100644 index 0000000000..241b7ef07d --- /dev/null +++ b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py @@ -0,0 +1,65 @@ + +import os +import sys +from unittest.mock import patch, MagicMock + +from pytest import fixture + +from UM.Resources import Resources +from UM.Trust import Trust +from ..PostProcessingPlugin import PostProcessingPlugin + +# not sure if needed +sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) + +""" In this file, commnunity refers to regular Cura for makers.""" + + +# noinspection PyProtectedMember +@patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) +def test_community_user_script_allowed(): + assert PostProcessingPlugin._isScriptAllowed("blaat.py") + +# noinspection PyProtectedMember +@patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) +def test_community_bundled_script_allowed(): + assert PostProcessingPlugin._isScriptAllowed(_bundled_file_path()) + +# noinspection PyProtectedMember +@patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) +def test_enterprise_unsigned_user_script_not_allowed(): + assert not PostProcessingPlugin._isScriptAllowed("blaat.py") + +@fixture +def mocked_get_instance_or_none(): + mocked_trust = MagicMock() + mocked_trust.signedFileCheck = MagicMock(return_value=True) + return mocked_trust + +@fixture +def mocked_get_signature_file_exists_for(): + return MagicMock(return_value=True) + +# noinspection PyProtectedMember +@patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) +@patch("UM.Trust", "signatureFileExistsFor") +@patch("UM.Trust.Trust.getInstanceOrNone") +def test_enterprise_signed_user_script_allowed(mocked_instance_or_none, mocked_get_instance_or_none): + file_path = "blaat.py" + realSignatureFileExistsFor = Trust.signatureFileExistsFor + Trust.signatureFileExistsFor = MagicMock(return_value=True) + assert PostProcessingPlugin._isScriptAllowed(file_path) + + # cleanup + Trust.signatureFileExistsFor = realSignatureFileExistsFor + +# noinspection PyProtectedMember +@patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) +def test_enterprise_bundled_script_allowed(): + assert PostProcessingPlugin._isScriptAllowed(_bundled_file_path()) + + +def _bundled_file_path(): + return os.path.join( + Resources.getStoragePath(Resources.Resources) + "scripts/blaat.py" + ) diff --git a/plugins/PostProcessingPlugin/tests/__init__.py b/plugins/PostProcessingPlugin/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 From c0d2977f4d4014e54562b03046f4b7d190b013de Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Tue, 24 Mar 2020 16:28:43 +0100 Subject: [PATCH 4/9] Cleanup test_enterprise_signed_user_script_allowed CURA-7319 --- .../tests/TestPostProcessingPlugin.py | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py index 241b7ef07d..a5ef9e47c9 100644 --- a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py @@ -3,8 +3,6 @@ import os import sys from unittest.mock import patch, MagicMock -from pytest import fixture - from UM.Resources import Resources from UM.Trust import Trust from ..PostProcessingPlugin import PostProcessingPlugin @@ -30,25 +28,17 @@ def test_community_bundled_script_allowed(): def test_enterprise_unsigned_user_script_not_allowed(): assert not PostProcessingPlugin._isScriptAllowed("blaat.py") -@fixture -def mocked_get_instance_or_none(): - mocked_trust = MagicMock() - mocked_trust.signedFileCheck = MagicMock(return_value=True) - return mocked_trust - -@fixture -def mocked_get_signature_file_exists_for(): - return MagicMock(return_value=True) - # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) -@patch("UM.Trust", "signatureFileExistsFor") -@patch("UM.Trust.Trust.getInstanceOrNone") -def test_enterprise_signed_user_script_allowed(mocked_instance_or_none, mocked_get_instance_or_none): - file_path = "blaat.py" +def test_enterprise_signed_user_script_allowed(): + mocked_trust = MagicMock() + mocked_trust.signedFileCheck = MagicMock(return_value=True) + realSignatureFileExistsFor = Trust.signatureFileExistsFor Trust.signatureFileExistsFor = MagicMock(return_value=True) - assert PostProcessingPlugin._isScriptAllowed(file_path) + + with patch("UM.Trust.Trust.getInstanceOrNone", return_value=mocked_trust): + assert PostProcessingPlugin._isScriptAllowed("blaat.py") # cleanup Trust.signatureFileExistsFor = realSignatureFileExistsFor From d5b58cf3b48cb25a166e1307ea29d0e8a3c3799b Mon Sep 17 00:00:00 2001 From: Jaime van Kessel Date: Wed, 25 Mar 2020 11:47:38 +0100 Subject: [PATCH 5/9] Add missing typing --- plugins/PostProcessingPlugin/PostProcessingPlugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index 8cfe730d95..de9b1905b7 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -342,7 +342,7 @@ class PostProcessingPlugin(QObject, Extension): global_container_stack.propertyChanged.emit("post_processing_plugin", "value") @staticmethod - def _isScriptAllowed(file_path) -> bool: + def _isScriptAllowed(file_path: str) -> bool: """Checks whether the given file is allowed to be loaded""" if not ApplicationMetadata.IsEnterpriseVersion: # No signature needed From dec68002bcebc06f79d4075412f1abc0624e66b1 Mon Sep 17 00:00:00 2001 From: Jaime van Kessel Date: Wed, 25 Mar 2020 12:35:15 +0100 Subject: [PATCH 6/9] Use patch.object to temporarily replace mock a function call CURA-7319 --- .../tests/TestPostProcessingPlugin.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py index a5ef9e47c9..2c9a25fcac 100644 --- a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py @@ -18,30 +18,29 @@ sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) def test_community_user_script_allowed(): assert PostProcessingPlugin._isScriptAllowed("blaat.py") + # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) def test_community_bundled_script_allowed(): assert PostProcessingPlugin._isScriptAllowed(_bundled_file_path()) + # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) def test_enterprise_unsigned_user_script_not_allowed(): assert not PostProcessingPlugin._isScriptAllowed("blaat.py") + # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) def test_enterprise_signed_user_script_allowed(): mocked_trust = MagicMock() mocked_trust.signedFileCheck = MagicMock(return_value=True) - realSignatureFileExistsFor = Trust.signatureFileExistsFor - Trust.signatureFileExistsFor = MagicMock(return_value=True) + with patch.object(Trust, "signatureFileExistsFor", return_value = True): + with patch("UM.Trust.Trust.getInstanceOrNone", return_value=mocked_trust): + assert PostProcessingPlugin._isScriptAllowed("blaat.py") - with patch("UM.Trust.Trust.getInstanceOrNone", return_value=mocked_trust): - assert PostProcessingPlugin._isScriptAllowed("blaat.py") - - # cleanup - Trust.signatureFileExistsFor = realSignatureFileExistsFor # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) From 92f278acc87b11c117e3172e76cfc8c7a78fa56b Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Wed, 25 Mar 2020 16:51:21 +0100 Subject: [PATCH 7/9] Change trusted scripts path from resources to plugin/scripts CURA-7319 --- .../PostProcessingPlugin/PostProcessingPlugin.py | 4 +++- .../tests/TestPostProcessingPlugin.py | 15 +++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index de9b1905b7..f206909f20 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -348,7 +348,9 @@ class PostProcessingPlugin(QObject, Extension): # No signature needed return True - if os.path.split(file_path) == os.path.join(Resources.getStoragePath(Resources.Resources), "scripts"): + if os.path.split(file_path)[0] == os.path.join( + PluginRegistry.getInstance().getPluginPath("PostProcessingPlugin"), + "scripts"): # Bundled scripts are trusted. return True diff --git a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py index 2c9a25fcac..360ea344ca 100644 --- a/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/tests/TestPostProcessingPlugin.py @@ -3,6 +3,7 @@ import os import sys from unittest.mock import patch, MagicMock +from UM.PluginRegistry import PluginRegistry from UM.Resources import Resources from UM.Trust import Trust from ..PostProcessingPlugin import PostProcessingPlugin @@ -12,6 +13,9 @@ sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) """ In this file, commnunity refers to regular Cura for makers.""" +mock_plugin_registry = MagicMock() +mock_plugin_registry.getPluginPath = MagicMock(return_value = "mocked_plugin_path") + # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", False) @@ -27,19 +31,22 @@ def test_community_bundled_script_allowed(): # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) -def test_enterprise_unsigned_user_script_not_allowed(): +@patch.object(PluginRegistry, "getInstance", return_value=mock_plugin_registry) +def test_enterprise_unsigned_user_script_not_allowed(plugin_registry): assert not PostProcessingPlugin._isScriptAllowed("blaat.py") - # noinspection PyProtectedMember @patch("cura.ApplicationMetadata.IsEnterpriseVersion", True) -def test_enterprise_signed_user_script_allowed(): +@patch.object(PluginRegistry, "getInstance", return_value=mock_plugin_registry) +def test_enterprise_signed_user_script_allowed(plugin_registry): mocked_trust = MagicMock() mocked_trust.signedFileCheck = MagicMock(return_value=True) + plugin_registry.getPluginPath = MagicMock(return_value="mocked_plugin_path") + with patch.object(Trust, "signatureFileExistsFor", return_value = True): with patch("UM.Trust.Trust.getInstanceOrNone", return_value=mocked_trust): - assert PostProcessingPlugin._isScriptAllowed("blaat.py") + assert PostProcessingPlugin._isScriptAllowed("mocked_plugin_path/scripts/blaat.py") # noinspection PyProtectedMember From 3bb3b69082f85848fea6697577f60e38a77d7802 Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Wed, 25 Mar 2020 17:32:08 +0100 Subject: [PATCH 8/9] Attempt to appease mypy --- plugins/PostProcessingPlugin/PostProcessingPlugin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index f206909f20..3f3ac9f6af 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -348,7 +348,8 @@ class PostProcessingPlugin(QObject, Extension): # No signature needed return True - if os.path.split(file_path)[0] == os.path.join( + dir_path = os.path.split(file_path)[0] + if dir_path == os.path.join( PluginRegistry.getInstance().getPluginPath("PostProcessingPlugin"), "scripts"): # Bundled scripts are trusted. From ddd7d15287ad915d58fea05fc9059ddb6e3c170d Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Wed, 25 Mar 2020 17:45:42 +0100 Subject: [PATCH 9/9] Attempt to appease mypy --- plugins/PostProcessingPlugin/PostProcessingPlugin.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/plugins/PostProcessingPlugin/PostProcessingPlugin.py b/plugins/PostProcessingPlugin/PostProcessingPlugin.py index 3f3ac9f6af..f4f0e23378 100644 --- a/plugins/PostProcessingPlugin/PostProcessingPlugin.py +++ b/plugins/PostProcessingPlugin/PostProcessingPlugin.py @@ -348,10 +348,11 @@ class PostProcessingPlugin(QObject, Extension): # No signature needed return True - dir_path = os.path.split(file_path)[0] - if dir_path == os.path.join( - PluginRegistry.getInstance().getPluginPath("PostProcessingPlugin"), - "scripts"): + dir_path = os.path.split(file_path)[0] # type: str + plugin_path = PluginRegistry.getInstance().getPluginPath("PostProcessingPlugin") + assert plugin_path is not None # appease mypy + bundled_path = os.path.join(plugin_path, "scripts") + if dir_path == bundled_path: # Bundled scripts are trusted. return True