Download LNK files with a different suffix?
Categories
(Firefox :: File Handling, defect, P2)
Tracking
()
People
(Reporter: mkaply, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-priv-escalation, csectype-spoof, sec-moderate)
We recently made a change so that we no longer LNK filesl, we just download them as is:
https://bugzilla.mozilla.org/show_bug.cgi?id=1466532
so they get saved with a LNK suffix which is technically an executable.
Looking at Chrome, they download these files with an extension of .download.
Should we be renaming these files like Chrome does?
Note there have been LNK issues in the past:
https://krebsonsecurity.com/tag/lnk/
Example:
https://codepen.io/SaschaNaz/pen/bGVojPW
I'm looking into the Chromium source to see if I can find when/why they do the rename.
|
Reporter | |
Comment 1•5 years ago
|
||
The answer appears to be that we should be renaming. In Chrome:
|
|
Reporter | |
Comment 2•5 years ago
|
||
More detail from Chrome
https://bugs.chromium.org/p/chromium/issues/detail?id=181332#c3
|
||
Comment 3•5 years ago
|
||
Yes and from looking at the code and the bug, it seems we want to make a windows-specific function that disarms not only based on the extension but also for whitespace & GUID pattterns.
I'm flagging as sec-moderate because the user has to be tricked to download the file.
|
||
Comment 4•5 years ago
|
||
Fixing this would require updates to both uriloader/exthandler/nsExternalHelperAppService.cpp 's nsExternalAppHandler::nsExternalAppHandler constructor https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/uriloader/exthandler/nsExternalHelperAppService.cpp#1195 and how it determines mSuggestedFileName and/or mTempFileExtension, and to the sanitizer at https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/toolkit/components/downloads/DownloadPaths.jsm#75 . See https://phabricator.services.mozilla.com/D65823 for another example of some of these types of changes.
Mike, are you able to take this?
|
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 5•5 years ago
|
||
Yes.
|
|
Reporter | |
Comment 6•5 years ago
|
||
I'm honestly not sure how to do this without breaking local LNK files (which work fine in Chrome and other browsers).
They all go through the same path.
Honestly, we were better off with broken LNK files from the web.
|
|
||
Comment 7•5 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #6)
I'm honestly not sure how to do this without breaking local LNK files (which work fine in Chrome and other browsers).
They all go through the same path.
I'm confused - presumably we can check whether the LNK file has a file: URL pre-download/use? Can you elaborate on what/where the issue is?
|
|
Reporter | |
Comment 8•5 years ago
|
||
I'm confused - presumably we can check whether the LNK file has a file: URL pre-download/use? Can you elaborate on what/where the issue is?
I was basing this on the links you pointed to. In those places where we do sanitization, we don't have the original URL.
For instance, in the helper app download, we're coming through promptForSaveToFileAsync
which came from nsExternalHelperAppService.cpp
So we'll have to move this all the way up into networking I think, into the http handler (since we don't want this to happen for file URLs)
|
|
||
Comment 9•5 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #8)
I'm confused - presumably we can check whether the LNK file has a file: URL pre-download/use? Can you elaborate on what/where the issue is?
I was basing this on the links you pointed to. In those places where we do sanitization, we don't have the original URL.
For instance, in the helper app download, we're coming through promptForSaveToFileAsync
|
|
||
Comment 10•5 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #6)
I'm honestly not sure how to do this without breaking local LNK files (which work fine in Chrome and other browsers).
They all go through the same path.
In fact, I'm also confused about this - how do local LNK files end up in the downloads path?
|
|
Reporter | |
Comment 11•5 years ago
|
||
I searched through that file and just missed that. Thank you.
In fact, I'm also confused about this - how do local LNK files end up in the downloads path?
Local files go through helper app code the same as remote files...
|
|
||
Comment 12•5 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #11)
In fact, I'm also confused about this - how do local LNK files end up in the downloads path?
Local files go through helper app code the same as remote files...
So how did this work before? I suspect the correct solution is going to be to resolve the lnk well before we get to the "download" point, as a .lnk to a html file should presumably open in the same tab in the browser.
|
||
Comment 13•5 years ago
•
|
||
Honestly, we were better off with broken LNK files from the web.
The previous code implicitly passed any remote LNKs to Windows immediately after clicking a link, and downloaded whatever file if the path is locally available, which might or might not sound scary. Ah no, it just tried passing nonexistent temporary lnk path to Windows and then failed. No scary story.
BTW, I'm not sure why we should only differentiate LNKs when BATs can also run arbitrary codes, could you elaborate more on this?
|
|
||
Comment 14•5 years ago
•
|
||
We can't run .bat files in Firefox, so if you click a link to a .bat file you'll get the download "what do you want to do with this file" dialog. They are also subject to further restrictions because they appear in lists of files that can be executable and/or need to be scanned by safebrowsing ( https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/xpcom/io/nsLocalFileCommon.cpp#43 ) - though so do .lnk.
The difference with .lnk is presumably that for file:/// pages, enterprise has relied on file: links to file: .lnk files actually loading their target (and then either prompting for download or rendering inline in the browser).
|
|
||
Comment 15•5 years ago
|
||
and then either prompting for download or rendering inline in the browser
<a href=foo.lnk> just navigates to the page without a prompt in file:/// pages. Can we rename them only when prompting a download panel?
|
|
Reporter | |
Comment 16•5 years ago
|
||
<a href=foo.lnk> just navigates to the page without a prompt in file:/// pages. Can we rename them only when prompting a download panel?
If a user has their system set to download LNK files without prompting, it should rename as well. We should copy the chrome behavior here.
On Chrome, the user can choose to rename the .lnk.download file to .lnk. But when the file is automatically downloaded or presented to the user in the file dialog, it should already be .lnk.download or something similar.
|
|
||
Comment 17•5 years ago
|
||
(In reply to Kagami :saschanaz from comment #15)
and then either prompting for download or rendering inline in the browser
<a href=foo.lnk>just navigates to the page without a prompt in file:/// pages. Can we rename them only when prompting a download panel?
So to be clear, what I meant is that if you have a link like that in a file:/// page, it should navigate to the target of foo.lnk - and if that is HTML or something else the browser can show inline, that's what happens. Otherwise (e.g. if the lnk points to an executable or whatever), you'll get a download prompt for the target of the lnk file.
When getting lnk files over http, we should rename them and do everything Mike is talking about. But I don't think we should do that for local file .lnk files.
|
|
Reporter | |
Comment 18•4 years ago
|
||
I thought I could take this, but I don't have bandwidth for this.
|
||
Updated•3 years ago
|
|
|
||
Comment 19•2 years ago
|
||
given CVE-2022-36314 I think this is now fixed? Mike, can you confirm?
|
|
Reporter | |
Comment 20•2 years ago
|
||
Yes, this is fixed. Sorry it took me so long to confirm.
LNKs are downloaded as foo.lnk.download
|
||
Updated•2 years ago
|
|
|
||
Updated•1 year ago
|
Description
•