Skip to content

fix(python): verify TLS cert and model hash on download #342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AAtomical wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(python): verify TLS cert and model hash on download #342

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions python/inspireface/modules/utils/resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,10 @@ def get_model(self, name: str, re_download: bool = False, ignore_verification: b
print(f"Downloading model '{name}'...")
downloading_flag.touch()

# Create SSL context and headers
# Create SSL context and headers. Keep certificate and hostname
# verification enabled (the defaults) so the download cannot be
# silently MITM'd by a network attacker.
ssl_context = ssl.create_default_context()
ssl_context.check_hostname = False
ssl_context.verify_mode = ssl.CERT_NONE
headers = {'User-Agent': 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36'}

req = urllib.request.Request(model_info["url"], headers=headers)
Expand All @@ -210,6 +210,21 @@ def get_model(self, name: str, re_download: bool = False, ignore_verification: b
sys.stdout.flush()

print("\nDownload completed")

# Verify the integrity of the freshly downloaded file before
# handing it back to the caller. Without this check a corrupted or
# attacker-supplied payload would be loaded as-is.
if ignore_verification:
print(f"Warning: Model verification skipped for '{name}' as requested.")
else:
downloaded_hash = get_file_hash_sha256(model_file)
if downloaded_hash != model_info["md5"]:

@devloai devloai Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The field is named "md5" in _MODEL_LIST but it stores SHA-256 hashes (64-char hex strings), and the comparison on line 221 is against get_file_hash_sha256(). This is inconsistent with the cached-file path which also uses get_file_hash_sha256() for the same field (line 175–176) — so the logic is at least self-consistent, but the key name "md5" is a clear misnomer that is preserved here.

This PR is a good opportunity to rename the field (or add an alias key), or at minimum add a comment clarifying that "md5" actually holds a SHA-256 digest, so future maintainers don't "fix" it by switching to hashlib.md5.

Suggested change
if downloaded_hash != model_info["md5"]:
if downloaded_hash != model_info["md5"]: # key name is misleading; value is a SHA-256 hex digest
actions

Feedback: Rate this comment to help me improve future code reviews:

  • 👍 Good - Helpful and accurate
  • 👎 Poor - Wrong, unclear, or unhelpful
  • Skip if you don't have any strong opinions either way.

model_file.unlink()
raise RuntimeError(
f"Downloaded model hash mismatch for '{name}': "
f"expected {model_info['md5']}, got {downloaded_hash}"
)

@devloai devloai Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When hash verification fails, the RuntimeError raised on line 223 is caught by the outer except Exception block, which re-raises a new RuntimeError. The original exception is not chained (raise ... from e is missing), so the specific mismatch message (expected vs. actual hash) gets buried inside the string f"Failed to download model: {e}" rather than appearing as a proper cause in the traceback.

Additionally, downloading_flag IS cleaned up by the except handler (line 234–235 checks .exists() before unlinking), so that's fine.

Fix suggestion for the outer except:

        except Exception as e:
            model_file.unlink(missing_ok=True)
            downloading_flag.unlink(missing_ok=True)
            raise RuntimeError(f"Failed to download model: {e}") from e
actions

Feedback: Rate this comment to help me improve future code reviews:

  • 👍 Good - Helpful and accurate
  • 👎 Poor - Wrong, unclear, or unhelpful
  • Skip if you don't have any strong opinions either way.


downloading_flag.unlink() # Remove the downloading flag
return str(model_file)

Expand Down