webkitdirectory could be used to trick users into allowing access to arbitrary folders

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Abdulrahman Alqabandi, Assigned: smaug)

Tracking

(4 keywords)

51 Branch
mozilla51
csectype-disclosure, csectype-spoof, regression, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50+ fixed, firefox51+ fixed)

Details

(Whiteboard: [post-critsmash-triage])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8781914 [details]
PoC file

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

Check attached PoC.

Works only on nightly builds. This was tested on 51.0a1 (2016-08-16)


Actual results:

The folder upload dialogue is not descriptive enough, which could lead to some users being confused what it does exactly.

On Windows 8.1:
Title of folder upload is 'File upload' and the confirmation button is 'Select Folder', nothing that will indicate multiple files will be uploaded, or that a folder will be uploaded.

On Ubuntu:
Title of the dialogue is 'File upload' and the confirmation button is 'Open', again, nothing that indicates you're uploading an entire folder and its files.


Expected results:

The title should be changed to 'Folder upload' and/or the confirmation button should be changed to 'Upload folder'

Updated

2 years ago
Blocks: 1288683
Group: firefox-core-security → core-security
Component: Untriaged → DOM
Product: Firefox → Core
Created attachment 8781920 [details]
test which works in Chrome too

Just a small change to the test to make it work in Chrome too.
(Reporter)

Comment 2

2 years ago
(In reply to Olli Pettay [:smaug] from comment #1)
> Created attachment 8781920 [details]
> test which works in Chrome too
> 
> Just a small change to the test to make it work in Chrome too.

On chrome it does indicate you are uploading a folder. It says 'Select folder to upload' this is on Windows OS, is something else showing for other OS?
(In reply to Abdulrahman Alqabandi[test] from comment #0)

> On Windows 8.1:
> Title of folder upload is 'File upload' and the confirmation button is
> 'Select Folder', nothing that will indicate multiple files will be uploaded,
> or that a folder will be uploaded.
How is File _upload_ and Select _folder_ not indicating multiple files will be uploaded?


> Expected results:
> 
> The title should be changed to 'Folder upload' and/or the confirmation
> button should be changed to 'Upload folder'

Yeah, this could be improved. Looks like Chrome uses "Upload" as the button on linux, which isn't any better, but
it has "Select folder to upload" as the title.

(not that anyone ever reads the title of filepicker dialog)
Assignee: nobody → bugs
(Reporter)

Comment 5

2 years ago
(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Abdulrahman Alqabandi[test] from comment #0)
> 
> > On Windows 8.1:
> > Title of folder upload is 'File upload' and the confirmation button is
> > 'Select Folder', nothing that will indicate multiple files will be uploaded,
> > or that a folder will be uploaded.
> How is File _upload_ and Select _folder_ not indicating multiple files will
> be uploaded?
> 
> 
> > Expected results:
> > 
> > The title should be changed to 'Folder upload' and/or the confirmation
> > button should be changed to 'Upload folder'
> 
> Yeah, this could be improved. Looks like Chrome uses "Upload" as the button
> on linux, which isn't any better, but
> it has "Select folder to upload" as the title.
> 
> (not that anyone ever reads the title of filepicker dialog)

Assumption is that non advanced users can be fooled if it does not strictly indicate a folder is being uploaded.
Created attachment 8781935 [details] [diff] [review]
patch

Change the title to "Select Folder to Upload".

We're a bit inconsistent with using capital letters in titles, so I ended up using what Chrome has.
Attachment #8781935 - Flags: review?(amarchesini)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8781935 - Flags: review?(amarchesini) → review+
We may want to tweak also the button label, especially because OSX doesn't seem to show any title, I was told.

For gtk, http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/widget/gtk/nsFilePicker.cpp#386

If I read some documentation right, getting NSOpenPanel* op and then [op setPrompt:@"<some value>"];
should work there. Is that right mstange?

On Windows we could use SetOkButtonLabel for IFileDialog (Vista+), but it is unclear what to do in XP.
jimm?
Though, the title is quite clear, so perhaps not dealing with XP is fine.
Flags: needinfo?(mstange)
Flags: needinfo?(jmathies)
(Reporter)

Comment 9

2 years ago
Since you are working on this, do you mind checking out Bug 1295922, if its not too much.
(In reply to Olli Pettay [:smaug] from comment #8)
> If I read some documentation right, getting NSOpenPanel* op and then [op
> setPrompt:@"<some value>"];
> should work there. Is that right mstange?

That is correct. See also the discussion in bug 1207114.
Flags: needinfo?(mstange)
Created attachment 8782120 [details] [diff] [review]
Use "Upload" as button label
Created attachment 8782123 [details] [diff] [review]
Use "Upload" as button label for directory picking

This should match what Chrome does. At least on linux is uses default button labels for file picking, but 'Upload' for directories.
Attachment #8782120 - Attachment is obsolete: true
Created attachment 8782139 [details] [diff] [review]
Use "Upload" as button label for directory picking
Attachment #8782123 - Attachment is obsolete: true
dveditz, are you ok with the approach I'm proposing here:
When directory picking is used, use "Select Folder to Upload" as the title of the filepicker where possible and use "Upload" and the button label where possible.

I was told OSX may not have the title at all at least in some versions, and changing the button label in WindowsXP isn't apparently quite trivial.

That is close to what Chrome does.
Flags: needinfo?(dveditz)
Created attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking
Attachment #8782139 - Attachment is obsolete: true
Wow, the usability here is pretty horrible. On OS X (10.11) there's no titlebar and it's not at all clear you're picking a whole directory to be uploaded. I'm not sure changing the positive button from "Open" to "Upload" (as in Chrome) gets that across. It's not any better in Chrome or Safari (though this example doesn't fully work in Safari -- the "Choose" button stays disabled).

This will be new and unexpected functionality for most users which makes it even easier to abuse. It might even be powerful enough to be worth a second confirmation dialog ("you have chosen to upload the following files... OK/Cancel"). Even if the user knows they're picking a folder for upload they might appreciate the chance to double-check the folder only contained the files they thought it did. Also, in testing at one point I double-clicked a folder thinking I was descending into it (to pick a sub-folder) and instead it submitted the contents--oops!

(In reply to Olli Pettay [:smaug] from comment #14)
> dveditz, are you ok with the approach I'm proposing here:
> When directory picking is used, use "Select Folder to Upload" as the title
> of the filepicker where possible and use "Upload" and the button label where
> possible.

That's better than what we have now, but I'm still unhappy with this functionality (in general, in all the browsers I've tested).
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #16)
> Wow, the usability here is pretty horrible. On OS X (10.11) there's no
> titlebar and it's not at all clear you're picking a whole directory to be
> uploaded. I'm not sure changing the positive button from "Open" to "Upload"
> (as in Chrome) gets that across. It's not any better in Chrome or Safari
> (though this example doesn't fully work in Safari -- the "Choose" button
> stays disabled).
Safari doesn't support webkitdirectory.

I should test Edge too since it does support webkitdirectory.



> This will be new and unexpected functionality for most users which makes it
> even easier to abuse.
Well, for Firefox users, but Chrome has had webkitdirectory for ages, and also Edge for a year or so.


> It might even be powerful enough to be worth a second
> confirmation dialog ("you have chosen to upload the following files...
> OK/Cancel"). 
I can see point here, but second confirmation dialog would be also rather ux hostile, IMO
I guess we would need totally new directory picker showing what all will be uploaded.

> 
> That's better than what we have now, but I'm still unhappy with this
> functionality (in general, in all the browsers I've tested).
dveditz, are you worried enough about this, meaning that we would postpone releasing this API for couple of Firefox releases to get some more complicated, yet safer UI, or do you think we can release with similar to Chrome's UI for now?
Flags: needinfo?(dveditz)
(Reporter)

Comment 18

2 years ago
On Edge both the title and the confirmation button are 'Select Folder'
Flags: needinfo?(dveditz)
Keywords: csectype-disclosure, csectype-spoof, sec-moderate
Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

baku for dom and ipc
mstange for OSX
jimm for Windows
karlt for Gtk
Attachment #8782155 - Flags: review?(mstange)
Attachment #8782155 - Flags: review?(karlt)
Attachment #8782155 - Flags: review?(jmathies)
Attachment #8782155 - Flags: review?(amarchesini)
Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

Review of attachment 8782155 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on nsFilePicker.mm
Attachment #8782155 - Flags: review?(mstange) → review+
Group: core-security → dom-core-security
Flags: sec-bounty?
Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

Review of attachment 8782155 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIFilePicker.idl
@@ +189,5 @@
> +   * If set to non-empty string, the nsIFilePicker implementation
> +   * may use okButtonLabel as the label for the button the user uses to accept
> +   * file selection.
> +   */
> +  attribute AString okButtonLabel;

don't you have to change the mockFilePicker as well?
Attachment #8782155 - Flags: review?(amarchesini) → review+
Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

>+  const gchar* accept_button = nullptr;

Please remove the unused "= nullptr", to clarify that accept_button is never null.

>+  nsXPIDLCString buttonLabel;
>+  if (!mOkButtonLabel.IsEmpty()) {
>+    buttonLabel.Adopt(ToNewUTF8String(mOkButtonLabel));
>+    accept_button = buttonLabel.get();

nsAutoCString supports .get(), so please use this instead of the unnecessary allocation from nsXPIDLCString/ToNewUTF8String().
Either declare buttonLabel as NS_ConvertUTF16toUTF8 or use Append/CopyUTF16toUTF8().
Attachment #8782155 - Flags: review?(karlt) → review+
ok. I was just using the same style as the other parts of the method.
(In reply to Andrea Marchesini [:baku] from comment #21)
> ::: widget/nsIFilePicker.idl
> @@ +189,5 @@
> > +   * If set to non-empty string, the nsIFilePicker implementation
> > +   * may use okButtonLabel as the label for the button the user uses to accept
> > +   * file selection.
> > +   */
> > +  attribute AString okButtonLabel;
> 
> don't you have to change the mockFilePicker as well?
I shouldn't need to, since it is implemented in JS and wouldn't do anything with the okButtonLabel.
And the return value of SetOkButtonLabel is ignored.

But I'll test.

Comment 25

2 years ago
(In reply to Olli Pettay [:smaug] from comment #8)
> On Windows we could use SetOkButtonLabel for IFileDialog (Vista+), but it is
> unclear what to do in XP.
> jimm?
> Though, the title is quite clear, so perhaps not dealing with XP is fine.

If you want to try this it can be done via the BrowseCallbackProc [1], when the init event is received in the callback we could set the text of the dialog [2].

I don't think it's worth the effort though. :)

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb762598(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms633546(v=vs.85).aspx
Flags: needinfo?(jmathies)

Updated

2 years ago
Attachment #8782155 - Flags: review?(jmathies) → review+
Flags: sec-bounty? → sec-bounty+
Created attachment 8783650 [details] [diff] [review]
file_picker_button_3.diff

Approval Request Comment
[Feature/regressing bug #]: bug 1288683 
[User impact if declined]: less clear title and button in filepickers
[Describe test coverage new/current, TreeHerder]: Manually tested, since we use OS provided filepickers
[Risks and why]: Should be safe
[String/UUID change made/needed]: new strings added
Attachment #8783650 - Flags: sec-approval?
Attachment #8783650 - Flags: approval-mozilla-aurora?
Comment on attachment 8781935 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1288683 
[User impact if declined]: less clear title and button in filepickers
[Describe test coverage new/current, TreeHerder]: Manually tested, since we use OS provided filepickers
[Risks and why]: Should be safe
[String/UUID change made/needed]: new strings added
Attachment #8781935 - Flags: sec-approval?
Attachment #8781935 - Flags: approval-mozilla-aurora?
Attachment #8782155 - Attachment is obsolete: true

Comment 28

2 years ago
As a sec-moderate, this doesn't need sec-approval to land. Clearing and adding Aurora approval.

How far back does this issue go? The sec-approval template questions weren't answered.
status-firefox48: --- → ?
status-firefox49: --- → ?
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → ?
tracking-firefox50: --- → +
tracking-firefox51: --- → +

Updated

2 years ago
Attachment #8781935 - Flags: sec-approval?
Attachment #8781935 - Flags: sec-approval+
Attachment #8781935 - Flags: approval-mozilla-aurora?
Attachment #8781935 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Attachment #8783650 - Flags: sec-approval?
Attachment #8783650 - Flags: sec-approval+
Attachment #8783650 - Flags: approval-mozilla-aurora?
Attachment #8783650 - Flags: approval-mozilla-aurora+
This is FF51/FF50 issue only, as the the approval question said, by referring to bug 1288683. Sorry if it wasn't clear.
status-firefox48: ? → unaffected
status-firefox49: ? → unaffected
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #27)
> [String/UUID change made/needed]: new strings added

Doesn't this require someone from the l10n team to sign off?
Flags: needinfo?(francesco.lodolo)
It's actually up to release drivers to decide when evaluating the approval request (but thanks for asking).

Personally I'm not happy to see this pushed to Aurora this late in the cycle, there's a good chance of not having it translated for several languages. It comes down to evaluating how bad the current situation is, and how confusing it would be to have this string in English. If we really need to, the sooner it lands the better.
Flags: needinfo?(francesco.lodolo)
Any thoughts, Sylvestre/Ritu?
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)

Updated

2 years ago
Depends on: 1297977
https://hg.mozilla.org/mozilla-central/rev/ba3d5deb3478
https://hg.mozilla.org/mozilla-central/rev/034773aa3bec
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to Francesco Lodolo [:flod] from comment #32)
> It's actually up to release drivers to decide when evaluating the approval
> request (but thanks for asking).
> 
> Personally I'm not happy to see this pushed to Aurora this late in the
> cycle, there's a good chance of not having it translated for several
> languages. It comes down to evaluating how bad the current situation is, and
> how confusing it would be to have this string in English. If we really need
> to, the sooner it lands the better.

Hi Al, Flod, Olli, in general, I agree that we do not want to break string freeze rules for Aurora/Beta cycle. I think the only potentially un-localized string in this case is "Select Folder to Upload". Right? Is there a string like this that might already be localized and used elsewhere that we can re-use in this case? Flod? Olli?
Flags: needinfo?(rkothari)
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(bugs)
Flags: needinfo?(abillings)

Comment 36

2 years ago
I don't know anything about string freezes. What I *do* know is that if we take this bug fix on Aurora, we'll never ship this security problem. That's what we'd prefer to do and that's my argument for taking it on Aurora.
Flags: needinfo?(abillings)
(In reply to Ritu Kothari (:ritu) from comment #35)
> (In reply to Francesco Lodolo [:flod] from comment #32)
> > It's actually up to release drivers to decide when evaluating the approval
> > request (but thanks for asking).
> > 
> > Personally I'm not happy to see this pushed to Aurora this late in the
> > cycle, there's a good chance of not having it translated for several
> > languages. It comes down to evaluating how bad the current situation is, and
> > how confusing it would be to have this string in English. If we really need
> > to, the sooner it lands the better.
> 
> Hi Al, Flod, Olli, in general, I agree that we do not want to break string
> freeze rules for Aurora/Beta cycle. I think the only potentially
> un-localized string in this case is "Select Folder to Upload". Right? Is
> there a string like this that might already be localized and used elsewhere
> that we can re-use in this case? Flod? Olli?

There are two strings, one in both patches.

"Select Folder to Upload" and
"Upload"


Couldn't find "Select Folder to Upload" to be used earlier for anything.
Flags: needinfo?(bugs)
We don't have any of the two strings. As said, given that everyone wants to land this, let's do it quickly.
How much can be disclosed of the reason for uplifting the strings?
Flags: needinfo?(francesco.lodolo)
I'm not going to speak for Al, but it doesn't seem like we need to be overly coy about the reason for the late string change since all affected releases will be fixed once tomorrow's Aurora nightlies go out.

https://hg.mozilla.org/releases/mozilla-aurora/rev/433caaa37ccf
https://hg.mozilla.org/releases/mozilla-aurora/rev/e58e63e362ff
status-firefox50: affected → fixed
status-firefox-esr45: ? → unaffected
Flags: needinfo?(sledru)
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #37)
> 
> There are two strings, one in both patches.
> 
> "Select Folder to Upload" and
> "Upload"
> 
> 
> Couldn't find "Select Folder to Upload" to be used earlier for anything.

Ok. Thanks! I am ok with these strings shipping in English when 50 goes to release.

Comment 41

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> I'm not going to speak for Al, but it doesn't seem like we need to be overly
> coy about the reason for the late string change since all affected releases
> will be fixed once tomorrow's Aurora nightlies go out.

An excellent point.
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.