291640 - (CVE-2023-37203) Drag and drop can create shortcuts to local files

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Paul Nickerson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

In the page javascript:'Drag this broken image to the desktop:<img
src="C:/windows/system32/cmd.exe">', dragging the broken image to the desktop
will create a shortcut to cmd.exe, which, when opened, will spawn cmd.exe

Reproducible: Always

Steps to Reproduce:
1.javascript:'Drag this broken image to the desktop:<img
src="C:/windows/system32/cmd.exe">'
2. drag broken image to desktop
3. double click new icon on desktop
Actual Results:  
cmd.exe opened

Expected Results:  
executable files shouldnt be draggable

Comment 1

[timeless]

ok, this one is real, and i can make it more dangerous pre sp2.

however, it's all limited to the 1.0 branch, i had someone test for me on trunk
and none of the cases i provided worked.

Comment 2

[(not currently active) Ted Mielczarek]

Confirming on 1.0.3.  In XPSP2, dragging to the desktop prompts me to add it as
an Active Desktop item, but dragging it to a folder creates a .url shortcut,
which will launch the application when double clicked.  In SP2 this has a
security prompt before it actually launches.

Comment 3

[Daniel Veditz [:dveditz]]

Is there a way to influence the name of the link? If it looks like <blah>cmd.exe
it's kind of on the user if they click it. You're not dragging the exe itself,
just a shortcut. Is there a way to pass parameters? That would certainly make
this more dangerous.

Comment 4

[timeless]

yes there is. i'll explain elsewhere.

Comment 5

[Jay Patel [:jay]]

Not going to make the 1.0.5 train, but need to look into this for future 1.0.x
release.

Comment 6

[Jay Patel [:jay]]

-'d per drivers meeting

Comment 7

[Chris Beard]

would be nice to fix, not critical for aviary1.5 (dveditz)

/cb

Comment 8

[Jesse Ruderman]

Did the patch in bug 285438 take care of this bug?

Comment 9

[Josh Aas]

Is this still an issue? Does it need to remain hidden?

Comment 10

[Josh Aas]

I just tested this using Windows 7 and the latest trunk nightly and this appears to still be a problem. Dragging the test icon to the desktop creates a shortcut to the executable that will execute without any prompts.

I don't know enough about security expectations with this sort of thing to know how big of a problem this is.

Comment 11

[Benjamin Smedberg]

This is only really a problem in combination with file-planting attacks or other things. But in any case we should fix it, probably doing a checkLoadURIWithPrincipal to see whether the website is allowed to link to that thing.

Comment 12

[Josh Aas]

Neil - can you look into this?

Comment 13

[Neil Deakin]

Attachment: patch

This patch does the basic implementation. There are two issues I am not sure how to deal with:

 - the test displays a 'grant privileges dialog'.
 - test testcase in the first comment uses a file path not a url.

But this patch is a suitable starting point.

Comment 14

[Neil Deakin]

Attachment: Patch version 2, fix up test

Comment 15

[Neil Deakin]

Comment on attachment 612661 [details] [diff] [review]
Patch version 2, fix up test

Not sure entirely about this patch as it doesn't fix the reported issue. Wondering if it would be better to have the platform-specific widget layer reject creating links to local files somehow.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Why would it have to be platform-specific?
Couldn't we whitelist the allowed schemes, or at least blacklist javascript: ?

Comment 17

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #16)
> Why would it have to be platform-specific?
> Couldn't we whitelist the allowed schemes, or at least blacklist javascript:
> ?

'javascript:' isn't used here.

The bug here is in preventing:
  event.dataTransfer.setData("text/x-moz-url-data", "c:/builds/");
which isn't actually a URL and Windows translates it in some manner when creating a shortcut.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 612661 [details] [diff] [review]
Patch version 2, fix up test

Ah, ok. Sounds like widget layer fix is needed.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 21

[:Gijs (he/him)]

Attachment: Bug 291640, r?emilio

Comment 22

[:Gijs (he/him)]

Filed bug 1832266 for tests.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=emilio
https://hg.mozilla.org/integration/autoland/rev/c8f66e4894442df742c8b2b29be26a2937c193be
https://hg.mozilla.org/mozilla-central/rev/c8f66e489444

Comment 24

[:Gijs (he/him)]

This fix works for file: but appears to be incomplete for c:/ links.

Comment 25

[:Gijs (he/him)]

Hm, so the reason I didn't catch this originally is that it seems that we are more strict than Windows in how we parse Windows paths into URLs. Specifically, a link that points to: C:\Users gets transformed to file:///C:/Users (and blocked, on current nightly already) but a link to C:/Users does not. This causes problems when we then pass this information to Windows and it does then interpret the target as a file path.

The simplest fix is to change the URI fixup parser to also recognize this syntax as a file URI, here. It's worth noting that just setting attemptFixup to true in that case is insufficient: our Windows path parsing code will fail to initialize the file when the path is made up of forward slashes instead of backslashes.

I have a patch for this. It passes urlbar and docshell urifixup unit tests without changes... in principle it shouldn't end up impacting any web content because we'd primarily use nsIURI for security checks and navigation, rather than fixup. I think fixing up C:/Users/ in the URL bar to file:///C:/Users/ (instead of a search query with the default search provider) is probably a net win, but if we think that may be in doubt we could put it behind a fixup flag and/or pref.

Comment 26

[:Gijs (he/him)]

Attachment: Bug 291640, r?mak

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=mak
https://hg.mozilla.org/integration/autoland/rev/d5620d8eb2090ec5a6024d64684b005429066c5c
https://hg.mozilla.org/mozilla-central/rev/d5620d8eb209

Comment 28

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 29

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt