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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: qab, Assigned: smaug)

Tracking

(4 keywords)

51 Branch
mozilla51
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

3 years ago
Posted file 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

3 years ago
Blocks: 1288683
Group: firefox-core-security → core-security
Component: Untriaged → DOM
Product: Firefox → Core
Assignee

Comment 1

3 years ago
Just a small change to the test to make it work in Chrome too.
Reporter

Comment 2

3 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?
Assignee

Comment 3

3 years ago
(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

Updated

3 years ago
Assignee: nobody → bugs
Reporter

Comment 5

3 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.
Assignee

Comment 7

3 years ago
Posted patch patchSplinter Review
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)
Assignee

Updated

3 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8781935 - Flags: review?(amarchesini) → review+
Assignee

Comment 8

3 years ago
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

3 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)
Assignee

Comment 11

3 years ago
Posted patch Use "Upload" as button label (obsolete) — Splinter Review
Assignee

Comment 12

3 years ago
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
Assignee

Comment 13

3 years ago
Attachment #8782123 - Attachment is obsolete: true
Assignee

Comment 14

3 years ago
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)
Assignee

Comment 15

3 years ago
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)
Assignee

Comment 17

3 years ago
(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

3 years ago
On Edge both the title and the confirmation button are 'Select Folder'
Flags: needinfo?(dveditz)
Assignee

Comment 19

3 years ago
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+
Assignee

Comment 23

3 years ago
ok. I was just using the same style as the other parts of the method.
Assignee

Comment 24

3 years ago
(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.
(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

3 years ago
Attachment #8782155 - Flags: review?(jmathies) → review+
Flags: sec-bounty? → sec-bounty+
Assignee

Comment 26

3 years ago
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?
Assignee

Comment 27

3 years ago
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?
Assignee

Updated

3 years ago
Attachment #8782155 - Attachment is obsolete: true
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.
Attachment #8781935 - Flags: sec-approval?
Attachment #8781935 - Flags: sec-approval+
Attachment #8781935 - Flags: approval-mozilla-aurora?
Attachment #8781935 - Flags: approval-mozilla-aurora+
Attachment #8783650 - Flags: sec-approval?
Attachment #8783650 - Flags: sec-approval+
Attachment #8783650 - Flags: approval-mozilla-aurora?
Attachment #8783650 - Flags: approval-mozilla-aurora+
Assignee

Comment 29

3 years ago
This is FF51/FF50 issue only, as the the approval question said, by referring to bug 1288683. Sorry if it wasn't clear.
(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

3 years ago
Depends on: 1297977
https://hg.mozilla.org/mozilla-central/rev/ba3d5deb3478
https://hg.mozilla.org/mozilla-central/rev/034773aa3bec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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)
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)
Assignee

Comment 37

3 years ago
(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
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.
(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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.