Skip to content

ModelDownloader: no HTTPS enforcement or SHA-256 verification for downloaded models (CWE-494) #1029

Closed

Description

@fernandotonon

Raised by CodeRabbit on #1028 (BiRefNet matting). Filing separately because it is pre-existing and repo-wide, not specific to that PR.

The gap

ModelDownloader::startDownload accepts a URL with no scheme validation and renames the finished file with no integrity check:

$ grep -nE "https|scheme|QCryptographicHash|sha256" src/ModelDownloader.cpp
(no matches)

Every model base URL is overridable by the user, via env var or QSettings — e.g. QTMESH_REMBG_MODEL_BASE_URL / ai/rembgModelBaseUrl, and the equivalent for pbr, unirig, segment, inbetween, motion, t2m, triposr, skintokens, facerig, depth. So a plain-HTTP source or a tampered mirror can substitute any model file, and it is loaded without verification.

CWE-494 (Download of Code Without Integrity Check). Reachability is internal and exploitability difficult — it needs either an attacker-controlled override or a MITM on a plain-HTTP base URL — but the blast radius is every ONNX/GGUF consumer.

Scope

21 files use ModelDownloader::instance(). This is not one feature's problem:

AIAssistManager, MotionInbetween, PhotoDepth, MeshSegmenter, AIModelCatalog,
UniRigPredictor, MotionGenerator, SkinTokensPredictor, BackgroundRemover, …

Suggested fix

  1. Reject non-HTTPS in startDownload (allow file:// for local testing, and keep the existing env/QSettings override working for a trusted mirror).
  2. Verify SHA-256 before emitting downloadCompleted — the digests are already known: the HF API returns each file's LFS oid, which is its sha256, and several recent PRs (feat(#1018): photo depth estimation via Depth-Anything-V2-Small (exported, parity-verified, hosted) #1027, feat(#1016): BiRefNet high-quality matting tier (exported, parity-verified, hosted) #1028) verified uploads that way. A per-model manifest of expected digests would make this a one-line check per consumer.
  3. Delete the partial file on mismatch rather than leaving it cached, so a corrupted fetch cannot poison later runs.

Why not fixed in #1028

That PR adds a matting tier; it does not touch ModelDownloader. Fixing a shared download path there would mean changing behaviour for 21 consumers under an unrelated title, with no test coverage for the other 20. Better as its own change with its own verification.

Worth noting a related precedent: the UniRig export shipped a graph emitting 100% NaN latents and went unnoticed for months (#1025). Integrity checking at download time would not have caught that (the file was intact, the export was wrong), but it is the same class of "we trust an artifact we never verified" problem, and both are worth closing.

Activity

  1. added 3 commits that reference this issue on Sep 16, 2026
  2. added 2 commits that reference this issue on Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions