Skip to content

Guard loading against suspicious zip members#515

Closed
massy-o wants to merge 1 commit into
skops-dev:mainfrom
massy-o:harden-zip-member-loading
Closed

Guard loading against suspicious zip members#515
massy-o wants to merge 1 commit into
skops-dev:mainfrom
massy-o:harden-zip-member-loading

Conversation

@massy-o
Copy link
Copy Markdown

@massy-o massy-o commented May 14, 2026

Summary

  • add a guarded ZIP member read helper to LoadContext
  • reject members with excessive uncompressed size or suspicious compression ratios before reading them into memory
  • route NumPy, bytes, and scipy sparse member reads through the guarded helper
  • add a regression test covering loads() and get_untrusted_types()

Tests

  • uv run --with-editable '.[rich]' --with pytest --with pytest-cov --with flaky --with pandas --with matplotlib pytest skops/io/tests/test_persist.py -k 'compression_ratio or compression_level or sparse_matrix'
  • local malformed .skops artifact check against load() and get_untrusted_types()

Note: ruff check on the touched files currently reports an existing E721 issue in skops/io/_general.py outside this change.

@massy-o massy-o force-pushed the harden-zip-member-loading branch from 516873c to 96f359c Compare May 14, 2026 04:40
@massy-o massy-o marked this pull request as ready for review May 14, 2026 04:41
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there's some common patterns in loading zip files in other libraries which we're replicating here, I wouldn't want to put these limits. This basically means the user can't load large objects, and users might have legit usecases for loading large objects. So I'm not sure if this heurestic is sensible here.

Happy to reopen if there are good arguments for this change, otherwise closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants