DownloadIntegration.downloadDone creates an empty file when a file was deleted after a download (ADS :Zone.Identifier)
Categories
(Toolkit :: Downloads API, defect, P3)
Tracking
()
People
(Reporter: robwu, Unassigned)
References
(Blocks 1 open bug)
Details
See https://bugzilla.mozilla.org/show_bug.cgi?id=1316394#c126 for context.
When a file has been downloaded, the file could be gone. The logic at https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/toolkit/components/downloads/DownloadIntegration.jsm#579-616 assumes that the file is still there, but this is not guaranteed. As a result, upon attempting to write to the Alternative Data Stream, an empty file may be created.
The logic also smells to me - why would it only write the Zone Identifier after the completion of the download? I would expect it to be attached to the file before writing any more data to the file, in case the download got interrupted.
|
||
Comment 1•4 years ago
|
||
downloadDone is apparently invoked when a download succeeds, thus I think it expects the file to be there.
When you say it could be gone, what is removing it, is this a race condition due to asynchronicity? It sounds like one from the original comment.
For the Zone Identifier, I agree it could probably be written earlier, likely it doesn't matter much if the file has a .part extension, and it should be moved if the file is then renamed at the end of the download. So it was maybe made for simplicity?
|
Reporter | |
Comment 2•4 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #1)
downloadDone is apparently invoked when a download succeeds, thus I think it expects the file to be there.
This expectation is natural, but the method is asynchrous and there is no guarantee that the file still exists at that point.
When you say it could be gone, what is removing it, is this a race condition due to asynchronicity? It sounds like one from the original comment.
In the test, this is a race condition (between the downloads implementation writing the ADS and the test removing the file).
Although the test can try to work around this by using internal APIs (such as download.whenSucceeded()), I believe that this should be fixed in the downloads core (because this bug also occurs in non-test scenarios, such as extensions).
|
|
||
Comment 3•4 years ago
|
||
We'd still check that the file exist in an async way though, so while that would reduce the likelihood of this happening, it sounds like it wouldn't completely solve the problem, unless we create the alternative stream sooner (as you suggested). I assume NTFS moves the alternate stream along with the file, if it's moved or renamed, so it wouldn't be so bad.
|
|
Reporter | |
Comment 4•4 years ago
|
||
This comment has become rather large. It starts with a summary of the background of this mark-of-the-web (MOTW) that is written via the ADS.
Then I delve into an analysis of the reliability of the MOTW/ADS in Firefox in relation to its purpose.
In the end I propose an implementation strategy to resolve this bug.
While looking into this, I came across Chrome's implementation, where they explicitly write the ADS after the download/rename, because otherwise AV scanners would scan an incomplete file, at https://bugs.chromium.org/p/chromium/issues/detail?id=127999
In Firefox, that concern is likely not relevant because Firefox does not invoke the IAttachmentExecute interface (details in bug 959187 and its external references, including https://mail.mozilla.org/pipermail/firefox-dev/2013-August/000848.html ), but writes the ADS directly. Still, writing it too early may potentially cause regressions.
In the current implementation, there may be a situation where the ADS (with the Mark-of-the-Web) is not set when the file is renamed before the download has been completed. There is quite some code between renaming the file and adding the MOTW:
- Renaming from temp file to final file, at
_checkReputationAndMovein DownloadCore.jsm (or maybe indownload.unblock). _checkReputationAndMoveis called by.execute()by DownloadCopySaver and DownloadLegacySaversaver.execute(...)is called indownload.startat https://searchfox.org/mozilla-central/rev/851bbbd9d9a38c2785a24c13b6412751be8d3253/toolkit/components/downloads/DownloadCore.jsm#480-483- There is lots of code between that point and the call to
._succeed(), which callsdownloadDone, which in turn is responsible for writing the MoTW as an ADS @ https://searchfox.org/mozilla-central/rev/50cb0892948fb4291b9a6b1b30122100ec7d4ef2/toolkit/components/downloads/DownloadIntegration.jsm#589-598- "Fortunately", the only async operation is a request to read some file metadata at https://searchfox.org/mozilla-central/rev/851bbbd9d9a38c2785a24c13b6412751be8d3253/toolkit/components/downloads/DownloadCore.jsm#480-483,496,567 . That still allows for a small window where someone can inadvertently open a file without the protections from the MoTW.
The implementation/timing of the MoTW/ADS writing is not perfect. Ideally it should be as close as possible as the rename to the final file name (and on the File thread), but I did not spend enough time on looking at the downloads implementation to see if it is safe to do so (e.g. missing one case can result in an absent MoTW; better late than never!). But I digress, this is a pre-existing issue, and certainly not going to be made worse by what I'm going to propose below.
The winAllowLengthBeyondMaxPathWithCaveats option is only used by this MOTW logic, we could update the implementation to confirm the existence of the file before https://searchfox.org/mozilla-central/rev/851bbbd9d9a38c2785a24c13b6412751be8d3253/toolkit/components/osfile/modules/osfile_win_front.jsm#379-388
The window between the file existence check and writing the ADS would be so small, that it is unlikely for the file to be removed in between, and thus bug 1316394 would be fixed by that.
Description
•