diff --git a/docs/changelog.d/877-model-comparison-control-callbacks.md b/docs/changelog.d/877-model-comparison-control-callbacks.md new file mode 100644 index 000000000..86dc92175 --- /dev/null +++ b/docs/changelog.d/877-model-comparison-control-callbacks.md @@ -0,0 +1,5 @@ +# Model-comparison callback-boundary hardening + +## Security + +- Harden parameter-count, audit-label, and real-valued model-comparison controls so caller-defined integer/string/NumPy subclasses and arbitrary integer-protocol providers are rejected before conversion or normalization callbacks execute, while preserving genuine NumPy scalar compatibility and Rust-owned Vuong arithmetic. diff --git a/docs/doctoring/model_comparison_control_validation.md b/docs/doctoring/model_comparison_control_validation.md index f5fa42a6d..6a3e77327 100644 --- a/docs/doctoring/model_comparison_control_validation.md +++ b/docs/doctoring/model_comparison_control_validation.md @@ -2,22 +2,26 @@ ## Decision -Public model-comparison semantic controls are validated as bounded package contracts before any caller-defined representation or numeric conversion hook can execute. `relation` accepts only `ModelRelation` or a built-in `str`; `alpha` and `omega_tol` accept the intended built-in/NumPy real-scalar domain and reject arbitrary coercible objects. Stable package-owned validation messages remain non-reflective. +Public model-comparison semantic controls are validated as bounded package contracts before any caller-defined representation or numeric conversion hook can execute. `relation` accepts only `ModelRelation` or a built-in `str`; `model_a` and `model_b` accept exact built-in strings before normalization; `k_a` and `k_b` accept exact built-in integers or genuine supported NumPy integer scalar classes; and `alpha` and `omega_tol` accept exact built-in real values or genuine supported NumPy integer/floating scalar classes. Caller-defined Python/NumPy subclasses and arbitrary integer-protocol providers are rejected before `__index__`, `__int__`, `__float__`, string-normalization, or representation callbacks can execute. Stable package-owned validation messages remain non-reflective. This hardening changes only the validation boundary. Rust-backed Vuong arithmetic, relation-safe routing, accepted relation identities, thresholds, result fields, and scientific interpretation remain unchanged. ## Security rationale -Python's data model specifies that `str(object)` invokes `object.__str__()`. The built-in `float()` conversion for a general object delegates to numeric conversion methods such as `__float__()`. Performing those conversions on untrusted controls therefore executes caller-defined behavior before a finite vocabulary/range contract has been established. Validation should first prove that the runtime value belongs to the package's accepted scalar/type domain, then normalize trusted values only. +Python's data model permits general integer conversion protocols such as `__index__` and numeric conversions such as `__float__`; string subclasses may also override normalization methods. Invoking these protocols before the package establishes a trusted scalar or label identity executes caller-controlled behavior inside a validation boundary. Module-name or inheritance metadata is not sufficient evidence that a scalar instance is one of the genuine NumPy scalar classes intended by the public contract. -The fail-first regressions use hostile objects whose `__str__`, `__repr__`, or `__float__` raise. Acceptance requires the existing package-owned `ValueError` surfaces without invoking those callbacks and without changing the Rust numerical path. +Validation therefore admits only exact built-in control types and an explicit finite set of genuine NumPy scalar classes, then normalizes those already-trusted values. This keeps user-defined subclasses and arbitrary protocol providers outside the trusted conversion boundary without changing the Rust numerical owner. + +The fail-first regressions use hostile `__index__`, integer-subclass, string-subclass, and spoofed NumPy floating-subclass controls. Acceptance requires the existing package-owned `ValueError` surfaces with zero hostile callback executions while ordinary supported NumPy scalars continue to normalize successfully. ## Verification contract - preserve every accepted `ModelRelation` identity and built-in-string relation value; -- preserve accepted built-in and NumPy real scalar semantics for `alpha` and `omega_tol`; -- preserve Boolean, non-finite, probability-range, and non-negative tolerance rejection; -- reject arbitrary caller objects before `str()`, `repr()`, or `float()` hooks execute; +- preserve exact built-in model labels while rejecting caller-defined string subclasses before normalization; +- preserve exact built-in integers and genuine supported NumPy integer scalars for non-negative parameter counts; +- preserve accepted exact built-in and genuine NumPy real-scalar semantics for `alpha` and `omega_tol`; +- preserve Boolean, non-finite, probability-range, non-negative count, and non-negative tolerance rejection; +- reject arbitrary integer-protocol providers and caller-defined Python/NumPy scalar subclasses before conversion or representation hooks execute; - preserve Rust-owned model-comparison arithmetic and scientific routing; - require focused regression coverage, full repository CI, Security Scan, SAST, and current-head review before integration. diff --git a/python/fast_mlsirm/model_comparison.py b/python/fast_mlsirm/model_comparison.py index 9c809a533..edc62155a 100644 --- a/python/fast_mlsirm/model_comparison.py +++ b/python/fast_mlsirm/model_comparison.py @@ -12,14 +12,36 @@ from dataclasses import dataclass from enum import Enum import math -import operator from typing import Any +import numpy as np + from .fitstats import vuong_nonnested MAX_CASEWISE_VALUES = 1_000_000 MAX_MODEL_LABEL_CHARS = 128 +_NUMPY_INTEGER_SCALAR_TYPES = ( + np.int8, + np.int16, + np.int32, + np.int64, + np.intp, + np.longlong, + np.uint8, + np.uint16, + np.uint32, + np.uint64, + np.uintp, + np.ulonglong, +) +_NUMPY_FLOAT_SCALAR_TYPES = ( + np.float16, + np.float32, + np.float64, + np.longdouble, +) + class ModelRelation(str, Enum): """Declared mathematical relationship between two candidate models.""" @@ -92,8 +114,8 @@ def _is_boolean_like(value: Any) -> bool: def _model_label(value: str, name: str) -> str: - """Return a bounded printable model label suitable for audit output.""" - if not isinstance(value, str): + """Return a bounded printable exact-string label suitable for audit output.""" + if type(value) is not str: raise ValueError(f"{name} must be a non-empty string") normalized = value.strip() if not normalized: @@ -121,36 +143,34 @@ def _relation(value: ModelRelation | str) -> ModelRelation: def _parameter_count(value: Any, name: str) -> int: - """Return a non-negative integer parameter count while rejecting booleans.""" - if _is_boolean_like(value): + """Return an exact trusted non-negative integer parameter count.""" + value_type = type(value) + if value_type is int: + normalized = value + elif any(value_type is trusted for trusted in _NUMPY_INTEGER_SCALAR_TYPES): + normalized = int(value) + else: raise ValueError(f"{name} must be a non-negative integer") - try: - normalized = operator.index(value) - except MemoryError: - raise - except Exception: - raise ValueError(f"{name} must be a non-negative integer") from None if normalized < 0: raise ValueError(f"{name} must be a non-negative integer") - return int(normalized) + return normalized def _trusted_real_scalar(value: Any, message: str) -> float: - """Return a built-in or NumPy real scalar without custom coercion hooks.""" + """Return an exact built-in or genuine NumPy real scalar.""" value_type = type(value) - if _is_boolean_like(value): + trusted = ( + value_type is int + or value_type is float + or any(value_type is candidate for candidate in _NUMPY_INTEGER_SCALAR_TYPES) + or any(value_type is candidate for candidate in _NUMPY_FLOAT_SCALAR_TYPES) + ) + if not trusted: raise ValueError(message) - if value_type is int or value_type is float: + try: return float(value) - if value_type.__module__.startswith("numpy"): - mro = value_type.__mro__ - if any( - base.__module__.startswith("numpy") - and base.__name__ in {"integer", "floating"} - for base in mro - ): - return float(value) - raise ValueError(message) + except OverflowError: + raise ValueError(message) from None def _alpha_value(value: Any) -> float: diff --git a/tests/test_model_comparison_control_callback_safety.py b/tests/test_model_comparison_control_callback_safety.py new file mode 100644 index 000000000..da3215f70 --- /dev/null +++ b/tests/test_model_comparison_control_callback_safety.py @@ -0,0 +1,159 @@ +"""Callback-safety regressions for model-comparison control metadata.""" + +from __future__ import annotations + +import numpy as np +import pytest + +from fast_mlsirm.model_comparison import ( + ComparisonStatus, + ModelRelation, + compare_nonnested_models, +) + + +_CASEWISE_A = (0.0, 0.1) +_CASEWISE_B = (0.0, 0.1) + + +class _HostileIndex: + """Arbitrary integer protocol provider whose callback must stay inert.""" + + calls = 0 + + def __index__(self) -> int: + """Record forbidden integer coercion.""" + type(self).calls += 1 + return 1 + + +class _HostileInt(int): + """Caller-defined integer subclass outside the trusted control boundary.""" + + calls = 0 + + def __index__(self) -> int: + """Record forbidden integer coercion if dispatched.""" + type(self).calls += 1 + return int.__index__(self) + + +class _HostileStr(str): + """Caller-defined string subclass whose normalization must not execute.""" + + calls = 0 + + def strip(self, *args: object, **kwargs: object) -> str: + """Record forbidden label normalization.""" + type(self).calls += 1 + return str.strip(self, *args, **kwargs) + + +class _HostileNumpyFloat(np.float64): + """NumPy-scalar subclass spoofing trusted-looking module metadata.""" + + __module__ = "numpy.user_controlled" + calls = 0 + + def __float__(self) -> float: + """Record forbidden real-scalar normalization.""" + type(self).calls += 1 + return np.float64.__float__(self) + + +def _compare(**overrides: object): + """Call the public comparison boundary without requiring native dispatch.""" + kwargs: dict[str, object] = { + "k_a": 1, + "k_b": 1, + "model_a": "A", + "model_b": "B", + "relation": ModelRelation.UNKNOWN, + "alpha": 0.05, + "omega_tol": 1e-12, + } + kwargs.update(overrides) + return compare_nonnested_models( + _CASEWISE_A, + _CASEWISE_B, + **kwargs, # type: ignore[arg-type] + ) + + +@pytest.mark.parametrize( + ("field", "value", "owner"), + [ + ("k_a", _HostileIndex(), _HostileIndex), + ("k_b", _HostileInt(1), _HostileInt), + ], +) +def test_parameter_controls_reject_untrusted_integer_types_without_callbacks( + field: str, + value: object, + owner: type, +) -> None: + """Parameter counts fail closed before integer-protocol dispatch.""" + owner.calls = 0 + with pytest.raises(ValueError, match=rf"{field} must be a non-negative integer"): + _compare(**{field: value}) + assert owner.calls == 0 + + +def test_model_label_rejects_string_subclass_without_normalization_callback() -> None: + """Audit labels require exact strings before any caller method can run.""" + _HostileStr.calls = 0 + with pytest.raises(ValueError, match="model_a must be a non-empty string"): + _compare(model_a=_HostileStr("A")) + assert _HostileStr.calls == 0 + + +@pytest.mark.parametrize( + ("field", "value", "message"), + [ + ("alpha", _HostileNumpyFloat(0.05), "alpha must be finite and in \\(0, 1\\)"), + ( + "omega_tol", + _HostileNumpyFloat(1e-12), + "omega_tol must be finite and non-negative", + ), + ], +) +def test_real_controls_reject_numpy_subclasses_without_float_callback( + field: str, + value: object, + message: str, +) -> None: + """Real-valued controls reject caller NumPy subclasses before coercion.""" + _HostileNumpyFloat.calls = 0 + with pytest.raises(ValueError, match=message): + _compare(**{field: value}) + assert _HostileNumpyFloat.calls == 0 + + +@pytest.mark.parametrize( + ("field", "message"), + [ + ("alpha", "alpha must be finite and in \\(0, 1\\)"), + ("omega_tol", "omega_tol must be finite and non-negative"), + ], +) +def test_real_controls_normalize_builtin_integer_overflow_to_value_error( + field: str, + message: str, +) -> None: + """Huge trusted integers retain the public field-specific error contract.""" + with pytest.raises(ValueError, match=message): + _compare(**{field: 10**10000}) + + +def test_genuine_numpy_scalars_remain_supported() -> None: + """Trusted NumPy scalar compatibility remains part of the public contract.""" + result = _compare( + k_a=np.int64(1), + k_b=np.uint32(1), + alpha=np.float64(0.05), + omega_tol=np.float32(1e-6), + ) + assert result.status is ComparisonStatus.UNKNOWN_RELATION + assert result.k_a == 1 + assert result.k_b == 1 diff --git a/tests/test_model_comparison_input_error_boundary.py b/tests/test_model_comparison_input_error_boundary.py index afbba7fbd..29214cc95 100644 --- a/tests/test_model_comparison_input_error_boundary.py +++ b/tests/test_model_comparison_input_error_boundary.py @@ -66,10 +66,13 @@ def __float__(self) -> float: class _MemoryIndex: - """Raise resource exhaustion during caller-controlled index coercion.""" + """Probe an untrusted integer protocol that must never be dispatched.""" + + calls = 0 def __index__(self) -> int: - """Preserve process-level resource exhaustion from index conversion.""" + """Record forbidden index conversion before simulating exhaustion.""" + type(self).calls += 1 raise MemoryError("index allocation exhausted") @@ -146,12 +149,22 @@ def test_parameter_count_conversion_failure_is_redacted() -> None: lambda: compare_nonnested_models(_MemoryIteratorFactory(), (0.1, 0.2), 2, 2), lambda: compare_nonnested_models(_MemoryIterable(), (0.1, 0.2), 2, 2), lambda: compare_nonnested_models((_MemoryFloat(), 0.2), (0.1, 0.2), 2, 2), - lambda: compare_nonnested_models( - (0.1, 0.2), (0.1, 0.2), _MemoryIndex(), 2 # type: ignore[arg-type] - ), ], ) def test_memory_error_remains_explicit_resource_exhaustion(callable_) -> None: - """Resource exhaustion must not be downgraded into an input-validation error.""" + """Resource exhaustion from accepted casewise callbacks remains explicit.""" with pytest.raises(MemoryError): callable_() + + +def test_untrusted_memory_index_is_rejected_without_callback() -> None: + """Parameter-count trust is established before arbitrary index protocols run.""" + _MemoryIndex.calls = 0 + with pytest.raises(ValueError, match="k_a must be a non-negative integer"): + compare_nonnested_models( + (0.1, 0.2), + (0.1, 0.2), + _MemoryIndex(), # type: ignore[arg-type] + 2, + ) + assert _MemoryIndex.calls == 0