Closed Bug 1752996 Opened 3 years ago Closed 3 years ago

Ensure all .part files have a random component

Categories

(Toolkit :: Downloads API, defect, P1)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
98 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox96 --- wontfix
firefox97 - wontfix
firefox98 blocking verified
firefox99 --- verified

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:

  1. (on Windows/Linux): randomletters.ext.part in a subfolder of TempD
    (on macOS): randomletters.ext.part in the downloads directory
  2. 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)
  3. 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: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attached file Bug 1752996, r?mhowell,mak,mtigley (obsolete) —

Depends on D137523

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

Blocks: 1753221

Comment on attachment 9261679 [details]
Bug 1752996, r?mhowell,mak,mtigley

Revision D137524 was moved to bug 1753221. Setting attachment 9261679 [details] to obsolete.

Attachment #9261679 - Attachment is obsolete: true
Severity: -- → S1
Priority: -- → P1

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.
Flags: needinfo?(gijskruitbosch+bugs)

clang-tidy was sad. I made clang-tidy happy. Hopefully it'll stick this time. :-\

Flags: needinfo?(gijskruitbosch+bugs)
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify+

It should be possible to verify this by:

  1. new profile
  2. ensure download improvements pref is turned on
  3. download a large file
  4. 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.

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.

Status: RESOLVED → VERIFIED
Attached image 2022-02-14_08h53_38.png
See Also: → 1755566
Whiteboard: [adv-main98-]
Group: core-security-release
Regressions: 1910260
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: