fix(keras): prevent spoofed built-in registered_name from hiding non-allowlisted modules#736
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAggregates module references from Keras layer objects/configs, updates flagging to detect non-allowlisted module references even when Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/keras_zip_scanner.py`:
- Around line 145-153: The current branch lets an identifier suppress warnings
if any allowlisted ref is present even when a non-allowlisted ref exists; change
the logic so suppression only occurs when there are zero non-allowlisted refs.
Concretely, in the block around normalized_registered_name ==
layer_class.strip() update the return condition to require that
_layer_uses_allowlisted_module(layer) is True,
_is_known_safe_allowlisted_registered_object(layer_class) is True, and
_layer_uses_non_allowlisted_module(layer) is False before suppressing; keep the
existing behavior of _is_known_safe_serialized_layer and otherwise flag when any
non-allowlisted module is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92d2037d-7cf9-42b9-a53e-306a21d428ec
📒 Files selected for processing (3)
CHANGELOG.mdmodelaudit/scanners/keras_zip_scanner.pytests/scanners/test_keras_zip_scanner.py
Harden Keras ZIP custom object detection so built-in registered_name values do not hide non-allowlisted modules. Co-Authored-By: Codex <noreply@openai.com>
mldangelo
force-pushed
the
codex/fix-keras-zip-scanner-to-detect-spoofed-objects
branch
from
980d940 to
666358a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c2c048f-de52-46d9-b122-6b52d4164c24
📒 Files selected for processing (3)
CHANGELOG.mdmodelaudit/scanners/keras_zip_scanner.pytests/scanners/test_keras_zip_scanner.py
Avoid emitting a duplicate Custom Object Detection warning for allowlisted registered objects when serialized module metadata is absent, while still flagging any non-allowlisted module reference. Co-authored-by: Codex <noreply@openai.com>
Extend the allowlisted registered-object exception to serialized layers with absent module refs so the no-module path does not raise custom-layer or custom-object false positives. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/keras_zip_scanner.py`:
- Around line 121-143: The current logic allows a layer like {"class_name":
"Add", "module": "keras.src.ops.numpy", "config": {"module": "evil.module"}} to
bypass detection because _is_known_safe_serialized_layer() never enforces that
all module refs are allowlisted; only the _should_flag_registered_object() path
uses the non-allowlist check. Update _is_known_safe_serialized_layer() (and any
place that trusts _SAFE_ALLOWLISTED_REGISTERED_OBJECTS) to call the existing
helper that detects non-allowlisted refs—use
_layer_uses_non_allowlisted_module(layer) (or a new thin wrapper) to reject
layers that have any non-allowlisted module references before treating
class_name or registered-name as safe so mixed/unsafe module refs cannot
suppress custom-layer detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31335720-8369-44a7-bf28-02f5129d0e83
📒 Files selected for processing (3)
CHANGELOG.mdmodelaudit/scanners/keras_zip_scanner.pytests/scanners/test_keras_zip_scanner.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelaudit/scanners/keras_zip_scanner.py (1)
149-156:⚠️ Potential issue | 🟠 MajorBuilt-in
class_namevalues bypass module allowlist validation.The early return at lines 151-152 trusts
is_known_safe_keras_layer_class(layer_class)without verifying module references. A crafted layer like{"class_name": "Add", "module": "evil.module", "config": {}}(noregistered_name) will:
- Return
Truehere because"Add"is a known-safe layer class- Skip
Custom Layer Class Detection- Skip
Custom Object Detection(noregistered_name)- Skip CVE-2025-1550 WARNING (not Lambda, not
fn_module, not dangerous)The module allowlist check added at lines 154-156 only protects the
_SAFE_ALLOWLISTED_REGISTERED_OBJECTSbranch, not built-in class names. Test coverage exists for scenarios withregistered_namepresent, but the exact case without it (built-in class + evil module + no registered_name) is untested.🔒 Suggested fix to validate modules for built-in class names
def _is_known_safe_serialized_layer(self, layer: dict[str, Any]) -> bool: layer_class = layer.get("class_name") - if is_known_safe_keras_layer_class(layer_class): - return True + if is_known_safe_keras_layer_class(layer_class) or self._is_known_safe_allowlisted_registered_object( + layer_class + ): + return not self._layer_uses_non_allowlisted_module(layer) - return self._is_known_safe_allowlisted_registered_object( - layer_class - ) and not self._layer_uses_non_allowlisted_module(layer) + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/keras_zip_scanner.py` around lines 149 - 156, The current _is_known_safe_serialized_layer returns True for any class_name that is_known_safe_keras_layer_class reports safe without validating the layer's module; change _is_known_safe_serialized_layer so both branches validate the module allowlist: when is_known_safe_keras_layer_class(layer_class) is True, also require that not self._layer_uses_non_allowlisted_module(layer) before returning True, and keep the existing combined check for the registered-object branch (_is_known_safe_allowlisted_registered_object and not _layer_uses_non_allowlisted_module). This ensures built-in class names with a non-allowlisted "module" (e.g., {"class_name":"Add","module":"evil.module"}) are treated as unsafe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scanners/test_keras_zip_scanner.py`:
- Around line 913-942: Add a new test function mirroring
test_allowlisted_registered_object_without_module_does_not_false_positive_custom_object
that constructs a Keras config where a built-in class_name ("Add") is present
with a non-allowlisted "module" and no "registered_name", call
KerasZipScanner().scan on the zip produced by create_configured_keras_zip, and
assert that the scan returns at least one warning/critical issue (check
result.issues for IssueSeverity.WARNING or IssueSeverity.CRITICAL) to ensure
builtin class_name + evil module is flagged; place the test alongside the
existing tests and name it something like
test_builtin_class_with_non_allowlisted_module_and_no_registered_name_is_flagged
so it covers the bypass scenario.
---
Outside diff comments:
In `@modelaudit/scanners/keras_zip_scanner.py`:
- Around line 149-156: The current _is_known_safe_serialized_layer returns True
for any class_name that is_known_safe_keras_layer_class reports safe without
validating the layer's module; change _is_known_safe_serialized_layer so both
branches validate the module allowlist: when
is_known_safe_keras_layer_class(layer_class) is True, also require that not
self._layer_uses_non_allowlisted_module(layer) before returning True, and keep
the existing combined check for the registered-object branch
(_is_known_safe_allowlisted_registered_object and not
_layer_uses_non_allowlisted_module). This ensures built-in class names with a
non-allowlisted "module" (e.g., {"class_name":"Add","module":"evil.module"}) are
treated as unsafe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1caf056f-4dda-403d-b9dc-46f5c65f578e
📒 Files selected for processing (2)
modelaudit/scanners/keras_zip_scanner.pytests/scanners/test_keras_zip_scanner.py
Treat built-in Keras class names as unsafe when any serialized module reference points outside the allowlist, and add regression coverage for the no-registered_name bypass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/keras_zip_scanner.py`:
- Around line 151-155: The check that treats registered allowlisted objects as
safe currently passes layer_class into
_is_known_safe_allowlisted_registered_object; change the call sites in the Keras
ZIP scanner (the branch that also checks is_known_safe_keras_layer_class) to
pass the layer's registered_name (not class_name) when available, then ensure
_is_known_safe_allowlisted_registered_object uses registered_name for matching;
keep the fallback to class_name only if registered_name is missing. Also update
the related branch that calls _layer_uses_non_allowlisted_module to use the same
registered_name logic and add a regression test that constructs a layer with
class_name != registered_name (e.g.,
{"class_name":"Wrapper","registered_name":"NotEqual","config":{}}) asserting it
is treated as allowlisted and reported safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 423e31c4-807b-40e6-ab30-1671d2866bef
📒 Files selected for processing (2)
modelaudit/scanners/keras_zip_scanner.pytests/scanners/test_keras_zip_scanner.py
| if is_known_safe_keras_layer_class(layer_class) or self._is_known_safe_allowlisted_registered_object( | ||
| layer_class | ||
| ) | ||
| ): | ||
| return not self._layer_uses_non_allowlisted_module(layer) | ||
|
|
There was a problem hiding this comment.
Match allowlisted registered objects against registered_name, not only class_name.
Line 151 and Line 167 still feed layer_class into _is_known_safe_allowlisted_registered_object(). A payload like {"class_name": "Wrapper", "registered_name": "NotEqual", "config": {}} will still be reported as a custom layer/object even though the registered object is allowlisted and there are no unsafe module refs. Please key this safe-object path off registered_name here and add a regression where class_name != registered_name.
🔧 Suggested direction
def _is_known_safe_serialized_layer(self, layer: dict[str, Any]) -> bool:
layer_class = layer.get("class_name")
+ registered_name = layer.get("registered_name")
if is_known_safe_keras_layer_class(layer_class) or self._is_known_safe_allowlisted_registered_object(
- layer_class
+ registered_name
+ ) or self._is_known_safe_allowlisted_registered_object(
+ layer_class
):
return not self._layer_uses_non_allowlisted_module(layer)
return False
...
layer_class = layer.get("class_name")
if isinstance(layer_class, str) and normalized_registered_name == layer_class.strip():
- if self._is_known_safe_serialized_layer(layer) or self._is_known_safe_allowlisted_registered_object(
- layer_class
- ):
+ if self._is_known_safe_serialized_layer(layer) or self._is_known_safe_allowlisted_registered_object(
+ normalized_registered_name
+ ):
return has_non_allowlisted_module
return TrueBased on learnings: Preserve or strengthen security detections; test both benign and malicious samples when adding scanner/feature changes.
Also applies to: 167-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/keras_zip_scanner.py` around lines 151 - 155, The check
that treats registered allowlisted objects as safe currently passes layer_class
into _is_known_safe_allowlisted_registered_object; change the call sites in the
Keras ZIP scanner (the branch that also checks is_known_safe_keras_layer_class)
to pass the layer's registered_name (not class_name) when available, then ensure
_is_known_safe_allowlisted_registered_object uses registered_name for matching;
keep the fallback to class_name only if registered_name is missing. Also update
the related branch that calls _layer_uses_non_allowlisted_module to use the same
registered_name logic and add a regression test that constructs a layer with
class_name != registered_name (e.g.,
{"class_name":"Wrapper","registered_name":"NotEqual","config":{}}) asserting it
is treated as allowlisted and reported safe.
…canner-to-detect-spoofed-objects
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Reviewed locally after updating with main: spoofed built-in class bypass fixed, targeted Keras/MAR tests passed, full non-slow suite passed.
mldangelo-oai
deleted the
codex/fix-keras-zip-scanner-to-detect-spoofed-objects
branch
2 participants
Motivation
.kerasZIP custom-object detection because spoofed built-inregistered_namevalues such as"Add"could hide a non-allowlisted module reference and bypass warnings.moduleorfn_modulereference.Description
_iter_layer_module_references()and track both allowlisted and non-allowlisted refs.mainand resolve theCHANGELOG.mdconflict under the active[Unreleased]section.Testing
uv run pytest tests/scanners/test_keras_zip_scanner.py -k "registered_builtin_layer_does_not_false_positive or builtin_registered_name_with_non_allowlisted_module_is_flagged or builtin_registered_name_with_mixed_module_refs_is_flagged or allowlisted_module_layer_does_not_false_positive or allowlisted_module_does_not_suppress_unknown_custom_layer"uv run ruff format modelaudit/ tests/uv run ruff check --fix modelaudit/ tests/uv run mypy modelaudit/uv run pytest -n auto -m "not slow and not integration" --maxfail=1Codex Task
Summary by CodeRabbit
Changelog
Bug Fixes
Tests