Skip to content

Strengthening security for TH repo - #235

Merged
albertocarpentieri merged 3 commits into
mainfrom
tkurth/security-updates
Aug 5, 2026
Merged

Strengthening security for TH repo#235
albertocarpentieri merged 3 commits into
mainfrom
tkurth/security-updates

Conversation

@azrael417

Copy link
Copy Markdown
Collaborator

Harden the 2D3DS dataset downloader and CI supply chain

Problem

Two security issues were reported against torch_harmonics/examples/stanford_2d3ds_dataset.py,
which ships as part of the installable torch_harmonics.examples package:

  1. Path traversal on extraction. _extract_tar() called tar.extractall() with no member
    filtering. A tar entry with ../ components — or a symlink pointing outside the tree — could
    write anywhere the process has permission. base_url is user-controllable, so a malicious
    mirror or a compromised upstream server turns this into arbitrary file write.

  2. No download integrity check. _download_file() fetched multi-GB tars over HTTPS with no
    checksum or signature, so a compromised server or an intercepted connection (TLS interception
    is common in corporate environments) could substitute a crafted archive. Combined with (1),
    this is a supply-chain path into a user's filesystem.

While fixing these, three latent bugs surfaced in the same download path, plus some CI hardening
that addresses the same class of risk on the build side.

Changes

Dataset downloader

  • Path traversal guard. Extraction now uses tar.extractall(filter="data") where available.
    data_filter was backported to 3.10.12+, so 3.10.0–3.10.11 remain in the supported range and
    fall back to _check_tar_members(), which validates each member path against the destination
    and additionally checks symlink/hardlink targets (a name-only check misses area_1/x -> ../../../etc/passwd).
  • SHA-256 verification. New DEFAULT_TAR_FILE_CHECKSUMS table covering all seven archives,
    verified after download and on the "file already exists" path, so a poisoned or truncated
    file left from an earlier run is not silently trusted. New checksums constructor argument
    accepts a custom mapping for mirrors, or None to opt out.
  • Fix: missing raise_for_status(). HTTP error bodies were written to disk as .tar files
    and handed to tarfile; the visible symptom was ReadError: not a gzip file from a saved 404 page.
  • Fix: .part path. local_path.split(".")[0] + ".part" breaks on any local_dir containing
    a dot — ./data wrote the partial file into the current working directory, v1.0/data
    produced v1.part. Now uses os.path.splitext.
  • Fix: resume corruption. The Range resume path appended unconditionally. If the server
    ignored the header and replied 200 with a full body, the partial file was corrupted. Resume
    now requires a 206, otherwise the download restarts cleanly.

Packaging

  • Minimum Python raised to 3.10 (requires-python, trove classifier, cibuildwheel build,
    the wheel matrix in build_wheels.yml, setup-python in tests/style, CONTRIBUTING).
    3.9 reached end of life in October 2025.
  • Removed tarfile from the 2d3ds extra. tarfile is stdlib and the name is unregistered
    on PyPI, so pip install torch-harmonics[2d3ds] failed at resolution with
    "No matching distribution found for tarfile".

CI supply chain

  • permissions: contents: read added to build_wheels.yml, tests.yml, and style.yml.
    The repo default was "read and write", so every job — including the one producing published
    wheels — ran with a token that could push commits and move tags. docs.yaml already scoped
    its own permissions and is unchanged. (The repo-level default and "Allow GitHub Actions to
    create and approve pull requests" were also turned off in repo settings.)
  • Pinned build tooling in .github/requirements/: cibuildwheel, twine, auditwheel,
    wheel (host); pip, setuptools, wheel, setuptools-scm (inside the manylinux
    containers); pre-commit (style). These previously resolved to whatever was latest at build
    time and run inside the job that produces the published wheels. Pinned to the versions CI was
    already resolving, so this is behavior-neutral today.
  • Dependabot pip entry on /.github/requirements so the pins don't go stale.

Testing

No GPU or kernel code is touched, so the kernel/benchmark workflow does not apply.

Dataset downloader — exercised end to end against a local HTTP server serving crafted tars
(Python 3.12, torch 2.9.0), rather than the 225 GB real dataset:

Scenario Result
Valid tar, matching checksum downloads, verifies, extracts
Server serves bytes other than expected RuntimeError before extraction; target dir never created
Malicious tar (../../pwned.txt), checksum matches blocked by extract filter; nothing written outside dest
Symlink escaping the destination blocked on the manual fallback path
Corrupt file already on disk rejected on the skip-download path
Interrupted download resumed via Range resumes, final checksum verifies, no stray .part in cwd
HTTP 404 HTTPError, nothing written to disk
checksums=None verification skipped as intended

Both extraction paths were tested — the stdlib data_filter and, with tarfile.data_filter
deleted to simulate 3.10.0–3.10.11, the manual fallback.

The Range fix was additionally confirmed against the real ETH server: area_4 resumed from
94% after an interrupted run and hashed to the expected digest.

Checksums — all seven archives (225 GB) were downloaded from cvg-data.inf.ethz.ch on
2026-08-05 via the library's own _download_file(), hashed, and deleted. The committed table
was verified to match the recorded digests exactly and to cover every entry in
DEFAULT_TAR_FILE_PAIRS, so no file falls through to the unverified warning path.

Packaging / CIpyproject.toml and all four workflow YAMLs parse; the three requirements
files resolve with no version conflicts; cibuildwheel 4.2.0's {project} placeholder
substitution in before_build was confirmed in its source (platforms/linux.py:287).

Behavior changes

  • Stanford2D3DSDownloader.__init__ gains a checksums keyword argument (defaulted; existing
    positional calls are unaffected).
  • Downloads whose checksum does not match now raise RuntimeError instead of extracting.
  • Tar members resolving outside local_dir now raise instead of being written.
  • Python 3.9 is no longer supported: source installs fail on Requires-Python, no cp39 wheels
    are built, and 3.9 users on pip install torch-harmonics silently resolve to the last 3.9-
    compatible release.
  • No numerical or API changes to any transform, convolution, or attention path.

Notes and limitations

  • The checksums pin what the ETH server served on 2026-08-05. This defends against future
    tampering, MITM, and silent re-uploads, but cannot retroactively prove those bytes are the
    2D-3D-S authors' originals; cross-checking a second mirror would be needed for that.
  • Pinned tooling covers direct dependencies only — transitive deps still float. Hash-pinned
    lockfiles (--generate-hashes + --require-hashes) would close that and can follow separately.
  • pip install numpy in CIBW_BEFORE_BUILD/BEFORE_TEST was left unpinned deliberately: it
    tests the declared runtime dependency and should track current numpy.
  • Recommend a workflow_dispatch run of build_wheels.yml before the next release tag to
    confirm the pinned toolchain in the real manylinux containers.

@azrael417
azrael417 requested a review from bonevbs August 5, 2026 09:55
@azrael417 azrael417 self-assigned this Aug 5, 2026
@albertocarpentieri
albertocarpentieri merged commit 059454e into main Aug 5, 2026
5 checks passed
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