1459264 - Scripts can open infinite file dialogs through calling .click() on file inputs

Status: VERIFIED FIXED

(Firefox :: Security, defect, P3)

Description

[Brian Gomes Bascoy]

Attachment: test.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180327223059

Steps to reproduce:

The attached HTML contains a script that will recursively open file selection dialogs blocking pretty much all kind of user interaction on the current Firefox window (until the OOM killer kills Firefox or something else?).

This was tested on Arch Linux with Gnome 3.


Actual results:

Firefox "hangs".


Expected results:

Firefox doesn't wait till the user selects a file but just keep instantiating file selection dialogs over and over. On Chromium, for instance, this script will be stopped at the 5th dialog.

Comment 1

[Brian Gomes Bascoy]

By the way, this also affects Firefox for Android: after a few seconds, some black screen and flickering, I get a message saying "Android System isn't responding. Do you want to close it?". After touching "OK" Firefox gets closed.

This could be pretty annoying since when you restart Firefox you start in the same exploit page (on my example you need user interacting, but I guess this could be done as soon as the script loads making the app unusable maybe?).

Thanks.

Comment 2

[Johann Hofmann [:johannh]]

Different variations of this have been reported in the past (bug 1332590, bug 682565) but I think we're aiming to solve it all this summer through implementing a permission prompt for multiple downloads in bug 1306334.

We should still keep the different variations open to make sure we test them after that's done...

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

This doesn't seem to be related to download spam, so I'm removing the dependency. It IS an interesting bug though. I'm not sure what the right fix is, here. I'd love to take this bug but I'll probably need mentorship from someone to figure out where to attempt to stop the "recursion".

 - I guess we could just block file inputs from being "activated" while one is already "active" (waiting for the user to pick a file).
 - Is there a legitimate use case for multiple file inputs activating at the same time? Doesn't seem like it.

Comment 4

[Johann Hofmann [:johannh]]

Ah, right, I misread, this is about the file open dialog and not about "Save file". Thanks for correcting that.

You could prevent non-user input using https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/events/EventStateManager.cpp#4143 on opening file inputs, so somewhere about here: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/html/HTMLInputElement.cpp#3938.

I'm not really sure about the web compat implications for this or whether this change is big enough to need an announcement or a pref. Maybe yes, considering that there's stuff like https://developer.mozilla.org/en-US/docs/Web/API/File/Using_files_from_web_applications#Using_hidden_file_input_elements_using_the_click()_method

Smaug probably has thoughts on this.

Comment 5

[Brian Gomes Bascoy]

Try modifying my example by adding input.click() at the end of the script: Firefox will block all those file dialogs because the click event was not generated by the user. Maybe some of that functionality could be used to prevent this kind of "popup bombs".

Comment 6

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

if type=file is click()'ed inside some user input, the file picker is supposed to be opened. Web depends on that behavior. I guess we should just prevent opening a new picker when there is another one open already for that particular browsing context tree. (so, add a flag to the top level docshell that file picker is open?)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: filePicker.patch

Here a different approach than what I suggested for bug 675574:

If we count how many popup blockers we have on stack, we can allow 1 popup for the whole chain. The last one resets the popup counter.

Comment 8

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

Comment on attachment 9030217 [details] [diff] [review]
filePicker.patch

>+/* static */ bool nsContentUtils::CanShowPopupByCount() {
>+  MOZ_ASSERT(sPopupStatePusherCount);
>+
>+  if (sPopupAllowedCount == 0) {
>+    ++sPopupAllowedCount;
>+    return true;
>+  }
>+
>+  return false;
>+}
This API is hard to understand. Why does a method called "CanDoFoo" increase some counter so that two
consecutive method calls return different values? And the method isn't documented at all.
Wouldn't something like
TryUsePopupOpeningToken() or some such hint more what the method actually does?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - patch

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - test

Comment 11

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

Comment on attachment 9030275 [details] [diff] [review]
part 1 - patch

>+/* static */ bool nsContentUtils::TryUsePopupOpeningToken() {
>+  MOZ_ASSERT(sPopupStatePusherCount);
>+
>+  if (sPopupAllowedCount == 0) {
>+    ++sPopupAllowedCount;
>+    return true;
>+  }
>+
>+  return false;
>+}
>+
> static bool JSONCreator(const char16_t* aBuf, uint32_t aLen, void* aData) {
>   nsAString* result = static_cast<nsAString*>(aData);
>   result->Append(static_cast<const char16_t*>(aBuf),
>                  static_cast<uint32_t>(aLen));
>   return true;
> }
> 
> /* static */ bool nsContentUtils::StringifyJSON(
>@@ -10478,8 +10492,20 @@ static bool JSONCreator(const char16_t* 
>         host.Find(".", false, startIndexOfCurrentLevel + 1);
>     if (startIndexOfNextLevel <= 0) {
>       return false;
>     }
>     host = NS_LITERAL_CSTRING("*") +
>            nsDependentCSubstring(host, startIndexOfNextLevel);
>   }
> }
>+
>+/* static */ void nsContentUtils::PopupStatePusherCreated() {
>+  ++sPopupStatePusherCount;
>+}
>+
>+/* static */ void nsContentUtils::PopupStatePusherDestroyed() {
>+  MOZ_ASSERT(sPopupStatePusherCount);
>+
>+  if (!--sPopupStatePusherCount) {
>+    sPopupAllowedCount = 0;
>+  }
>+}

Wouldn't this all be easier to understand if sPopupAllowedCount was a boolean, 
sPopupAllowed. It would be false by default and set to true in 
PopupStatePusherCreated() if ++sPopupStatePusherCount == 1.
It needs to be set back to false in nsContentUtils::PopupStatePusherDestroyed()
if (!--sPopupStatePusherCount) {

nsContentUtils::TryUsePopupOpeningToken()  would then do something like
bool retVal = sPopupAllowed;
sPopupAllowed = false;
return retVal;

with that, r+

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2dc7c0e8c9
Allow just 1 FilePicker per event, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bf214bd2e6
Allow just 1 FilePicker per event - tests, r=smaug

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3c2dc7c0e8c9
https://hg.mozilla.org/mozilla-central/rev/c4bf214bd2e6

Comment 14

[Sajedul Islam]

I have reproduced this bug with Nightly 61.0a1 (2018-05-04) on Windows 10, 64 bit.

This bug's fix is verified with Latest Nightly !

Build ID 20190108101613
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

[bugday-20190102]

Comment 16

[Catalin Sasca, Desktop QA [:csasca]]

Thank you Sajedul for verifying this!

Comment 17

[Brian Gomes Bascoy]

Release Note Request