[Security] SHA-256 validation #531
Conversation
| XamlRoot = this.XamlRoot | ||
| }; | ||
|
|
||
| var result = await dialog.ShowAsync(); |
There was a problem hiding this comment.
When you close the app under this logic, an intermediate state issue should occur.
There was a problem hiding this comment.
Nice catch. I added cleanup logic to handle verification-failed models on app close.
| } | ||
| }, | ||
| PrimaryButtonText = "Delete model", | ||
| SecondaryButtonText = "Keep anyway", |
There was a problem hiding this comment.
What is the security rationale for allowing users to keep potentially
compromised models?
There was a problem hiding this comment.
Initially I didn't implement the "Keep" option, but I added it after considering that some users are developers who understand the risks. There are also legitimate scenarios where verification may fail (e.g., hash metadata out of sync on the source), and without this option, developers would have no way to use a model if verification consistently fails. The default action is "Delete" and keeping requires explicit user choice. Do you think we should not give developers any option to keep verification-failed files?
There was a problem hiding this comment.
makes sense. Since our target audience is developers, I agree we shouldn't block them completely.
To mitigate the risk, I suggest 2 improvements:
1.If user chooses to "keep anyway", its status should not simply change to Completed (which makes it look identical to a normal model). It will be better to display an “Unverified” label or a warning icon in the UI to remind users that this model carries potential risks.
2. The current warning msg is quite technical but doesn't clearly explain the security implications to users who may not fully understand what match the expected hash means. maybe we can consider like: "This could indicate file tampering or corruption. You will be using an unverified model that may behave unexpectedly"
AIDevGallery/Utils/ModelDownload.cs
Outdated
| if (fileInfo.Length != downloadableFile.Size) | ||
| { | ||
| // file did not download properly, should retry | ||
| throw new IOException($"File size mismatch for {downloadableFile.Name}: expected {downloadableFile.Size}, got {fileInfo.Length}"); |
There was a problem hiding this comment.
Where is this exception handled?
There was a problem hiding this comment.
file size mismatch throws an IOException immediately, while SHA-256 verification failure allows user choice.
Should we handle size mismatches similarly (with user choice) or should both be treated as hard failures?
There was a problem hiding this comment.
Good catch. The original code had an empty if block here. I'm trying to address this issue for the legacy code. Size imcomplete sounds more like a hard failure to me since the files will not be used. Any thoughts?
| { | ||
| DownloadUrl = $"https://huggingface.co/{hfUrl.Organization}/{hfUrl.Repo}/resolve/{hfUrl.Ref}/{f.Path}", | ||
| Size = f.Size, | ||
| Size = f.Lfs?.Size ?? f.Size, |
There was a problem hiding this comment.
LFS size is always reliable and available? this will not break existing downloads where Lfs is null?
There was a problem hiding this comment.
Good question. The ?? operator should handle this: if Lfs is null (non-LFS files), it falls back to f.Size, which is the original logic. This change here is for some of my local HF validation logic which has been removed. We can revert it back. Let me clean it up.
Summary
This PR adds file integrity verification for model downloads from both Hugging Face and GitHub repositories using SHA-256 hashing.
Changes
Core Verification Logic
oidfield in the API responseModel Download Enhancements
Verifyingstatus to the download progress statesVerificationFailedstatus when hash mismatch is detectedUI Updates