diff --git a/src/unitxt/artifact.py b/src/unitxt/artifact.py index e1ccae320e..94044d4555 100644 --- a/src/unitxt/artifact.py +++ b/src/unitxt/artifact.py @@ -46,6 +46,72 @@ def verify_legal_catalog_name(name): ), f'Artifict name ("{name}") should be alphanumeric. Use "." for nesting (e.g. myfolder.my_artifact)' +class IllegalCatalogNameError(ValueError): + """Raised when an artifact reference is not a legal catalog name. + + Used by :func:`sanitize_catalog_identifier` so callers that accept an + artifact reference from an untrusted source can tell "this is not a catalog + name" apart from any other failure, and answer accordingly. + """ + + def __init__(self, name): + self.name = name + super().__init__( + f'Artifact reference ("{name}") is not a legal catalog name. A catalog ' + 'name is alphanumeric, uses "' + + constants.catalog_hierarchy_sep + + '" for nesting (e.g. metrics.accuracy), and may carry overrides in ' + "square brackets (e.g. metrics.f1_macro[average=macro]). File paths " + "are not accepted." + ) + + +def sanitize_catalog_identifier( + identifier: str, +) -> Tuple[str, Optional[Dict[str, Any]]]: + """Validates an artifact reference as a catalog name, returning its parts. + + Intended for references that arrive from an untrusted source (a request URL, + for example). :func:`fetch_artifact` deliberately accepts filesystem paths as + well as catalog names, which is right for library callers but not for a value + an outside caller controls -- there, a path would let that caller name any + file on the host (CWE-22). + + Returning the validated name rather than the original string is what makes + this safe to rely on: the caller passes on the value that was actually + checked, so no unvalidated substring can reach a loader, and the + sanitization is visible to static analysis rather than only to a reader. + The bracketed overrides are returned already parsed, to be applied as + keyword overrides instead of being re-serialized into the name. + + :param identifier: the artifact reference to validate. + :return: a tuple ``(name, overrides)``, where ``name`` is the validated + catalog name and ``overrides`` is the parsed contents of the square + brackets, or ``None`` if there were none. + :raises IllegalCatalogNameError: if ``identifier`` is not a legal catalog name. + """ + if not isinstance(identifier, str): + raise IllegalCatalogNameError(identifier) + + try: + name, args = separate_inside_and_outside_square_brackets(identifier) + except ValueError as e: + # Malformed brackets. Reported as an illegal name so the caller can + # answer with a client error instead of an unhandled failure. + raise IllegalCatalogNameError(identifier) from e + + if not is_name_legal_for_catalog(name): + raise IllegalCatalogNameError(identifier) + + # Rebuilt from the validated segments, so the returned name is derived from + # what the check above accepted and shares nothing with the raw input. + segments = name.split(constants.catalog_hierarchy_sep) + if not all(is_name_legal_for_catalog(segment) for segment in segments): + raise IllegalCatalogNameError(identifier) + + return constants.catalog_hierarchy_sep.join(segments), args + + def dict_diff_string(dict1, dict2, max_diff=200): keys_in_both = dict1.keys() & dict2.keys() added = {k: dict2[k] for k in dict2.keys() - dict1.keys()} diff --git a/src/unitxt/operators.py b/src/unitxt/operators.py index 410b940df1..43385f5eb0 100644 --- a/src/unitxt/operators.py +++ b/src/unitxt/operators.py @@ -91,6 +91,7 @@ LRUCache, deep_copy, flatten_dict, + json_dump, recursive_copy, recursive_shallow_copy, shallow_copy, @@ -1187,11 +1188,31 @@ class ArtifactFetcherMixin: _artifacts_cache = LRUCache(max_size=1000) @classmethod - def get_artifact(cls, artifact_identifier: str) -> Artifact: - if str(artifact_identifier) not in cls._artifacts_cache: - artifact, catalog = fetch_artifact(artifact_identifier) - cls._artifacts_cache[str(artifact_identifier)] = artifact - return shallow_copy(cls._artifacts_cache[str(artifact_identifier)]) + def get_artifact( + cls, + artifact_identifier: str, + overwrite_kwargs: Optional[Dict[str, Any]] = None, + ) -> Artifact: + """Fetches an artifact, caching it by identifier. + + :param artifact_identifier: an Artifact, a catalog name, or a path -- + whatever :func:`fetch_artifact` accepts. + :param overwrite_kwargs: arguments to override on the fetched artifact. + Lets a caller that has already parsed ``name[key=value]`` overrides + pass them separately, instead of re-serializing them into the + identifier. + """ + cache_key = str(artifact_identifier) + if overwrite_kwargs: + # Keep overridden artifacts from colliding with the plain one, or + # with each other, in the cache. + cache_key += f"[{json_dump(overwrite_kwargs)}]" + if cache_key not in cls._artifacts_cache: + artifact, _catalog = fetch_artifact( + artifact_identifier, overwrite_kwargs=overwrite_kwargs + ) + cls._artifacts_cache[cache_key] = artifact + return shallow_copy(cls._artifacts_cache[cache_key]) class ApplyOperatorsField(InstanceOperator): diff --git a/src/unitxt/service/metrics/main.py b/src/unitxt/service/metrics/main.py index ec29b55228..32568177ba 100644 --- a/src/unitxt/service/metrics/main.py +++ b/src/unitxt/service/metrics/main.py @@ -13,8 +13,8 @@ from ...artifact import ( Artifact, - is_name_legal_for_catalog, - separate_inside_and_outside_square_brackets, + IllegalCatalogNameError, + sanitize_catalog_identifier, ) from ...operator import MultiStreamOperator from ...operators import ArtifactFetcherMixin @@ -71,21 +71,27 @@ def compute( t0 = time.perf_counter() # Security: metric_name arrives from the request URL, and the artifact loader # it is passed to also accepts filesystem paths -- so without this check a - # caller could name any file on the server and have it opened, with its - # contents surfacing in the error message (CWE-22, path injection). - # Only catalog names are a legitimate value here, so require one: the - # validator permits [\w.] plus optional [...] overrides, and therefore no + # caller could name any file on the server and have it opened (CWE-22, path + # injection). Only catalog names are a legitimate value here, so require one: + # the validator permits [\w.] plus optional [...] overrides, and therefore no # path separators, "..", drive letters or absolute paths. - requested_name, _ = separate_inside_and_outside_square_brackets(metric_name) - if not is_name_legal_for_catalog(requested_name): + # + # Everything below uses safe_metric_name, the name returned by the sanitizer, + # and never metric_name itself. Passing on the value that was actually + # validated is what makes the check load-bearing: a check on one variable + # does not make a different variable safe. The raw value is kept out of the + # log lines too, so it cannot be used to forge log entries. + try: + safe_metric_name, metric_overrides = sanitize_catalog_identifier(metric_name) + except IllegalCatalogNameError as e: raise HTTPException( status_code=400, detail="Invalid metric name. A metric must be referenced by its " "catalog name (alphanumeric, '.' for nesting), not by a file path.", - ) + ) from e try: logging.info(f"Request from [{token['sub']}]") - logging.info(f"Computing metric '{metric_name}'.") + logging.info(f"Computing metric '{safe_metric_name}'.") logging.info( f"MetricRequest contains {len(request['instance_inputs'])} input instances" ) @@ -97,7 +103,9 @@ def compute( logging.info("Acquired compute_lock, starting computation .. ") start_infer_time = datetime.datetime.now() # obtain the metric to compute - metric: Artifact = ArtifactFetcherMixin.get_artifact(metric_name) + metric: Artifact = ArtifactFetcherMixin.get_artifact( + safe_metric_name, overwrite_kwargs=metric_overrides + ) metric: MultiStreamOperator = cast(MultiStreamOperator, metric) # Confidence interval is currently always off for remote metrics. # metric. @@ -113,7 +121,7 @@ def compute( infer_time = datetime.datetime.now() - start_infer_time wait_time = start_infer_time - start_time logging.info( - f"Computed {len(metric_results)} metric '{metric_name}' results, " + f"Computed {len(metric_results)} metric '{safe_metric_name}' results, " f"took: {infer_time!s}, waited: {wait_time!s}')" ) @@ -126,7 +134,7 @@ def compute( finally: t1 = time.perf_counter() logging.info( - f"Request for metric '{metric_name}' handled in [{t1 - t0:.2f}] secs." + f"Request for metric '{safe_metric_name}' handled in [{t1 - t0:.2f}] secs." ) diff --git a/tests/library/test_path_injection_service.py b/tests/library/test_path_injection_service.py index 7fca994361..4e1a9727f0 100644 --- a/tests/library/test_path_injection_service.py +++ b/tests/library/test_path_injection_service.py @@ -1,16 +1,23 @@ -"""Regression tests for CodeQL py/path-injection alerts cs-12 .. cs-16 (#1991). +"""Regression tests for CodeQL py/path-injection alerts cs-12..cs-18 (#1991, #1994). -CodeQL traced one taint flow for all five alerts: the ``metric_name`` URL path -parameter of the metrics service (``src/unitxt/service/metrics/main.py``) reaching -``open()`` in ``Artifact.is_artifact_file`` and ``utils.load_json``, because the -artifact loader accepts filesystem paths as well as catalog names. +CodeQL traced one taint flow for every alert: the ``metric_name`` URL path +parameter of the metrics service (``src/unitxt/service/metrics/main.py``) +reaching ``open()`` in ``Artifact.is_artifact_file`` and ``utils.load_json``, +because the artifact loader accepts filesystem paths as well as catalog names. -The remediation has two halves, tested here: +A first fix (#1993) added a validity check to the endpoint, but kept passing the +original ``metric_name`` to the loader. The check was effective at runtime, yet +the value that was validated and the value that was used were different +variables, so the alerts stayed open -- rightly, since that shape only stays +correct as long as nobody edits the two lines apart. -1. The service rejects anything that is not a catalog name, so the untrusted - value never reaches the loader. -2. ``load_json`` no longer echoes file contents into its error, so a path that - does reach it cannot be used to read a file (CWE-209). +The remediation tested here: + +1. ``sanitize_catalog_identifier`` validates a reference and returns the name it + validated, so callers pass on the checked value rather than the raw one. +2. The service uses only that returned name, and answers 400 otherwise. +3. ``load_json`` does not echo file contents into its error, so a path that does + reach it cannot be used to read a file (CWE-209). The library's own support for loading an artifact from a file path is a documented feature and is deliberately left working. @@ -23,42 +30,52 @@ from unitxt.artifact import ( Artifact, + IllegalCatalogNameError, fetch_artifact, - is_name_legal_for_catalog, - separate_inside_and_outside_square_brackets, + sanitize_catalog_identifier, ) +from unitxt.operators import ArtifactFetcherMixin from unitxt.utils import load_json - -def _service_guard_accepts(metric_name): - """Mirrors the check performed by the service's ``compute`` endpoint. - - The endpoint itself needs fastapi, which is an optional dependency, so the - guard logic is exercised directly here. - """ - requested_name, _ = separate_inside_and_outside_square_brackets(metric_name) - return bool(is_name_legal_for_catalog(requested_name)) - - -class TestServiceMetricNameGuard(unittest.TestCase): - def test_rejects_absolute_paths(self): - for name in ("/etc/passwd", "/tmp/secret.txt", "/var/run/secrets/token"): - with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) - - def test_rejects_traversal(self): - for name in ( - "../../etc/passwd", - "../catalog/thing.json", - "metrics.accuracy/../../etc/passwd", - ): +# Shapes that must never be accepted from an untrusted caller. Each would +# otherwise be opened as a filesystem path by the artifact loader. +UNSAFE_REFERENCES = [ + "/etc/passwd", + "/tmp/secret.txt", + "/var/run/secrets/token", + "../../etc/passwd", + "../catalog/thing.json", + "metrics.accuracy/../../etc/passwd", + r"C:\windows\win.ini", + r"..\..\secret.txt", + "~/.ssh/id_rsa", + "./relative.json", + "/", + "", + "metrics\x00.accuracy", + "http://evil.example/x.json", +] + + +class TestSanitizeCatalogIdentifierRejectsPaths(unittest.TestCase): + def test_rejects_unsafe_references(self): + for name in UNSAFE_REFERENCES: with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(name) - def test_rejects_windows_style_paths(self): - for name in (r"C:\windows\win.ini", r"..\..\secret.txt"): + def test_rejects_malformed_brackets(self): + """Must be a clean rejection, not an unhandled ValueError from the parser.""" + for name in ("metrics.accuracy[unclosed", "a]b[c", "metrics.x]"): with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(name) + + def test_rejects_non_strings(self): + for value in (None, 123, ["metrics.accuracy"], {"a": 1}): + with self.subTest(value=value): + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(value) def test_rejects_a_real_artifact_file_path(self): """Even a genuine artifact file must not be loadable by path remotely.""" @@ -71,27 +88,58 @@ def test_rejects_a_real_artifact_file_path(self): Artifact.is_artifact_file(path), "sanity: the file is a loadable artifact, so only the guard stops it", ) - self.assertFalse(_service_guard_accepts(path)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(path) + + def test_error_names_the_offending_value(self): + with self.assertRaises(IllegalCatalogNameError) as ctx: + sanitize_catalog_identifier("/etc/passwd") + self.assertEqual(ctx.exception.name, "/etc/passwd") + + def test_is_a_value_error(self): + """Callers catching ValueError must keep working.""" + self.assertTrue(issubclass(IllegalCatalogNameError, ValueError)) + +class TestSanitizeCatalogIdentifierAcceptsNames(unittest.TestCase): def test_accepts_plain_catalog_names(self): for name in ("metrics.accuracy", "accuracy", "metrics.rouge"): with self.subTest(name=name): - self.assertTrue(_service_guard_accepts(name)) + self.assertEqual(sanitize_catalog_identifier(name), (name, None)) def test_accepts_catalog_names_with_overrides(self): - """Bracketed overrides must keep working; only the name part is checked.""" - self.assertTrue(_service_guard_accepts("metrics.f1_macro[average=macro]")) + """Bracketed overrides must keep working; only the name part is a path risk.""" + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + self.assertEqual(name, "metrics.f1_macro") + self.assertEqual(overrides, {"ignore_case": False}) + def test_returns_the_validated_name_not_the_input(self): + """The point of the fix: what comes back is derived from what was checked. -class TestServiceEndpointAppliesTheGuard(unittest.TestCase): - """The guard above is only meaningful if ``compute`` actually performs it. + If this ever returns the caller's string unchanged, the returned value is + no safer than the input and the sanitizer is decorative. + """ + returned, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + self.assertNotIn("[", returned) + self.assertEqual(returned, "metrics.f1_macro") + self.assertEqual(overrides, {"ignore_case": False}) + + +class TestSanitizedNameIsWhatTheServiceUses(unittest.TestCase): + """The sanitizer only helps if ``compute`` passes on its result. The endpoint needs fastapi (an optional dependency), so rather than calling - it, assert that its source wires the validator in before the artifact - loader. This fails if the check is removed or moved after get_artifact. + it, assert its source uses the sanitized name and never the raw parameter. + This fails if the two are edited apart again, which is what kept the alerts + open after the first fix. """ - def _compute_source(self): + @staticmethod + def _compute_source(): service_main = os.path.join( os.path.dirname( os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -107,18 +155,70 @@ def _compute_source(self): start = source.index("def compute(") return source[start : source.index("\n@app.", start)] - def test_compute_validates_the_name_before_loading_the_artifact(self): + def test_compute_sanitizes_before_loading_the_artifact(self): body = self._compute_source() - self.assertIn("is_name_legal_for_catalog", body) - self.assertIn("HTTPException", body) + self.assertIn("sanitize_catalog_identifier", body) self.assertLess( - body.index("is_name_legal_for_catalog"), + body.index("sanitize_catalog_identifier"), body.index("get_artifact"), - "the name must be validated before it reaches the artifact loader", + "the name must be sanitized before it reaches the artifact loader", ) def test_compute_rejects_with_a_client_error(self): - self.assertIn("status_code=400", self._compute_source()) + body = self._compute_source() + self.assertIn("IllegalCatalogNameError", body) + self.assertIn("status_code=400", body) + + def test_compute_never_passes_the_raw_parameter_onward(self): + """``metric_name`` must be consumed by the sanitizer and nothing else.""" + body = self._compute_source() + uses = [ + line.strip() + for line in body.splitlines() + if "metric_name" in line + and "safe_metric_name" not in line + and not line.strip().startswith("#") + ] + # Only the signature and the sanitizer call may mention the raw value. + for line in uses: + with self.subTest(line=line): + self.assertTrue( + "metric_name: str" in line + or "sanitize_catalog_identifier(metric_name)" in line, + f"raw metric_name must not be used here: {line}", + ) + + +class TestGetArtifactOverrides(unittest.TestCase): + """Overrides now travel beside the name, so they must still apply.""" + + def test_overrides_are_applied(self): + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + artifact = ArtifactFetcherMixin.get_artifact(name, overwrite_kwargs=overrides) + self.assertFalse(artifact.ignore_case) + + def test_overrides_do_not_poison_the_cache_for_the_plain_artifact(self): + plain_before = ArtifactFetcherMixin.get_artifact("metrics.f1_macro") + self.assertTrue(plain_before.ignore_case, "sanity: default is True") + + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + overridden = ArtifactFetcherMixin.get_artifact(name, overwrite_kwargs=overrides) + self.assertFalse(overridden.ignore_case) + + plain_after = ArtifactFetcherMixin.get_artifact("metrics.f1_macro") + self.assertTrue( + plain_after.ignore_case, + "an overridden fetch must not be served from, or written to, " + "the cache entry of the plain artifact", + ) + + def test_without_overrides_behaves_as_before(self): + artifact = ArtifactFetcherMixin.get_artifact("metrics.accuracy") + self.assertIsNotNone(artifact) class TestLoadJsonDoesNotLeakFileContents(unittest.TestCase):