diff --git a/datalab/adapters_metadata/base_adapter.py b/datalab/adapters_metadata/base_adapter.py index 9b1f86802..e36504694 100644 --- a/datalab/adapters_metadata/base_adapter.py +++ b/datalab/adapters_metadata/base_adapter.py @@ -8,12 +8,16 @@ from __future__ import annotations +import warnings from abc import ABC, abstractmethod from typing import TYPE_CHECKING, Any, ClassVar, Generator import guidata.dataset as gds from sigima.objects import ImageObj, SignalObj +from datalab.config import _ +from datalab.utils.datasetjson import decode_dataset_json + if TYPE_CHECKING: import pandas as pd from sigima.objects.scalar import GeometryResult, TableResult @@ -175,11 +179,15 @@ def get_param(self) -> gds.DataSet | None: """Get parameter dataset associated with this result. Returns: - Parameter dataset if present, None otherwise + Parameter dataset if present and readable (a warning is emitted for + stored parameters that cannot be decoded), None otherwise """ param_json = self.result.attrs.get("param_json") if param_json is not None: - return gds.json_to_dataset(param_json) + try: + return decode_dataset_json(param_json) + except ValueError: + warnings.warn(_("Failed to deserialize analysis parameters from JSON.")) return None def remove_from(self, obj: SignalObj | ImageObj) -> None: diff --git a/datalab/gui/creation.py b/datalab/gui/creation.py index 6e8124972..4746beedb 100644 --- a/datalab/gui/creation.py +++ b/datalab/gui/creation.py @@ -33,6 +33,7 @@ ) from datalab.config import _ +from datalab.utils.datasetjson import decode_dataset_json if TYPE_CHECKING: from qtpy import QtWidgets as QW @@ -117,7 +118,7 @@ def extract_creation_parameters(obj: SignalObj | ImageObj) -> gds.DataSet | None raise ValueError( "Peak parameterization set on non-peak creation parameters" ) - return gds.json_to_dataset(dataset_json) + return decode_dataset_json(dataset_json) dataset_json = options[LEGACY_CREATION_PARAMETERS_OPTION] if not isinstance(dataset_json, str): @@ -125,7 +126,7 @@ def extract_creation_parameters(obj: SignalObj | ImageObj) -> gds.DataSet | None raw_params = _decode_dataset_json(dataset_json) if raw_params.get("class_name") in {"GaussParam", "LorentzParam", "VoigtParam"}: validate_peak_creation_params(raw_params) - return gds.json_to_dataset(dataset_json) + return decode_dataset_json(dataset_json) def convert_legacy_creation_parameters( @@ -150,7 +151,7 @@ def convert_legacy_creation_parameters( raise ValueError("Object has no legacy creation parameters") raw_params = _decode_dataset_json(dataset_json) converted = convert_legacy_peak_creation_params(raw_params) - param = gds.json_to_dataset(json.dumps(converted)) + param = decode_dataset_json(json.dumps(converted)) insert_creation_parameters(obj, param) return param diff --git a/datalab/gui/processor/base.py b/datalab/gui/processor/base.py index 1767dddda..e87b34955 100644 --- a/datalab/gui/processor/base.py +++ b/datalab/gui/processor/base.py @@ -65,6 +65,7 @@ from datalab.gui.processor.execution import CANCELLED, ExecutionService from datalab.history.effects import capture_effects from datalab.objectmodel import get_title_reference, get_uuid, patch_title_with_ids +from datalab.utils.datasetjson import decode_dataset_json from datalab.utils.qthelpers import create_progress_bar, qt_try_except if TYPE_CHECKING: @@ -106,10 +107,10 @@ def set_param_from_json(self, param_json: str | list[str]) -> None: try: if isinstance(param_json, list): # Handle list of JSON strings - self.param = [gds.json_to_dataset(p) for p in param_json] + self.param = [decode_dataset_json(p) for p in param_json] else: # Handle single JSON string - self.param = gds.json_to_dataset(param_json) + self.param = decode_dataset_json(param_json) except Exception: # pylint: disable=broad-except warnings.warn(_("Failed to deserialize processing parameters from JSON.")) self.param = None diff --git a/datalab/history/core.py b/datalab/history/core.py index 2a60badb3..f53f3c0a1 100644 --- a/datalab/history/core.py +++ b/datalab/history/core.py @@ -13,12 +13,13 @@ from typing import Any import numpy as np -from guidata.dataset.conv import dataset_to_json, json_to_dataset +from guidata.dataset.conv import dataset_to_json from guidata.dataset.datatypes import DataSet from qtpy import QtCore as QC from sigima.objects.base import BaseROI from datalab.config import _ +from datalab.utils.datasetjson import decode_dataset_json _TRUSTED_ROI_MODULE_PREFIX = "sigima." @@ -162,7 +163,7 @@ def decode_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]: for key, value in kwargs.items(): if isinstance(value, dict) and _DATASET_MARKER in value: try: - decoded[key] = json_to_dataset(value[_DATASET_MARKER]) + decoded[key] = decode_dataset_json(value[_DATASET_MARKER]) except (TypeError, ValueError, KeyError): warnings.warn( _("Failed to deserialize history DataSet kwarg %r.") % key @@ -183,7 +184,7 @@ def decode_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]: elif isinstance(value, dict) and _DATASET_LIST_MARKER in value: try: decoded[key] = [ - json_to_dataset(item) for item in value[_DATASET_LIST_MARKER] + decode_dataset_json(item) for item in value[_DATASET_LIST_MARKER] ] except (TypeError, ValueError, KeyError): warnings.warn( @@ -200,7 +201,7 @@ def copy_history_value(value: Any) -> Any: if callable(value): raise TypeError("History duplication does not support callable kwargs") if isinstance(value, DataSet): - return json_to_dataset(dataset_to_json(value)) + return decode_dataset_json(dataset_to_json(value)) if isinstance(value, BaseROI): return decode_roi(encode_roi(value)) if isinstance(value, dict): diff --git a/datalab/locale/fr/LC_MESSAGES/datalab.po b/datalab/locale/fr/LC_MESSAGES/datalab.po index 71ecc9e9d..84c572f49 100644 --- a/datalab/locale/fr/LC_MESSAGES/datalab.po +++ b/datalab/locale/fr/LC_MESSAGES/datalab.po @@ -10,6 +10,9 @@ msgstr "" "Content-Type: text/plain; charset=utf-8\n" "Content-Transfer-Encoding: 8bit\n" +msgid "Failed to deserialize analysis parameters from JSON." +msgstr "Échec de la désérialisation des paramètres d'analyse depuis le format JSON." + msgid "Results" msgstr "Résultats" diff --git a/datalab/tests/features/common/stored_parameters_unit_test.py b/datalab/tests/features/common/stored_parameters_unit_test.py new file mode 100644 index 000000000..1ec01c43e --- /dev/null +++ b/datalab/tests/features/common/stored_parameters_unit_test.py @@ -0,0 +1,261 @@ +# Copyright (c) DataLab Platform Developers, BSD 3-Clause license, see LICENSE file. + +""" +Parameter sets stored in workspaces never choose the code DataLab runs + +Workspaces store parameter sets as JSON naming their class. These tests check +that decoding them neither imports a module named by a file nor calls anything +but an already known parameter class (issue #407), and that legitimate +parameters, including plugin ones and those saved by earlier versions, are read +as before. +""" + +from __future__ import annotations + +import json +import re +import sys +import tempfile +import types +from pathlib import Path + +import guidata.dataset as gds +import numpy as np +import pytest +import sigima.objects as sio +from sigima.objects import GaussParam +from sigima.objects.scalar import GeometryResult, KindShape +from sigima.params import NormalizeParam + +import datalab +from datalab.adapters_metadata import GeometryAdapter +from datalab.gui.newobject import ( + CREATION_PARAMETERS_OPTION, + extract_creation_parameters, + insert_creation_parameters, +) +from datalab.gui.processor.base import ProcessingParameters +from datalab.history.core import copy_history_value, decode_kwargs +from datalab.utils.datasetjson import ParameterClassError, decode_dataset_json + +FAKE_MODULE = "dl_untrusted_test_module" +PLUGIN_MODULE = "dl_fake_plugin_module" + + +class PluginParam(gds.DataSet): + """Parameter set of a plugin module""" + + value = gds.FloatItem("Value", default=1.0) + + +def _crafted(module_name: str, class_name: str) -> str: + """Return normalization parameters whose class fields were edited.""" + payload = json.loads(gds.dataset_to_json(NormalizeParam())) + payload.update(class_module=module_name, class_name=class_name) + return json.dumps(payload) + + +@pytest.fixture(name="calls") +def fixture_calls(monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Register a loaded module whose callables record their calls.""" + calls: list[str] = [] + + def boom() -> object: + calls.append("boom") + return object() + + class NotAParameterSet: # pylint: disable=too-few-public-methods + """Class with a side effect when instantiated""" + + def __init__(self) -> None: + calls.append("NotAParameterSet") + + module = types.ModuleType(FAKE_MODULE) + module.boom = boom + module.NotAParameterSet = NotAParameterSet + monkeypatch.setitem(sys.modules, FAKE_MODULE, module) + return calls + + +@pytest.fixture(name="plugin_module") +def fixture_plugin_module(monkeypatch: pytest.MonkeyPatch) -> types.ModuleType: + """Register a loaded third-party module defining a parameter set.""" + module = types.ModuleType(PLUGIN_MODULE) + module.PluginParam = PluginParam + monkeypatch.setitem(sys.modules, PLUGIN_MODULE, module) + monkeypatch.setattr(PluginParam, "__module__", PLUGIN_MODULE) + return module + + +@pytest.mark.parametrize( + "param", + [ + NormalizeParam.create(method="amplitude"), + GaussParam.create(amplitude=-2.5, sigma=0.7, mu=0.3, y0=0.75), + ], +) +def test_legitimate_parameters_are_decoded_as_by_guidata(param: gds.DataSet) -> None: + """Parameters of Sigima classes are decoded exactly as guidata decodes them.""" + text = gds.dataset_to_json(param) + decoded = decode_dataset_json(text) + assert type(decoded) is type(param) + assert gds.dataset_to_json(decoded) == gds.dataset_to_json( + gds.json_to_dataset(text) + ) + + +def test_fields_are_read_tolerantly_as_before() -> None: + """Unknown fields are ignored and missing ones keep their default values.""" + payload = json.loads(gds.dataset_to_json(NormalizeParam.create(method="area"))) + payload["field_of_a_future_version"] = 42 + decoded = decode_dataset_json(json.dumps(payload)) + assert decoded.method == NormalizeParam.create(method="area").method + del payload["method"] + decoded = decode_dataset_json(json.dumps(payload)) + assert decoded.method == NormalizeParam().method + + +def test_plugin_parameters_are_decoded(plugin_module: types.ModuleType) -> None: + """A parameter class of a loaded plugin module stays usable.""" + param = PluginParam() + param.value = 3.5 + text = gds.dataset_to_json(param) + assert json.loads(text)["class_module"] == plugin_module.__name__ + decoded = decode_dataset_json(text) + assert isinstance(decoded, PluginParam) + assert decoded.value == 3.5 + + +@pytest.mark.parametrize("name", ["boom", "NotAParameterSet"]) +def test_loaded_callables_are_never_called(calls: list[str], name: str) -> None: + """A loaded function or class that is not a parameter set is refused.""" + with pytest.raises(ParameterClassError, match="not a parameter set class"): + decode_dataset_json(_crafted(FAKE_MODULE, name)) + assert not calls + + +def test_issue_reproduction_creates_nothing(monkeypatch: pytest.MonkeyPatch) -> None: + """The reproduction of issue #407 no longer calls ``tempfile.mkdtemp``.""" + calls: list[str] = [] + monkeypatch.setattr(tempfile, "mkdtemp", lambda: calls.append("mkdtemp")) + with pytest.raises(ParameterClassError): + decode_dataset_json(_crafted("tempfile", "mkdtemp")) + assert not calls + + +def test_untrusted_modules_are_never_imported(monkeypatch: pytest.MonkeyPatch) -> None: + """A module that DataLab has not loaded is not imported for a file.""" + monkeypatch.delitem(sys.modules, "this", raising=False) + with pytest.raises(ParameterClassError, match="which DataLab does not use"): + decode_dataset_json(_crafted("this", "d")) + assert "this" not in sys.modules + + +@pytest.mark.parametrize( + ("module_name", "message"), + [ + ("datalab_lookalike_module.params", "which DataLab does not use"), + ("datalab.__main__", "which DataLab does not use"), + ("datalab.tests.not_a_module", "which DataLab does not use"), + ("sigima._not_a_module", "which DataLab does not use"), + ("sigima.not_a_module_for_tests", "missing module"), + ], +) +def test_only_public_modules_of_datalab_packages_may_be_imported( + module_name: str, message: str +) -> None: + """Only public modules of DataLab's own packages are looked for on import.""" + with pytest.raises(ParameterClassError, match=message): + decode_dataset_json(_crafted(module_name, "NormalizeParam")) + + +@pytest.mark.parametrize( + "text", + [ + json.dumps({"class_name": "NormalizeParam"}), + json.dumps({"class_module": 1, "class_name": "NormalizeParam"}), + json.dumps({"class_module": "sigima.params", "class_name": "os.path"}), + json.dumps({"class_module": "sigima.params", "class_name": "__class__"}), + json.dumps(["sigima.params", "NormalizeParam"]), + ], +) +def test_malformed_class_fields_are_refused(text: str) -> None: + """Missing, non-text or non-class names are refused.""" + with pytest.raises(ParameterClassError): + decode_dataset_json(text) + + +def test_processing_parameters_refuse_crafted_classes(calls: list[str]) -> None: + """Processing parameters stored with an object (Processing tab, History).""" + stored = ProcessingParameters(func_name="normalize", pattern="1-to-1") + with pytest.warns(UserWarning): + stored.set_param_from_json(_crafted(FAKE_MODULE, "boom")) + assert stored.param is None + assert not calls + stored.set_param_from_json(gds.dataset_to_json(NormalizeParam())) + assert isinstance(stored.param, NormalizeParam) + + +def test_analysis_parameters_refuse_crafted_classes(calls: list[str]) -> None: + """Parameters of analysis results shown in the Properties panel.""" + obj = sio.create_signal("signal", np.arange(4.0), np.arange(4.0)) + result = GeometryResult( + title="fwhm", + func_name="fwhm", + kind=KindShape.SEGMENT, + coords=np.array([[0.0, 0.0, 1.0, 0.0]]), + roi_indices=None, + attrs={"param_json": _crafted(FAKE_MODULE, "boom")}, + ) + GeometryAdapter(result).add_to(obj) + (adapter,) = GeometryAdapter.iterate_from_obj(obj) + with pytest.warns(UserWarning): + assert adapter.get_param() is None + assert not calls + + +def test_creation_parameters_refuse_crafted_classes(calls: list[str]) -> None: + """Creation parameters shown in the Creation tab.""" + obj = sio.create_signal("signal", np.arange(4.0), np.arange(4.0)) + insert_creation_parameters(obj, sio.SineParam()) + envelope = obj.get_metadata_options()[CREATION_PARAMETERS_OPTION] + envelope["dataset_json"] = _crafted(FAKE_MODULE, "boom") + obj.set_metadata_option(CREATION_PARAMETERS_OPTION, envelope) + with pytest.raises(ValueError): + extract_creation_parameters(obj) + assert not calls + + +def test_history_arguments_refuse_crafted_classes(calls: list[str]) -> None: + """Parameters of History actions read from a workspace or a .dlhist file.""" + crafted = _crafted(FAKE_MODULE, "boom") + with pytest.warns(UserWarning): + decoded = decode_kwargs( + { + "param": {"__dataset_json__": crafted}, + "params": {"__dataset_list_json__": [crafted]}, + } + ) + assert decoded == {"param": None, "params": []} + assert not calls + + +def test_history_copies_keep_their_parameters() -> None: + """Duplicating a History action still copies its parameter sets.""" + param = NormalizeParam.create(method="area") + copied = copy_history_value(param) + assert copied is not param + assert gds.dataset_to_json(copied) == gds.dataset_to_json(param) + + +def test_no_permissive_decoding_left_in_datalab() -> None: + """DataLab decodes stored parameter sets only through the safe decoder.""" + package = Path(datalab.__file__).parent + pattern = re.compile(r"\bjson_to_dataset\b") + offenders = [ + str(path.relative_to(package)) + for path in package.rglob("*.py") + if "tests" not in path.relative_to(package).parts + and pattern.search(path.read_text(encoding="utf-8")) + ] + assert not offenders diff --git a/datalab/utils/datasetjson.py b/datalab/utils/datasetjson.py new file mode 100644 index 000000000..e3a535533 --- /dev/null +++ b/datalab/utils/datasetjson.py @@ -0,0 +1,109 @@ +# Copyright (c) DataLab Platform Developers, BSD 3-Clause license, see LICENSE file. + +""" +Safe decoding of parameter sets stored in workspaces + +DataLab stores guidata parameter sets in workspaces as JSON documents naming +their class (``class_module`` and ``class_name``). guidata's permissive decoder +imports the named module and calls the named object, so a workspace received +from someone else could choose code to run. :func:`decode_dataset_json` only +builds :class:`guidata.dataset.DataSet` subclasses that DataLab already knows. +""" + +from __future__ import annotations + +import importlib +import json +import sys + +import guidata.dataset as gds +from guidata.io.jsonfmt import JSONReader + +#: Packages whose modules may be imported to find a stored parameter class +TRUSTED_PACKAGES = ("datalab", "guidata", "sigima", "sigimax") + + +class ParameterClassError(ValueError): + """Stored parameters name a class that may not be used""" + + +def _may_import(module_name: str) -> bool: + """Return True if *module_name* may be imported to find a parameter class. + + Only public modules of DataLab's own packages qualify: private modules + (e.g. ``__main__``) and test modules are never imported for a file. + """ + parts = module_name.split(".") + return ( + parts[0] in TRUSTED_PACKAGES + and all(part.isidentifier() and not part.startswith("_") for part in parts) + and "tests" not in parts + ) + + +def resolve_dataset_class(module_name: object, class_name: object) -> type: + """Return the parameter class named by stored parameters. + + The class must be a :class:`guidata.dataset.DataSet` subclass, defined in a + module already loaded (DataLab, Sigima, guidata, installed plugins) or in a + public module of DataLab's own packages, which is then imported. Nothing else + is imported or called. + + Args: + module_name: Value of the ``class_module`` field + class_name: Value of the ``class_name`` field + + Returns: + The parameter class + + Raises: + ParameterClassError: If the class may not be used + """ + if not isinstance(module_name, str) or not isinstance(class_name, str): + raise ParameterClassError("Stored parameters do not name their class") + module = sys.modules.get(module_name) + if module is None: + if not _may_import(module_name): + raise ParameterClassError( + f"Stored parameters name module {module_name!r}, " + "which DataLab does not use" + ) + try: + module = importlib.import_module(module_name) + except ImportError as exc: + raise ParameterClassError( + f"Stored parameters name missing module {module_name!r}" + ) from exc + cls = getattr(module, class_name, None) if class_name.isidentifier() else None + if not (isinstance(cls, type) and issubclass(cls, gds.DataSet)): + raise ParameterClassError( + f"{module_name}.{class_name} is not a parameter set class" + ) + return cls + + +def decode_dataset_json(json_str: str) -> gds.DataSet: + """Decode parameters serialized by :func:`guidata.dataset.dataset_to_json`. + + The class is resolved by :func:`resolve_dataset_class`. Fields are then read + as guidata does: unknown fields are ignored and missing fields keep their + default values, so parameters saved by earlier versions stay readable. + + Args: + json_str: JSON document stored with an object, a result or a History + action + + Returns: + The decoded parameter set + + Raises: + ParameterClassError: If the stored class may not be used + ValueError: If *json_str* is not valid JSON + """ + payload = json.loads(json_str) + if not isinstance(payload, dict): + raise ParameterClassError("Stored parameters are not a JSON object") + cls = resolve_dataset_class(payload.get("class_module"), payload.get("class_name")) + param = cls() + param.deserialize(JSONReader(json_str)) + return param diff --git a/doc/locale/fr/LC_MESSAGES/release_notes/release_1.04.po b/doc/locale/fr/LC_MESSAGES/release_notes/release_1.04.po index d4550f362..e3c24c179 100644 --- a/doc/locale/fr/LC_MESSAGES/release_notes/release_1.04.po +++ b/doc/locale/fr/LC_MESSAGES/release_notes/release_1.04.po @@ -371,3 +371,9 @@ msgstr "**Création de signaux :**" msgid "The blackbody signal (**Create > Blackbody (Planck's law)**) now has axis titles and units (wavelength in m, spectral radiance in W·sr⁻¹·m⁻³), which were missing (fixes [Issue #400](https://github.com/DataLab-Platform/DataLab/issues/400))" msgstr "Le signal de corps noir (**Création > Corps noir (loi de Planck)**) a désormais des titres et des unités d'axes (longueur d'onde en m, luminance énergétique spectrale en W·sr⁻¹·m⁻³), qui étaient absents (corrige [l'issue #400](https://github.com/DataLab-Platform/DataLab/issues/400))" + +msgid "**Security:**" +msgstr "**Sécurité :**" + +msgid "Opening a workspace received from someone else can no longer make DataLab load or run other code installed on the computer. The parameters stored with objects, analysis results and History actions name their class, and DataLab imported whatever module they named and called the object named there, which a modified file could use, for example, to make DataLab quit. Only parameter classes of DataLab, Sigima, guidata and installed plugins are now used; other stored parameters are reported as unreadable (fixes [Issue #407](https://github.com/DataLab-Platform/DataLab/issues/407))" +msgstr "Ouvrir un espace de travail reçu d'une autre personne ne permet plus de faire charger ou exécuter par DataLab un autre code installé sur l'ordinateur. Les paramètres enregistrés avec les objets, les résultats d'analyse et les actions de l'historique nomment leur classe, et DataLab importait le module ainsi nommé puis appelait l'objet désigné, ce qu'un fichier modifié pouvait exploiter, par exemple pour faire quitter DataLab. Seules les classes de paramètres de DataLab, Sigima, guidata et des plugins installés sont désormais utilisées ; les autres paramètres enregistrés sont signalés comme illisibles (corrige [l'issue #407](https://github.com/DataLab-Platform/DataLab/issues/407))" diff --git a/doc/release_notes/release_1.04.md b/doc/release_notes/release_1.04.md index 6c627077c..9b2f538ca 100644 --- a/doc/release_notes/release_1.04.md +++ b/doc/release_notes/release_1.04.md @@ -174,3 +174,7 @@ DataLab now uses stable UUID-based object references across object trees, result * Errors raised by remote commands, such as `calc` with an unknown computation function or `set_object` with an unknown object, are now reported to the client instead of being silently ignored * Fixed remote commands sometimes returning the result of the previous command instead of their own: for example, `get_current_object_uuid` could return a panel name instead of `None` when there was no current object * Fixed DataLab freezing when closed while a remote command was still running (e.g. when closing the main window with `call_method("close")`) + +**Security:** + +* Opening a workspace received from someone else can no longer make DataLab load or run other code installed on the computer. The parameters stored with objects, analysis results and History actions name their class, and DataLab imported whatever module they named and called the object named there, which a modified file could use, for example, to make DataLab quit. Only parameter classes of DataLab, Sigima, guidata and installed plugins are now used; other stored parameters are reported as unreadable (fixes [Issue #407](https://github.com/DataLab-Platform/DataLab/issues/407))