Ensure all .part files have a random component
Categories
(Toolkit :: Downloads API, defect, P1)
Tracking
()
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main98-])
Attachments
(2 files, 1 obsolete file)
See bug 1752888 for background.
Without improvements_to_download_panel
enabled, the target of backgroundfilesaver saves goes through the following sequence:
- (on Windows/Linux):
randomletters.ext.part
in a subfolder of TempD
(on macOS):randomletters.ext.part
in the downloads directory - once the user accepts the download for saving or opening:
realfilename.ext.part
in the destination directory (either user-picked or default downloads directory, or temp directory on Windows/Linux for files being opened) - once the download is complete:
realfilename.ext
in the destination directory.
With the improvements_to_download_panel
pref enabled, we automatically move from step 1 to step 2 without user interaction, unless the user has configured that filetype to "always ask", or unless the user has "always ask me where to save files" enabled and the filetype isn't set to open automatically.
This leads to a predictable filename, in most cases in a predictable location, which is a security risk. Even though it is less user-friendly, we should probably have a random part to the filename in step 2.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Assignee | ||
Comment 2•3 years ago
|
||
Depends on D137523
Assignee | ||
Comment 3•3 years ago
•
|
||
After a bunch of conversation with Ryan and Freddy, decided to land this (without landing the tests right now) because the bug only affects non-release channels, deducing a working exploit from just the patch is pretty difficult, and there are in fact a number of unrelated changes in the patch (different rng for the part file random bits, different base64 encoding as well) that would make it more difficult for a would-be commit reader to take advantage. Beta 97 channel will have a fix for this issue sometime in the next 24-48-ish hours once the patch in bug 1753096 lands, and earlier branches are not affected. Landing now seemed preferable to waiting until after soft freeze and then uplifting.
I'll file a follow-up to land the tests once the 98 branch has hit release, and move the commit over.
Autoland link:
https://hg.mozilla.org/integration/autoland/rev/a384c409bd44bb96f02d51a1503de30b59a95dd4
Comment 4•3 years ago
|
||
Comment on attachment 9261679 [details]
Bug 1752996, r?mhowell,mak,mtigley
Revision D137524 was moved to bug 1753221. Setting attachment 9261679 [details] to obsolete.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 5•3 years ago
|
||
Landed: https://hg.mozilla.org/integration/autoland/rev/a384c409bd44bb96f02d51a1503de30b59a95dd4
Backed out for Windows build bustages:
https://hg.mozilla.org/integration/autoland/rev/d841cd13e1f74fa5769446a6afa8bb74574da792
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Yh40A0gFS2uOLjQT548uDA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2fb87cbdc6bc6b8301a1b5d57f1ec574c9351464
Failure log: https://treeherder.mozilla.org/logviewer?job_id=366475980&repo=autoland
[task 2022-02-02T14:57:05.369Z] 14:57:05 INFO - /builds/worker/checkouts/gecko/uriloader/exthandler/nsExternalHelperAppService.cpp(2762,47): error: cannot pass an arithmetic expression of built-in types to 'CheckedInt<unsigned int>'
[task 2022-02-02T14:57:05.369Z] 14:57:05 INFO - CheckedInt<uint16_t> fullPathLength = uint32_t(path.Length()) + 1 +
[task 2022-02-02T14:57:05.369Z] 14:57:05 INFO - ^
[task 2022-02-02T14:57:05.369Z] 14:57:05 INFO - 1 error generated.
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
clang-tidy was sad. I made clang-tidy happy. Hopefully it'll stick this time. :-\
Comment 7•3 years ago
|
||
r=mhowell,dveditz
https://hg.mozilla.org/integration/autoland/rev/587b022bdc841ef60936876f4d13ba5272aefb9f
https://hg.mozilla.org/mozilla-central/rev/587b022bdc84
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
It should be possible to verify this by:
- new profile
- ensure download improvements pref is turned on
- download a large file
- check the filename for the
.part
file that appears in the default downloads directory. It should have a random component before the final set of extensions.
Comment 9•3 years ago
|
||
Hello,
Verified the fix as per the STR from Comment 8 on the latest Nightly (99.0a1/20220213214259) and Beta (98.0b4/20220213185901) under Windows 10 x64 and Ubuntu 16.04 LTS.
While the download of the large file is underway, a .part
file appears in the default downloads directory, having a random string of characters before the final set of extensions, confirming the fix.
In my particular case, I’ve initiated the download of a 1GB zip file named “1GB.zip”. In the default downloads directory a “.part” file can be seen with the following name “1GB.4EajTYFc.zip.part”. The .part file disappears after the file download is complete.
For further details, please see the attached screenshot.
Comment 10•3 years ago
|
||
Updated•3 years ago
|
Updated•2 years ago
|
Description
•