1226977 - (CVE-2016-5266) Outgoing dataTransfer items are not filtered

Status: RESOLVED FIXED

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

Description

[Rafael Gieschke]

Outgoing dataTransfer items are not filtered

1. Open https://ebenda.org/2015/drag-drop/ in Firefox and Chrome
2. Drag and drop the displayed Firefox icon to Chrome
3. See the contents of your root directory/C: drive, the web page has read access to any file on your system

Windows convention seems to be to treat FileNameW, FileName and CF_HDROP items as (trusted) filenames (as done by Chromium in https://chromium.googlesource.com/chromium/src.git/+/master/ui/base/clipboard/clipboard_util_win.cc#179 ).

X11 convention seems to be (used by GNOME Files and by Chromium in https://chromium.googlesource.com/chromium/src.git/+/master/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc#306 ) to treat file URIs in "text/uri-list" as (trusted) filenames.

Thus, dataTransfer items have to be filtered for (at least) these types and not passed through directly to the system.

Chromium does not use generic items but puts most generic items in a combined "chromium/x-web-custom-data" item. Additionally, it seems to not use "text/uri-list" at all for URIs but sets "text/x-moz-url" and "_NETSCAPE_URL". Even these values are filtered for file URIs (if not set by pages accessed via file:// themselves): https://chromium.googlesource.com/chromium/src.git/+/master/content/browser/renderer_host/render_view_host_impl.cc#1118 .

Comment 1

[Rafael Gieschke]

This is now also https://code.google.com/p/chromium/issues/detail?id=580005 as it affects Chrome users. A solution in Chrome without cooperation from Mozilla seems to be very hard, though.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Matt, can you or Kamil reproduce this?

Comment 3

[Neil Deakin]

On Windows, part of this would be covered by bugs 860857 and 585810.

On all platforms, we should filter out urls such as 'file' in setData to disallow adding them.

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

On a VM of Windows 8, I was able to see these results:

Well done! Oh, and this is your root directory:

/C_drive/$Recycle.Bin
/C_drive/Boot
/C_drive/bootmgr
/C_drive/BOOTNXT
/C_drive/BOOTSECT.BAK
/C_drive/Documents and Settings
/C_drive/pagefile.sys
/C_drive/PerfLogs
/C_drive/Program Files
/C_drive/Program Files (x86)
/C_drive/ProgramData
/C_drive/Recovery
/C_drive/swapfile.sys
/C_drive/System Volume Information
/C_drive/Users
/C_drive/Windows


On a VM of Ubuntu 14.04.3, I see this:

Well done! Oh, and this is your root directory:

/<root>/bin
/<root>/etc
/<root>/lib
/<root>/media
/<root>/cdrom
/<root>/boot
/<root>/sys
/<root>/root
/<root>/srv
/<root>/run
/<root>/mnt
/<root>/lost+found
/<root>/home
/<root>/var
/<root>/lib64
/<root>/dev
/<root>/usr
/<root>/sbin

Comment 5

[Rafael Gieschke]

Thanks for looking at this.

I, though, do not see how this is not sec-high but sec-moderate. It clearly fulfils "Theft of arbitrary files from local system". For being sec-moderate, drag and drop from Firefox to Chrome would then have to be a complex or unusual action. Drag and drop is surely not complex. To help the user open the web page in Chrome, you could even do something (under Windows) like "ev.dataTransfer.setData('text/x-moz-url', '...');" (used in <https://ebenda.org/2015/drag-drop/index2.html>), so you can drag and drop the image one time for opening the web page in Chrome and a second time for getting the file access.

It is not unusual either considering people have pasted JavaScript code into Web Console and Scratchpad or saved web pages and opened them locally in the past. In contrast, dragging and dropping is a usual action you easily can make people do. Apart from the game-like PoC, another possible way would be a web app telling the user to switch to Chrome for a feature they want to use, offering the possibility to keep existing data without having to upload it to a server (shown in <https://ebenda.org/2015/drag-drop/index2.html>). Not even an advanced user would think that this could be a dangerous or unusual action; in fact, one whole use case of drag and drop in HTML5 (not possible before) is to enable interaction between web and native apps.

The same goes for bug 1226979.

Comment 6

[Rafael Gieschke]

Just one more way to exploit this under Linux (and GNOME): by using "ev.dataTransfer.effectAllowed = 'move'" and making the user drag and drop the image from Firefox to Nautilus (Files) (a feature found in Chrome and used by Gmail as "dragging out files" ("DownloadURL"; <http://www.thecssninja.com/html5/gmail-dragout>)), you can make the user move files/directories away from KNOWN locations unconsciously (I currently do not see that you could expand that to uknown locations, though). This could make the system potentially unbootable (moving /bin away in case the user has write access to the origin location; see <https://ebenda.org/2015/drag-drop/index3.html>) or lock the user out of their data after the next reboot (if the username is known by moving /home/.ecryptfs/$USER/.ecryptfs/; unless "Show Hidden Files" is active, the user will not even see that a directory was moved to the selected drop location).

Comment 7

[Rafael Gieschke]

(In reply to Neil Deakin from comment #3)
> On Windows, part of this would be covered by bugs 860857 and 585810.

Yes, using a whitelist for native item formats (like Chrome does) and putting everything else in a catch-all custom format would fix this. Aligning it to Chrome would be great of course, as it would still allow for a "drag and drop your work over to Chrome"-web app as outlined above.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

(In reply to Rafael Gieschke from comment #5)
> Thanks for looking at this.
> 
> I, though, do not see how this is not sec-high but sec-moderate. It clearly
> fulfils "Theft of arbitrary files from local system". For being
> sec-moderate, drag and drop from Firefox to Chrome would then have to be a
> complex or unusual action. Drag and drop is surely not complex.

It isn't a drive by exploit triggered by the web. The local user has to be convinced to do this to themselves with drag and drop *and* they have to do it between two different web browsers. The difficulty in convincing people to do something like this and the likelihood of this happening is factored into security ratings. Realistically, most people are not going to drag and drop locally just because a website suggests that they do so.

Comment 9

[Rafael Gieschke]

(In reply to Al Billings [:abillings] from comment #8)
> It isn't a drive by exploit triggered by the web. The local user has to be
> convinced to do this to themselves with drag and drop *and* they have to do
> it between two different web browsers. The difficulty in convincing people
> to do something like this and the likelihood of this happening is factored
> into security ratings. Realistically, most people are not going to drag and
> drop locally just because a website suggests that they do so.

Thanks for these explanations. There are counterexamples (e.g., requiring installation of a web app, <https://www.mozilla.org/en-US/security/advisories/mfsa2014-82/>; requiring autocomplete interaction, <https://www.mozilla.org/en-US/security/advisories/mfsa2015-24/>) but I do see your point.

Comment 10

[Rafael Gieschke]

Updating comment #6, this can be turned into a permanent DoS attack on the user's data by combination with another potential bug:

0. Use Ubuntu (or Ubuntu GNOME) 15.10 with an encrypted home directory (encrypting your home directory is an option prominently shown during installation the user can activate by clicking a check box, <https://help.ubuntu.com/community/EncryptedHome>).
1. Visit <https://ebenda.org/2015/drag-drop/destroy.html> in Firefox.
2. Drag and drop a file from the default file manager (Files/Nautilus) to the web page.
3. Drag and drop the displayed file icon back to the file manager. You will not see any effect, probably thinking the action has not worked.
4. Restart your system.
5. You will no longer be able to log in using the used account.

Step 2 will make available the full path of the dropped file to the web page via dataTransfer.getData("text/plain") (this part also works under Windows, via dataTransfer.getData("text/x-moz-url"), try it at <https://ebenda.org/2015/drag-drop/display.html>). I am not sure if this is intentional, for the input element it was considered a security bug (<https://www.mozilla.org/en-US/security/advisories/mfsa2013-43/>, the step from the file path to "Disclosure of OS username" is quite easy :-)). If it is unintentional, I can open another bug if needed (I did not find any open bug on this).

In the case of Linux/X11, Nautilus really sends "text/plain" (together with ['x-special/gnome-icon-list', 'text/uri-list', 'UTF8_STRING', 'COMPOUND_TEXT', 'TEXT', 'STRING', 'text/plain;charset=utf-8', 'text/plain']) containing the file URL (it does not send "text/x-moz-url", though, which is present in dataTransfer in Linux/X11, too); in the case of Windows, Windows Explorer does not send such ("text/x-moz-url") a type, so it must have been synthesized by Firefox. Chrome does not expose the full path of dropped files to web pages in either case and I guess Firefox should not do so either.

Step 3 constructs a file:///home/.ecryptfs/$USER/.ecryptfs/ URL and makes Nautilus move the directory (instead of copying it) via dataTransfer.effectAllowed = "move". As hidden files are not shown by default, no immediate effect will be visible to the user. The home directory's encryption is contained within .ecrpytfs, so the user's complete data will be permanently lost if the user has not backed up the key. If they have, it will at least be a major hassle to regain the data.

Comment 11

[Rafael Gieschke]

Okay, two more bugs :-). (Tell me if these should be filed as separate bugs.)

Comment 12

[Rafael Gieschke]

DoS attack on local system

0. Use Windows.
1. Visit <https://ebenda.org/2015/drag-drop/flood.html>.
2. Drag the Firefox image. You might face the unresponsive-script dialog and have to do this several times.
3. All sorts of strange things will happen with your system, you have to reboot.

I think this boils down to exhausting all available RegisterClipboardFormat formats ("Registered clipboard formats are identified by values in the range 0xC000 through 0xFFFF.", <https://msdn.microsoft.com/en-us/library/windows/desktop/ms649049%28v=vs.85%29.aspx>). This namespace is shared by drag and drop and the clipboard and possibly other things. As a consequence, drag and drop sometimes stops to work at all at step 3 and newly started applications will fail to finish launching.

The prefix is used to ensure to continue generating new item types in every run if the script is stopped via the unresponsive-script dialog in step 2.

Comment 13

[Rafael Gieschke]

Firefox will crash if data in dataTransfer.setData is too large (unknown if this is further exploitable)

0. Use Windows.
1. Visit <https://ebenda.org/2015/drag-drop/flood2.html>.
2. Drag the Firefox image.
3. Firefox will crash.

Comment 14

[Rafael Gieschke]

Linux/X11 seems to handle both bugs well and seems to really send these items if not stopped via the unresponsive-script dialog. At most, Firefox seems to become unresponsive (and the system becomes a bit sluggish) but does not immediately crash. Chrome handles all cases reasonable well by only crashing the tab after some time.

Just for completeness, I do not know and care about how this stuff works under OS X but someone should probably look at that, too. And parts of these bugs might also be possible using the already implemetend parts of Clipboard API in Firefox.

Comment 15

[Rafael Gieschke]

Maybe someone can add [e-mail] to CC, who has investigated the same bug as issue 580005 and discussed possibly formats in bug 860857? I will not do so myself as I think that Mozilla should be in ultimate control with whom to share this discussion. Thanks.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Please file separate bugs for distinct issues. Otherwise, it becomes very hard to determine when something is "fixed" if a report/bugzilla ID is for separate things.

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to Rafael Gieschke from comment #15)
> I will not [CC someone] myself as I think that Mozilla should be in ultimate
> control with whom to share this discussion. Thanks.

Please feel free to CC anyone you trust to handle private information appropriately. As the reporter you could reveal this to anyone at any time anywhere else. If instead you CC them into the conversation here then we can take part, too.

Comment 18

[Neil Deakin]

All of the comments above describe the same issue, except for comment 13 which should be filed as a separate bug.

Comment 19

[Neil Deakin]

Attachment: Part 1 - prevent pages from using non-string data

We can prevent pages from using non-string data values. Once 906420 is done, we might be change these methods to chrome-only, but would need to look if there are sites that depend on it.

Comment 20

[Neil Deakin]

Attachment: Part 2 - put unrecognized types into a placeholder type

This makes all types not on a whitelist be stored in a separate special-type.

Some notes:
  - we may wish to alter the whitelist of known formats.
  - I haven't compiled or testes this on Windows, only on Mac and Linux
  - the special-type I used in not the same one that Chrome uses.

Comment 21

[Neil Deakin]

Attachment: Part 3 - filter inaccessible uris

This patch is unfinished, but filters out file uris from urls. It still needs to be able to handle text/uri-list.

Comment 22

[Rafael Gieschke]

(In reply to Neil Deakin from comment #18)
> All of the comments above describe the same issue, except for comment 13
> which should be filed as a separate bug.

Thank you. Yes, these basically are all variants of the same issue, removing the dependency on Chrome and adding a DoS attack by exhausting system resources. The latter would rule out the blacklist-only fix. But as you have already implemented the proper catchall-format fix, this actually does not really matter.

Updating comment 14, this actually DOES work on Linux/X11, too. Item formats will have to go through XInternAtom and the X server will have to keep the mappings in memory (which probably is why the system then feels sluggish) even after Firefox is closed ("It will become undefined only when the last connection to the X server closes.", <http://linux.die.net/man/3/xinternatom>). As Atom is "typedef unsigned long int Atom;" it is hard though to exhaust the complete namespace, here.

I have filed the bug in comment 13 as bug 1249521 (including crash reports). This also includes one further setData abuse I forgot about (using a very long format string) that should already be handled by the patches in this bug. They all look like OOM (I tested this on a VM with limited memory), so the bug might also be closed directly again.

I also filed bug 1249522 for the issue in comment 10 (local file path is available to web page after drag and drop).

Comment 23

[Rafael Gieschke]

(In reply to Neil Deakin from comment #20)
> Created attachment 8720650 [details] [diff] [review]
> Part 2 - put unrecognized types into a placeholder type
> 
> This makes all types not on a whitelist be stored in a separate special-type.

Your "application/x-moz-custom-clipdata" format looks great, the type tag might (one day) also facilitate something like exchanging file-kind DataTransferItem between browsers by providing paths to Blobs already written to disk. In bug 860857, concerns were raised about performance as the whole data has to be read every time. This might somewhat be avoided by adding an index structure at the front or sending another "application/x-moz-custom-cliptypes" format containing only the types. You would then not have to actually parse the real data (and the sending application would not have to send it) until getData or getAsString (here parsing could even be async) is being called.

Comment 24

[Rafael Gieschke]

As bug 1249521 is public, the URLs in this bug are now:

- <https://ebenda.org/2015/drag-drop/> -> <https://ebenda.org/2015/drag-drop/spoof.html>
- <https://ebenda.org/2015/drag-drop/index2.html> -> <https://ebenda.org/2015/drag-drop/spoof2.html>
- <https://ebenda.org/2015/drag-drop/index3.html> -> <https://ebenda.org/2015/drag-drop/spoof3.html>
- <https://ebenda.org/2015/drag-drop/flood.html> -> <https://ebenda.org/2015/drag-drop/flood-dos.html>

Comment 25

[Neil Deakin]

Attachment: Part 1 - prevent pages from using non-string data

Fixes test issues.

Comment 26

[Neil Deakin]

Attachment: Part 2.2 - put unrecognized types into a placeholder type

Fixes some minor issues and tests.

Comment 27

[Neil Deakin]

Try run is mostly ok on non-Windows:

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=16959258

I believe the latest patches fix the issues on Linux, Mac and Android.

I won't be as available for a couple of weeks.

Comment 28

[Neil Deakin]

Attachment: Testcase

Testing
-------

Patch 1 has an automated testcase. Step 5 below tests tab dragging which could be affected by this bug.

For patch 2, I used the attached testcase for testing that no behaviour problems arise.

Test 1:
 1. Select the text from the upper box (red).
 2. Copy it to the clipboard.
 3. Click in the destination box (blue) and paste there.
 
Both step 2 and step 3 should generate output in the last box (black). The background will turn red on failure. There should be three types listed, 'text/html', 'text/bacon' and 'text/uri-list'.

Test 2:
 1. Select the text from the upper box (red).
 2. Drag the selection and drop it into the blue box.

The data should match the expected result.


Test 3: Repeat both test 1 and 2 with e10s disabled versus enabled.

Test 4: Create two profiles and run Firefox two times, one for each profile. Load the testcase in each running instance. Repeat step 1, but copy and paste between each instance. 

Test 5: Using the two separate profiles, drag and drop from one running instance to the other.

Test 6: Ensure that dragging browser tabs still works properly, as it uses a custom type with non-string data.

Test 7: Repeat all of the above on each platform. This bug does not affect Android as it only supports a plain text clipboard.

The data should be the same in all cases. Note that the 'text/html' type may be reformatted to include surrounding tags (html, head, charset, body, and so on)
or fragment comments when moved to and from the clipboard or system drag buffer. This is OK.

It is important to test different profiles as this ensures that data goes through the real system clipboard/drag buffer. When copying within the same instance, cached data is used instead using different codepaths.

I verified the patches using this testcase and the steps above on Mac, Linux and Windows.

Comment 29

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

Sorry, I'm late with feedback, since review queue tends to be rather long all the time.
If you think the patches are ready for review, switch the feedback? to review? to speed up the process.

Comment 30

[Neil Deakin]

Attachment: Part 2.3 - put unrecognized types into a placeholder type

Updated patch.

Note that bug 860857 is essentially fixed by this.

Comment 31

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

Comment on attachment 8721537 [details] [diff] [review]
Part 1 - prevent pages from using non-string data


>+  exception = null;
>+  try {
>+    clipboardData.mozSetDataAt("application/something", "This is data", 0);
>+  } catch(ex) {
>+    exception = ex;
>+  }
>+  is(exception, null, "Can set custom data to a string");
>+  SimpleTest.finish();
>+}


Please test also passing {} as data, and [].

Comment 32

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

Comment on attachment 8734458 [details] [diff] [review]
Part 2.3 - put unrecognized types into a placeholder type

"If format equals "text", change it to "text/plain".
If format equals "url", change it to "text/uri-list"."
is from the spec. Something to keep in mind for ::SetData method
But that is perhaps something for a different bug.



>+// Used for custom clipboard types.
>+enum CustomClipboardTypeId {
>+  CustomClipboardTypeId_None,
>+  CustomClipboardTypeId_String
Could you start enum values with 'e'

>+  nsCOMPtr<nsIStorageStream> storageStream;
>+  nsCOMPtr<nsIBinaryOutputStream> stream;
>+
>   bool added = false;
>-  for (uint32_t f = 0; f < count; f++) {
>-    const TransferItem& formatitem = item[f];
>-    if (!formatitem.mData) { // skip empty items
>-      continue;
>+  bool handlingCustomFormats = true;
>+  uint32_t totalCustomLength = 0;
>+
>+  const char* knownFormats[] = { kTextMime, kHTMLMime, kNativeHTMLMime, kRTFMime,
>+                                 kURLMime, kURLDataMime, kURLDescriptionMime, kURLPrivateMime,
>+                                 kPNGImageMime, kJPEGImageMime, kGIFImageMime, kNativeImageMime,
>+                                 kFileMime, kFilePromiseMime, kFilePromiseDirectoryMime,
>+                                 kMozTextInternal, kHTMLContext, kHTMLInfo };
>+
>+  /*
>+   * Two passes are made here to iterate over all of the types. First, look for
>+   * any types that are not in the list of known types. For this pass, handlingCustomFormats
>+   * will be false.
Sounds odd. We're looking for custom formats (== non-known types), yet handlingCustomFormats is false.
And based on the code, the first pass has handlingCustomFormats == true.

 Data that corresponds to unknown types will be pulled out and
>+   * inserted into a single type (kCustomTypesMime) by writing the data into a stream.
>+   *
>+   * The second pass will iterate over the formats looking for known types. These are
>+   * added as is. The unknown types are all then inserted as a single type (kCustomTypesMime)
>+   * in the same position of the first custom type. This model is used to maintain the
>+   * format order as best as possible.
>+   *
It is unclear to me why we need one customtypes, and not just store all the custom types separately.




>+   * The format of the kCustomTypesMime type is one or more of the following stored sequentially:
>+   *   <32-bit> type (only none or string is supported)
>+   *   <32-bit> length of format
>+   *   <wide string> format
>+   *   <32-bit> length of data
>+   *   <wide string> data or <pointer> data provider
I don't understand this. How does one know the data isn't data but a provider.

>+   * A type of CustomClipboardTypeId_None ends the list, without any following data.
>+   */
>+  do {
>+    for (uint32_t f = 0; f < count; f++) {
>+      const TransferItem& formatitem = item[f];
>+      if (!formatitem.mData) { // skip empty items
>+        continue;
>+      }
>+
>+      // If the data is of one of the well-known formats, use it directly.
>+      bool isCustomFormat = true;
>+      for (uint32_t f = 0; f < ArrayLength(knownFormats); f++) {
>+        if (formatitem.mFormat.EqualsASCII(knownFormats[f])) {
>+          isCustomFormat = false;
>+          break;
>+        }
>+      }
>+
>+      if (handlingCustomFormats == isCustomFormat) {
This is hard to read. Couldn't you just have clearly one if() for the initial round when
handlingCustomFormats is true and 'else' for the case when it is false.
And in !handlingCustomFormats case, check first if stream, and if so, create the input stream.
(though, it is still unclear to me what the custom format and stream are about, and why they are needed.)

>+        }
>+        else {

Nit, it should be } else {

>+      else if (isCustomFormat && stream) {
>+        // Write out a terminator.
>+        totalCustomLength += 4;
>+        stream->Write32(CustomClipboardTypeId_None);
>+
>+        nsCOMPtr<nsIInputStream> inputStream;
>+        storageStream->NewInputStream(0, getter_AddRefs(inputStream));
>+
>+        uint32_t amountRead;
>+        char* buffer = static_cast<char*>(moz_xmalloc(totalCustomLength));

Don't you rather want to use nsStringBuffer here so that we can actually share the data...

>+        inputStream->Read(buffer, totalCustomLength, &amountRead);
>+
>+        nsAutoCString str;
>+        str.Adopt(buffer, totalCustomLength);
...here


>+
>+        nsCOMPtr<nsISupportsCString> strSupports(do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID));
>+        strSupports->SetData(str);
...and then here.

What happens if a web page tries to set application/x-moz-custom-clipdata as data type?


But I don't understand the need for application/x-moz-custom-clipdata and all the complexity its handling causes.
Please explain.
(Wondering why we can't add some flag to transferrable that certain item is "custom". And yes, limit data to string.)

Comment 33

[Neil Deakin]

Recognized types (text, urls, html, etc) are placed on the clipboard as is using the same type you passed to setData. This is needed so that well-known types can be exchanged between other applications.

Unrecognized types (those made up by a web page) are currently done the same way. This means that a page can craft some data with a type name that another application happens to use, resulting in a potential security issue. The reverse is also true in that we can read data of some type. The original testcase links don't work anymore but I believe it used some Windows file types.

This patch moves the data for all non-recognized types into a single custom type that gets serialized and deserialized when needed. This is similar technique to what Chrome does.

Comment 34

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

But why we need to serialize them to one type, and why not just have some flag telling that certain entry in the transferrable is custom? Or just check each time it is accessed whether it is custom, and if so, not expose it to OS (or from OS to web)?

Comment 35

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #34)
> But why we need to serialize them to one type, and why not just have some
> flag telling that certain entry in the transferrable is custom?

We need to put the data into some type that isn't going to be used by the OS. If a page adds multiple custom types, they are all put into this type to be easily found later when pasting/dropping. I'm not clear what a flag would be for or where it would go?

> Or just check each time it is accessed whether it is custom, and if so, not expose
> it to OS (or from OS to web)?

That would prevent someone from cut and pasting (or dragging) between two profiles, or when restarting the browser in-between cut and paste. Uncommon cases perhaps, but I don't see any reason to intentionally prevent them.

Comment 36

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

So couldn't we store a flag in DataStruct (nsTransferable.h) and just not give custom data to OS?

Comment 37

[Rafael Gieschke]

(In reply to Neil Deakin from comment #33)
> The reverse is also true in that we can read data of some type. The original
> testcase links don't work anymore but I believe it used some Windows file
> types.

Sorry, the URL is now <https://ebenda.org/2015/drag-drop/spoof.html> (see comment 24). It, indeed, uses the "FilenameW" type.

One other important aspect of this solution: a naïve approach would be to simply prefix every unrecognized type with "web+" (like it is done at registerProtocolHandler). This would not be enough, though, as every new type passed to the OS consumes system resources until the next restart of the system, which can be exploited to DoS the system (shown in comment 12 and comment 22, URL is now <https://ebenda.org/2015/drag-drop/flood-dos.html>). So, it really must be only ONE additional catch-all type passed to the OS.

Besides cut and pasting and dragging between two profiles, this will also enable interoperability between browsers (if you can agree with Google and the others on a common format, see bug 860857 comment 19, might be specced in <https://w3c.github.io/clipboard-apis/>). A use case would be something like <https://ebenda.org/2015/drag-drop/spoof2.html>.

Comment 38

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #36)
> So couldn't we store a flag in DataStruct (nsTransferable.h) and just not
> give custom data to OS?

For the reasons in my second point above and Rafael gave. Also, if one placed only custom data into dataTransfer, there would be nothing left to add if we stripped it out.

I intentionally used the same technique as Chrome, although not compatible (we can spec out a common format later if desired).

Comment 39

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

So we do want to give the custom data to OS so that FF can get it back from there?

Comment 40

[Neil Deakin]

Yes, the plan for the future (see bug 860857) is also to come up with a common format that can be interoperable between browsers.

Comment 41

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

ok, could you update the patch and ask review.

Comment 42

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #32)
> Comment on attachment 8734458 [details] [diff] [review]
> Part 2.3 - put unrecognized types into a placeholder type
> 
> "If format equals "text", change it to "text/plain".
> If format equals "url", change it to "text/uri-list"."
> is from the spec. Something to keep in mind for ::SetData method
> But that is perhaps something for a different bug.
> 

We already do this in GetRealFormat.


> I don't understand this. How does one know the data isn't data but a provider.

The comment should be updated as we only need to handle strings


> What happens if a web page tries to set application/x-moz-custom-clipdata as data type?


I'll add something that will block this.

Comment 43

[Neil Deakin]

Attachment: Part 2.4 - put unrecognized types into a placeholder type

Comment 44

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

Comment on attachment 8738680 [details] [diff] [review]
Part 2.4 - put unrecognized types into a placeholder type

>+   * The format of the kCustomTypesMime type is one or more of the following stored sequentially:
>+   *   <32-bit> type (only none or string is supported)
>+   *   <32-bit> length of format
>+   *   <wide string> format
>+   *   <32-bit> length of data
>+   *   <wide string> data
ok, so stream should write those lengths automatically.
...

>+            stream->Write32(eCustomClipboardTypeId_String);
>+            stream->WriteWStringZ(formatitem.mFormat.get());
>+            stream->WriteWStringZ(data.get());
But WriteWStringZ isn't quite '\0' safe, as far as I see. and data may contain '\0'
'\u0000' is a valid JS string. I assume some devs do use DOMString as a data container.
So, should we write and read bytes and not WStringsZ, or nsIBinaryOutputStream should have method taking
const nsAString& as a param and actually use its length 

>+            totalCustomLength += formatitem.mFormat.Length() * 2 + length + 12;
And this doesn't deal with possible '\0' in the strings.

>+        // Read the data from the string and add a null-terminator as ToString needs it.
>+        uint32_t amountRead;
>+        inputStream->Read(static_cast<char *>(stringBuffer->Data()), totalCustomLength, &amountRead);
drop space after 'char'
So here we might be able to read only shorter string

>+        static_cast<char *>(stringBuffer->Data())[totalCustomLength] = 0;
yet this would set the '\0' to the end of the string buffer. So part of the memory might be uninitialized.

>-      CacheExternalData(formats[f], 0, sysPrincipal);
>+      if (f == 0) {
>+        FillInExternalCustomTypes(0, sysPrincipal);
>+      }
>+      else {
Nit, it should be } else {
same also elsewhere

>+DataTransfer::FillInExternalCustomTypes(uint32_t aIndex, nsIPrincipal* aPrincipal)
>+{
>+  TransferItem item;
>+  item.mFormat.AssignLiteral(kCustomTypesMime);
>+
>+  FillInExternalData(item, aIndex);
>+  if (!item.mData)
>+    return;
Nit
if (expr) {
  ...
}
same also elsewhere
>+      is(cd.getData("text/x-custom"), "Custom Text", "paste text/custom multiple types");
>     }
>-    // this is empty because only the built-in types are supported at the moment
>-    is(cd.getData("text/x-custom"), "", "paste text/custom multiple types");
>+    else {
>+      is(cd.getData("text/x-custom"), "", "paste text/custom multiple types");
>+    }
>+
>+    is(cd.getData("application/x-moz-custom-clipdata"), "", "application/x-moz-custom-clipdata is not present");
>+
>+    exh = false;
>+    try { cd.setData("application/x-moz-custom-clipdata", "Some Data"); } catch (ex) { exh = true; }
>+    ok(exh, "exception occured setData with application/x-moz-custom-clipdata");
it might be good to test strings with \u0000 values

>+    else if (flavorStr.EqualsLiteral(kCustomTypesMime)) {
>+      NSString* type = [cocoaPasteboard availableTypeFromArray:[NSArray arrayWithObject:kCustomTypesPboardType]];
>+      if (!type)
>+        continue;
>+
>+      NSData *pasteboardData = GetDataFromPasteboard(cocoaPasteboard, type);
Nit, be consistent with * usage. It goes with the type, not with variable name when defining a variable.

Comment 45

[Neil Deakin]

Attachment: Part 2.5 - put unrecognized types into a placeholder type

Comment 46

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

Comment on attachment 8741346 [details] [diff] [review]
Part 2.5 - put unrecognized types into a placeholder type


>+      uint32_t length;
>+      nsCOMPtr<nsISupports> convertedData;
>+
>+      if (handlingCustomFormats) {
>+        if (!ConvertFromVariant(formatitem.mData, getter_AddRefs(convertedData), &length)) {
Might be good to add some comment that length is in bytes. Maybe just rename the variable to
lengthInBytes or something. Otherwise one needs to always read ConvertFromVariant what it does.

>+
>+            int32_t formatLength = formatitem.mFormat.Length() << 1;
Isn't * 2 easier to read? Or perhaps even formatitem.mFormat.Length() * sizeof(nsString::char_type);

>+            totalCustomLength += formatLength + length + 12;
This magical '12' needs a comment, and perhaps change it to (sizeof(uint32_t) * 3)


>+          }
>+        }
>+      } else if (isCustomFormat && stream) {
>+        // This is the second pass of the loop (handlingCustomFormats is false).
>+        // When encountering the first custom format, append all of the stream
>+        // at this position.
>+
>+        // Write out a terminator.
>+        totalCustomLength += 4;
s/4/sizeof(uint32_t)/

>+        RefPtr<nsStringBuffer> stringBuffer = nsStringBuffer::Alloc(totalCustomLength + 1);
>+
>+        // Read the data from the string and add a null-terminator as ToString needs it.
>+        uint32_t amountRead;
>+        inputStream->Read(static_cast<char*>(stringBuffer->Data()), totalCustomLength, &amountRead);
>+        static_cast<char *>(stringBuffer->Data())[amountRead] = 0;
s/char */char*/

>
+DataTransfer::FillInExternalCustomTypes(nsIVariant* aData, uint32_t aIndex, nsIPrincipal* aPrincipal)
>+{
>+  char* chrs;
You leak chrs...

>+  uint32_t len = 0;
>+  nsresult rv = aData->GetAsStringWithSize(&len, &chrs);
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }
>+
>+  nsAutoCString str(chrs, len);
so make this nsCString which Adopts chrs ?

>+  uint32_t type;
>+  do {
>+    stream->Read32(&type);
>+    if (type == eCustomClipboardTypeId_String) {
>+      uint32_t formatLength;
>+      stream->Read32(&formatLength);
>+      char* formatBytes;
>+      stream->ReadBytes(formatLength, &formatBytes);
>+      nsAutoString format;
>+      format.Adopt(reinterpret_cast<char16_t*>(formatBytes), formatLength >> 1);
I'd prefer / sizeof(...the right type...)

>+      uint32_t dataLength;
>+      stream->Read32(&dataLength);
>+      char* dataBytes;
>+      stream->ReadBytes(dataLength, &dataBytes);
>+      nsAutoString data;
>+      data.Adopt(reinterpret_cast<char16_t*>(dataBytes), dataLength >> 1);
same here

>+    else if (flavorStr.EqualsLiteral(kCustomTypesMime)) {
>+      NSString* type = [cocoaPasteboard availableTypeFromArray:[NSArray arrayWithObject:kCustomTypesPboardType]];
>+      if (!type) {
>+        continue;
>+      }
>+
>+      NSData* pasteboardData = GetDataFromPasteboard(cocoaPasteboard, type);
>+      if (!pasteboardData) {
>+        continue;
>+      }
>+
>+      unsigned int dataLength = [pasteboardData length];
>+      void* clipboardDataPtr = malloc(dataLength);
>+      if (!clipboardDataPtr) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      [pasteboardData getBytes:clipboardDataPtr];
>+
>+      nsCOMPtr<nsISupports> genericDataWrapper;
>+      nsPrimitiveHelpers::CreatePrimitiveForData(flavorStr, clipboardDataPtr, dataLength,
>+                                                 getter_AddRefs(genericDataWrapper));
>+
>+      aTransferable->SetTransferData(flavorStr, genericDataWrapper, sizeof(nsIInputStream*));
you leak clipboardDataPtr, call free(clipboardDataPtr); like the code above.

>+++ b/widget/cocoa/nsDragService.mm
...
>+    else if (flavorStr.EqualsLiteral(kCustomTypesMime)) {
>+      NSString* availableType = [item availableTypeFromArray:[NSArray arrayWithObject:kCustomTypesPboardType]];
>+      if (!availableType || !IsValidType(availableType, false)) {
>+          continue;
>+      }
>+      NSData *pasteboardData = [item dataForType:availableType];
>+      if (!pasteboardData) {
>+        continue;
>+      }
>+
>+      unsigned int dataLength = [pasteboardData length];
>+      void* clipboardDataPtr = malloc(dataLength);
>+      if (!clipboardDataPtr) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      [pasteboardData getBytes:clipboardDataPtr];
>+
>+      nsCOMPtr<nsISupports> genericDataWrapper;
>+      nsPrimitiveHelpers::CreatePrimitiveForData(flavorStr, clipboardDataPtr, dataLength,
>+                                                 getter_AddRefs(genericDataWrapper));
>+
>+      aTransferable->SetTransferData(flavorStr, genericDataWrapper, sizeof(nsIInputStream*));
ditto, clipboardDataPtr is leaked.


OSX and Windows specific stuff could use another reviews, from jimm and mstange I guess.

Comment 47

[Neil Deakin]

Attachment: Part 2.6 - put unrecognized types into a placeholder type

Windows parts

Comment 48

[Neil Deakin]

Comment on attachment 8742964 [details] [diff] [review]
Part 2.6 - put unrecognized types into a placeholder type

Mac parts

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 8742964 [details] [diff] [review]
Part 2.6 - put unrecognized types into a placeholder type

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

r+ on the widget/cocoa/ parts

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 8742964 [details] [diff] [review]
Part 2.6 - put unrecognized types into a placeholder type

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

clipboard changes look ok to me.

Comment 52

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb1d405b1239f342187201225a6cac0ff21440c2
Bug 1226977, non-string types are only needed for chrome contexts, r=smaug

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb1d405b1239

Comment 54

[Hallvord R. M. Steen [:hallvors]]

I've reported a bug against the HTML spec to lock down this API more and make it safer: https://github.com/whatwg/html/issues/1244
(I hope that's the right level to spec things at..)

Comment 55

[Karl Tomlinson (:karlt)]

Comment on attachment 8721537 [details] [diff] [review]
Part 1 - prevent pages from using non-string data

Comment 53.

Comment 56

[Karl Tomlinson (:karlt)]

Comment on attachment 8742964 [details] [diff] [review]
Part 2.6 - put unrecognized types into a placeholder type

Landed for bug 860857 as
https://hg.mozilla.org/mozilla-central/rev/4cbc83f4c4b1