Skip to content

fix(security): harden backup import and OOXML extraction - #61881

Open
zapabob wants to merge 4 commits into
NousResearch:mainfrom
zapabob:codex/security-backup-import-hardening
Open

fix(security): harden backup import and OOXML extraction#61881
zapabob wants to merge 4 commits into
NousResearch:mainfrom
zapabob:codex/security-backup-import-hardening

Conversation

@zapabob

@zapabob zapabob commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Bound backup ZIP member count, compressed size, expanded size, and compression ratio before extraction.
  • Count directory entries, reject normalized-member collisions, validate archive link members, and stream the exact validated ZipInfo objects.
  • Normalize the optional .hermes/ wrapper before external-state classification and confine restoration to declared provider roots.
  • Bound DOCX and XLSX archive expansion in tools/read_extract.py.
  • Preserve the upstream Tirith checksum fallback unchanged; this PR no longer changes Tirith provenance behavior.

Validation

  • scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/tools/test_read_extract.py -q
  • git diff --check upstream/main...HEAD
  • Diff against current upstream is restricted to backup import, OOXML extraction, and regression tests.

No fork-only plugins or _docs files are included.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #43942 (open — curator rollback tar symlink/abs-path hardening) and merged #50354 (same hermes_cli/backup.py, snapshot/manifest traversal guards). Not a duplicate: this adds zip-bomb / oversized-member bounds plus external-provider path confinement on the hermes import extraction path. Defense-in-depth hardening (operator supplies the archive) — out-of-scope of the OS-isolation security boundary per SECURITY.md §3.2.

@zapabob
zapabob force-pushed the codex/security-backup-import-hardening branch 2 times, most recently from cfd1655 to 23cb257 Compare July 10, 2026 08:11
@zapabob zapabob changed the title fix(security): bound backup archive imports fix(security): bound archive resource usage Jul 10, 2026
@zapabob
zapabob force-pushed the codex/security-backup-import-hardening branch from 23cb257 to 987e37c Compare July 10, 2026 08:43
@zapabob zapabob changed the title fix(security): bound archive resource usage fix(security): bound resource use and verify Tirith provenance Jul 10, 2026
@zapabob
zapabob force-pushed the codex/security-backup-import-hardening branch 2 times, most recently from a87f8c5 to 4d56e7f Compare July 10, 2026 10:11

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the archive hardening work. The backup-import premise is live on current main: hermes_cli/backup.py:543-545 enumerates every ZIP member and :645-646 reads each member unbounded; DOCX/XLSX extraction likewise reads archive members without resource validation at tools/read_extract.py:109-110 and :135-149.

Problems

  • tools/tirith_security.py:424 makes cosign mandatory for auto-install. This reverses intentional behavior from e3f9894c (“tirith cosign-free install”): current main deliberately proceeds with SHA-256 verification when cosign or its artifacts are unavailable (tools/tirith_security.py:424-451). The user-facing security documentation matches that contract (website/docs/user-guide/security.md:592). Requiring cosign removes the scanner for ordinary supported-platform auto-installs rather than hardening its existing behavior.

Suggested changes

  • Keep the ZIP/OOXML resource limits and external-provider restore confinement, but drop the Tirith provenance behavior and its test rewrites. Retain the existing abort on an explicit cosign rejection while preserving the SHA-256 fallback for unavailable/broken provenance verification.

Automated hermes-sweeper review.

Comment thread tools/tirith_security.py Outdated
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@zapabob

zapabob commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Implemented the maintainer's requested contract correction.\n\n- Restored SHA-256 verification as the always-required integrity check.\n- Restored optional cosign provenance verification when cosign and its artifacts are available.\n- cosign absence, unavailable artifacts, and cosign execution failure fall back to SHA-256.\n- An explicit cosign verification rejection remains fail-closed.\n- Archive resource limits, member validation, and restore-target confinement remain unchanged.\n\nFocused verification: uv run pytest -q tests/tools/test_tirith_security.py passed (95 tests); Ruff passed for the changed files.

@zapabob
zapabob force-pushed the codex/security-backup-import-hardening branch from deca63d to b57d0bc Compare July 12, 2026 12:03
@zapabob zapabob changed the title fix(security): bound resource use and verify Tirith provenance fix(security): harden backup import and OOXML extraction Jul 12, 2026

zapabob commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up is pushed. I removed the Tirith provenance change from the effective diff and preserved the upstream checksum fallback. The remaining change is limited to backup-import and OOXML extraction resource/path validation; the focused backup and extraction tests pass.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants