Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer, r=enndeakin draft
authorMichael Layzell <michael@thelayzells.com>
Tue, 23 Aug 2016 11:29:48 -0400
changeset 406851 c2c52d6f8bfcf24ab19c53bf4b100e9889e9b60d
parent 404375 bad612fd3ab0263e3bcb37c06f2659d48a0687d7
child 529763 002c5cdec8a7b2d45e9c488ac55f55f609b47301
push id27844
push userbmo:michael@thelayzells.com
push dateMon, 29 Aug 2016 17:51:45 +0000
reviewersenndeakin
bugs1297186
milestone51.0a1
Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer, r=enndeakin MozReview-Commit-ID: 8LRBekaOUJR
dom/events/DataTransfer.cpp
dom/events/DataTransfer.h
dom/events/DataTransferItem.cpp
dom/events/DataTransferItem.h
dom/events/DataTransferItemList.cpp
dom/events/DataTransferItemList.h
dom/events/test/test_DataTransferItemList.html
--- a/dom/events/DataTransfer.cpp
+++ b/dom/events/DataTransfer.cpp
@@ -278,34 +278,36 @@ DataTransfer::SetEffectAllowedInt(uint32
 
 NS_IMETHODIMP
 DataTransfer::GetMozUserCancelled(bool* aUserCancelled)
 {
   *aUserCancelled = MozUserCancelled();
   return NS_OK;
 }
 
-FileList*
+already_AddRefed<FileList>
 DataTransfer::GetFiles(ErrorResult& aRv)
 {
-  return mItems->Files();
+  return mItems->Files(nsContentUtils::SubjectPrincipal());
 }
 
 NS_IMETHODIMP
 DataTransfer::GetFiles(nsIDOMFileList** aFileList)
 {
   if (!aFileList) {
     return NS_ERROR_FAILURE;
   }
 
-  ErrorResult rv;
-  RefPtr<FileList> files = GetFiles(rv);
-  if (NS_WARN_IF(rv.Failed())) {
-    return rv.StealNSResult();
-  }
+  // The XPCOM interface is only avaliable to system code, and thus we can
+  // assume the system principal. This is consistent with the previous behavour
+  // of this function, which also assumed the system principal.
+  //
+  // This code is also called from C++ code, which expects it to have a System
+  // Principal, and thus the SubjectPrincipal cannot be used.
+  RefPtr<FileList> files = mItems->Files(nsContentUtils::GetSystemPrincipal());
 
   files.forget(aFileList);
   return NS_OK;
 }
 
 already_AddRefed<DOMStringList>
 DataTransfer::GetTypes(ErrorResult& aRv) const
 {
@@ -692,32 +694,16 @@ DataTransfer::SetDataAtInternal(const ns
     return NS_ERROR_DOM_INDEX_SIZE_ERR;
   }
 
   // Don't allow the custom type to be assigned.
   if (aFormat.EqualsLiteral(kCustomTypesMime)) {
     return NS_ERROR_TYPE_ERR;
   }
 
-  // Don't allow non-chrome to add non-string or file data. We'll block file
-  // promises as well which are used internally for drags to the desktop.
-  if (!nsContentUtils::IsSystemPrincipal(aSubjectPrincipal)) {
-    if (aFormat.EqualsLiteral(kFilePromiseMime) ||
-        aFormat.EqualsLiteral(kFileMime)) {
-      return NS_ERROR_DOM_SECURITY_ERR;
-    }
-
-    uint16_t type;
-    aData->GetDataType(&type);
-    if (type == nsIDataType::VTYPE_INTERFACE ||
-        type == nsIDataType::VTYPE_INTERFACE_IS) {
-      return NS_ERROR_DOM_SECURITY_ERR;
-    }
-  }
-
   return SetDataWithPrincipal(aFormat, aData, aIndex, aSubjectPrincipal);
 }
 
 void
 DataTransfer::MozSetDataAt(JSContext* aCx, const nsAString& aFormat,
                            JS::Handle<JS::Value> aData,
                            uint32_t aIndex, ErrorResult& aRv)
 {
@@ -831,17 +817,17 @@ DataTransfer::GetFilesAndDirectories(Err
     return nullptr;
   }
 
   RefPtr<Promise> p = Promise::Create(global, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
-  RefPtr<FileList> files = mItems->Files();
+  RefPtr<FileList> files = mItems->Files(nsContentUtils::SubjectPrincipal());
   if (NS_WARN_IF(!files)) {
     return nullptr;
   }
 
   Sequence<RefPtr<File>> filesSeq;
   files->ToSequence(filesSeq, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
--- a/dom/events/DataTransfer.h
+++ b/dom/events/DataTransfer.h
@@ -141,17 +141,17 @@ public:
   void GetData(const nsAString& aFormat, nsAString& aData, ErrorResult& aRv);
 
   void SetData(const nsAString& aFormat, const nsAString& aData,
                ErrorResult& aRv);
 
   void ClearData(const mozilla::dom::Optional<nsAString>& aFormat,
                  mozilla::ErrorResult& aRv);
 
-  FileList* GetFiles(mozilla::ErrorResult& aRv);
+  already_AddRefed<FileList> GetFiles(mozilla::ErrorResult& aRv);
 
   already_AddRefed<Promise> GetFilesAndDirectories(ErrorResult& aRv);
 
   already_AddRefed<Promise> GetFiles(bool aRecursiveFlag, ErrorResult& aRv);
 
 
   void AddElement(Element& aElement, mozilla::ErrorResult& aRv);
 
--- a/dom/events/DataTransferItem.cpp
+++ b/dom/events/DataTransferItem.cpp
@@ -98,41 +98,46 @@ DataTransferItem::SetData(nsIVariant* aD
         break;
       }
     }
 
     mData = nullptr;
     return;
   }
 
-  mKind = KIND_OTHER;
   mData = aData;
+  mKind = KindFromData(mData);
+}
 
+/* static */ DataTransferItem::eKind
+DataTransferItem::KindFromData(nsIVariant* aData)
+{
   nsCOMPtr<nsISupports> supports;
   nsresult rv = aData->GetAsISupports(getter_AddRefs(supports));
   if (NS_SUCCEEDED(rv) && supports) {
     // Check if we have one of the supported file data formats
     if (nsCOMPtr<nsIDOMBlob>(do_QueryInterface(supports)) ||
         nsCOMPtr<BlobImpl>(do_QueryInterface(supports)) ||
         nsCOMPtr<nsIFile>(do_QueryInterface(supports))) {
-      mKind = KIND_FILE;
-      return;
+      return KIND_FILE;
     }
   }
 
   nsAutoString string;
   // If we can't get the data type as a string, that means that the object
   // should be considered to be of the "other" type. This is impossible
   // through the APIs defined by the spec, but we provide extra Moz* APIs,
   // which allow setting of non-string data. We determine whether we can
   // consider it a string, by calling GetAsAString, and checking for success.
   rv = aData->GetAsAString(string);
   if (NS_SUCCEEDED(rv)) {
-    mKind = KIND_STRING;
+    return KIND_STRING;
   }
+
+  return KIND_OTHER;
 }
 
 void
 DataTransferItem::FillInExternalData()
 {
   if (mData) {
     return;
   }
@@ -230,36 +235,44 @@ DataTransferItem::FillInExternalData()
     NS_WARNING("Clipboard data provided by the OS does not match predicted kind");
   }
 #endif
 }
 
 already_AddRefed<File>
 DataTransferItem::GetAsFile(ErrorResult& aRv)
 {
+  return GetAsFileWithPrincipal(nsContentUtils::SubjectPrincipal(), aRv);
+}
+
+already_AddRefed<File>
+DataTransferItem::GetAsFileWithPrincipal(nsIPrincipal* aPrincipal, ErrorResult& aRv)
+{
   if (mKind != KIND_FILE) {
     return nullptr;
   }
 
-  nsCOMPtr<nsIVariant> data = Data(nsContentUtils::SubjectPrincipal(), aRv);
+  // This is done even if we have an mCachedFile, as it performs the necessary
+  // permissions checks to ensure that we are allowed to access this type.
+  nsCOMPtr<nsIVariant> data = Data(aPrincipal, aRv);
   if (NS_WARN_IF(!data || aRv.Failed())) {
     return nullptr;
   }
 
-  nsCOMPtr<nsISupports> supports;
-  aRv = data->GetAsISupports(getter_AddRefs(supports));
-  MOZ_ASSERT(!aRv.Failed() && supports,
-             "File objects should be stored as nsISupports variants");
-  if (aRv.Failed() || !supports) {
-    return nullptr;
-  }
-
   // Generate the dom::File from the stored data, caching it so that the
   // same object is returned in the future.
   if (!mCachedFile) {
+    nsCOMPtr<nsISupports> supports;
+    aRv = data->GetAsISupports(getter_AddRefs(supports));
+    MOZ_ASSERT(!aRv.Failed() && supports,
+               "File objects should be stored as nsISupports variants");
+    if (aRv.Failed() || !supports) {
+      return nullptr;
+    }
+
     if (nsCOMPtr<nsIDOMBlob> domBlob = do_QueryInterface(supports)) {
       Blob* blob = static_cast<Blob*>(domBlob.get());
       mCachedFile = blob->ToFile();
     } else if (nsCOMPtr<BlobImpl> blobImpl = do_QueryInterface(supports)) {
       MOZ_ASSERT(blobImpl->IsFile());
       mCachedFile = File::Create(mDataTransfer, blobImpl);
     } else if (nsCOMPtr<nsIFile> ifile = do_QueryInterface(supports)) {
       mCachedFile = File::CreateFromFile(mDataTransfer, ifile);
@@ -270,17 +283,24 @@ DataTransferItem::GetAsFile(ErrorResult&
 
   RefPtr<File> file = mCachedFile;
   return file.forget();
 }
 
 already_AddRefed<FileSystemEntry>
 DataTransferItem::GetAsEntry(ErrorResult& aRv)
 {
-  RefPtr<File> file = GetAsFile(aRv);
+  return GetAsEntryWithPrincipal(nsContentUtils::SubjectPrincipal(), aRv);
+}
+
+already_AddRefed<FileSystemEntry>
+DataTransferItem::GetAsEntryWithPrincipal(nsIPrincipal* aPrincipal,
+                                          ErrorResult& aRv)
+{
+  RefPtr<File> file = GetAsFileWithPrincipal(aPrincipal, aRv);
   if (NS_WARN_IF(aRv.Failed()) || !file) {
     return nullptr;
   }
 
   nsCOMPtr<nsIGlobalObject> global;
   // This is annoying, but DataTransfer may have various things as parent.
   nsCOMPtr<EventTarget> target =
     do_QueryInterface(mDataTransfer->GetParentObject());
--- a/dom/events/DataTransferItem.h
+++ b/dom/events/DataTransferItem.h
@@ -43,17 +43,20 @@ public:
     , mDataTransfer(aDataTransfer)
   {
     MOZ_ASSERT(mDataTransfer, "Must be associated with a DataTransfer");
   }
 
   already_AddRefed<DataTransferItem> Clone(DataTransfer* aDataTransfer) const;
 
   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
+
+  // NOTE: This accesses the subject principal, and should not be called from C++
   void GetAsString(FunctionStringCallback* aCallback, ErrorResult& aRv);
+
   void GetKind(nsAString& aKind) const
   {
     switch (mKind) {
     case KIND_FILE:
       aKind = NS_LITERAL_STRING("file");
       return;
     case KIND_STRING:
       aKind = NS_LITERAL_STRING("string");
@@ -74,19 +77,25 @@ public:
   {
     return mKind;
   }
   void SetKind(eKind aKind)
   {
     mKind = aKind;
   }
 
+  // NOTE: This accesses the subject principal, and should not be called from C++
   already_AddRefed<File> GetAsFile(ErrorResult& aRv);
+  already_AddRefed<File> GetAsFileWithPrincipal(nsIPrincipal* aPrincipal,
+                                                ErrorResult& aRv);
 
+  // NOTE: This accesses the subject principal, and should not be called from C++
   already_AddRefed<FileSystemEntry> GetAsEntry(ErrorResult& aRv);
+  already_AddRefed<FileSystemEntry> GetAsEntryWithPrincipal(nsIPrincipal* aPrincipal,
+                                                            ErrorResult& aRv);
 
   DataTransfer* GetParentObject() const
   {
     return mDataTransfer;
   }
 
   nsIPrincipal* Principal() const
   {
@@ -115,16 +124,18 @@ public:
   {
     return mChromeOnly;
   }
   void SetChromeOnly(bool aChromeOnly)
   {
     mChromeOnly = aChromeOnly;
   }
 
+  static eKind KindFromData(nsIVariant* aData);
+
 private:
   ~DataTransferItem() {}
   already_AddRefed<File> CreateFileFromInputStream(nsIInputStream* aStream);
 
   // The index in the 2d mIndexedItems array
   uint32_t mIndex;
 
   bool mChromeOnly;
--- a/dom/events/DataTransferItemList.cpp
+++ b/dom/events/DataTransferItemList.cpp
@@ -195,25 +195,58 @@ DataTransferItemList::Add(File& aData, E
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
   MOZ_ASSERT(item->Kind() == DataTransferItem::KIND_FILE);
 
   return item;
 }
 
-FileList*
-DataTransferItemList::Files()
+already_AddRefed<FileList>
+DataTransferItemList::Files(nsIPrincipal* aPrincipal)
 {
+  // The DataTransfer can hold data with varying principals, coming from
+  // different windows. This means that permissions checks need to be made when
+  // accessing data from the DataTransfer. With the accessor methods, this is
+  // checked by DataTransferItem::Data(), however with files, we keep a cached
+  // live copy of the files list for spec compliance.
+  //
+  // A DataTransfer is only exposed to one webpage, and chrome code. The chrome
+  // code should be able to see all files on the DataTransfer, while the webpage
+  // should only be able to see the files it can see. As chrome code doesn't
+  // need as strict spec compliance as web visible code, we generate a new
+  // FileList object every time you access the Files list from chrome code, but
+  // re-use the cached one when accessing from non-chrome code.
+  //
+  // It is not legal to expose an identical DataTransfer object is to multiple
+  // different principals without using the `Clone` method or similar to copy it
+  // first. If that happens, this method will assert, and return nullptr in
+  // release builds. If this functionality is required in the future, a more
+  // advanced caching mechanism for the FileList objects will be required.
+  RefPtr<FileList> files;
+  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
+    files = new FileList(static_cast<nsIDOMDataTransfer*>(mDataTransfer));
+    GenerateFiles(files, aPrincipal);
+    return files.forget();
+  }
+
   if (!mFiles) {
     mFiles = new FileList(static_cast<nsIDOMDataTransfer*>(mDataTransfer));
+    mFilesPrincipal = aPrincipal;
     RegenerateFiles();
   }
 
-  return mFiles;
+  if (!aPrincipal->Subsumes(mFilesPrincipal)) {
+    MOZ_ASSERT(false, "This DataTransfer should only be accessed by the system "
+               "and a single principal");
+    return nullptr;
+  }
+
+  files = mFiles;
+  return files.forget();
 }
 
 void
 DataTransferItemList::MozRemoveByTypeAt(const nsAString& aType,
                                         uint32_t aIndex,
                                         ErrorResult& aRv)
 {
   if (NS_WARN_IF(mDataTransfer->IsReadOnly() ||
@@ -278,16 +311,32 @@ already_AddRefed<DataTransferItem>
 DataTransferItemList::SetDataWithPrincipal(const nsAString& aType,
                                            nsIVariant* aData,
                                            uint32_t aIndex,
                                            nsIPrincipal* aPrincipal,
                                            bool aInsertOnly,
                                            bool aHidden,
                                            ErrorResult& aRv)
 {
+  if (!nsContentUtils::IsSystemPrincipal(aPrincipal)) {
+    DataTransferItem::eKind kind = DataTransferItem::KindFromData(aData);
+    if (kind == DataTransferItem::KIND_OTHER) {
+      NS_WARNING("Disallowing adding non string/file types to DataTransfer");
+      aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+      return nullptr;
+    }
+
+    if (aType.EqualsASCII(kFileMime) ||
+        aType.EqualsASCII(kFilePromiseMime)) {
+      NS_WARNING("Disallowing adding x-moz-file or x-moz-file-promize types to DataTransfer");
+      aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+      return nullptr;
+    }
+  }
+
   if (aIndex < mIndexedItems.Length()) {
     nsTArray<RefPtr<DataTransferItem>>& items = mIndexedItems[aIndex];
     uint32_t count = items.Length();
     for (uint32_t i = 0; i < count; i++) {
       RefPtr<DataTransferItem> item = items[i];
       nsString type;
       item->GetType(type);
       if (type.Equals(aType)) {
@@ -486,30 +535,39 @@ DataTransferItemList::RegenerateFiles()
   // list. That way we can avoid the unnecessary work if the user never touches
   // the files list.
   if (mFiles) {
     // We clear the list rather than performing smaller updates, because it
     // simplifies the logic greatly on this code path, which should be very
     // infrequently used.
     mFiles->Clear();
 
-    uint32_t count = Length();
-    for (uint32_t i = 0; i < count; i++) {
-      ErrorResult rv;
-      bool found;
-      RefPtr<DataTransferItem> item = IndexedGetter(i, found, rv);
-      if (NS_WARN_IF(!found || rv.Failed())) {
+    DataTransferItemList::GenerateFiles(mFiles, mFilesPrincipal);
+  }
+}
+
+void
+DataTransferItemList::GenerateFiles(FileList* aFiles,
+                                    nsIPrincipal* aFilesPrincipal)
+{
+  MOZ_ASSERT(aFiles);
+  MOZ_ASSERT(aFilesPrincipal);
+  uint32_t count = Length();
+  for (uint32_t i = 0; i < count; i++) {
+    ErrorResult rv;
+    bool found;
+    RefPtr<DataTransferItem> item = IndexedGetter(i, found, rv);
+    if (NS_WARN_IF(!found || rv.Failed())) {
+      continue;
+    }
+
+    if (item->Kind() == DataTransferItem::KIND_FILE) {
+      RefPtr<File> file = item->GetAsFileWithPrincipal(aFilesPrincipal, rv);
+      if (NS_WARN_IF(rv.Failed() || !file)) {
         continue;
       }
-
-      if (item->Kind() == DataTransferItem::KIND_FILE) {
-        RefPtr<File> file = item->GetAsFile(rv);
-        if (NS_WARN_IF(rv.Failed() || !file)) {
-          continue;
-        }
-        mFiles->Append(file);
-      }
+      aFiles->Append(file);
     }
   }
 }
 
 } // namespace mozilla
 } // namespace dom
--- a/dom/events/DataTransferItemList.h
+++ b/dom/events/DataTransferItemList.h
@@ -66,17 +66,17 @@ public:
 
   void Clear(ErrorResult& aRv);
 
   already_AddRefed<DataTransferItem>
   SetDataWithPrincipal(const nsAString& aType, nsIVariant* aData,
                        uint32_t aIndex, nsIPrincipal* aPrincipal,
                        bool aInsertOnly, bool aHidden, ErrorResult& aRv);
 
-  FileList* Files();
+  already_AddRefed<FileList> Files(nsIPrincipal* aPrincipal);
 
   // Moz-style helper methods for interacting with the stored data
   void MozRemoveByTypeAt(const nsAString& aType, uint32_t aIndex,
                          ErrorResult& aRv);
   DataTransferItem* MozItemByTypeAt(const nsAString& aType, uint32_t aIndex);
   const nsTArray<RefPtr<DataTransferItem>>* MozItemsAt(uint32_t aIndex);
   uint32_t MozItemCount() const;
 
@@ -89,22 +89,25 @@ public:
 
 private:
   void ClearDataHelper(DataTransferItem* aItem, uint32_t aIndexHint,
                        uint32_t aMozOffsetHint, ErrorResult& aRv);
   DataTransferItem* AppendNewItem(uint32_t aIndex, const nsAString& aType,
                                   nsIVariant* aData, nsIPrincipal* aPrincipal,
                                   bool aHidden);
   void RegenerateFiles();
+  void GenerateFiles(FileList* aFiles, nsIPrincipal* aFilesPrincipal);
 
   ~DataTransferItemList() {}
 
   RefPtr<DataTransfer> mDataTransfer;
   bool mIsExternal;
   RefPtr<FileList> mFiles;
+  // The principal for which mFiles is cached
+  nsCOMPtr<nsIPrincipal> mFilesPrincipal;
   nsTArray<RefPtr<DataTransferItem>> mItems;
   nsTArray<nsTArray<RefPtr<DataTransferItem>>> mIndexedItems;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_DataTransferItemList_h
--- a/dom/events/test/test_DataTransferItemList.html
+++ b/dom/events/test/test_DataTransferItemList.html
@@ -74,16 +74,18 @@
       is(files[0], file, "It should be the same file as the file we originally created");
       is(file, e.dataTransfer.mozGetDataAt("text/html", 1),
          "It should be stored in index 1 for mozGetDataAt");
 
       var file2 = new File(['<a id="c"><b id="d">yo!</b></a>'], "myotherfile.html",
                            {type: "text/html"});
       dtList.add(file2);
 
+      todo(files.length == 2, "This test has chrome privileges, so the FileList objects aren't updated live");
+      files = e.dataTransfer.files;
       is(files.length, 2, "The files property should have been updated in place");
       is(files[1], file2, "It should be the same file as the file we originally created");
       is(file2, e.dataTransfer.mozGetDataAt("text/html", 2),
          "It should be stored in index 2 for mozGetDataAt");
 
       var oldLength = dtList.length;
       var randomString = "foo!";
       e.dataTransfer.mozSetDataAt("random/string", randomString, 3);
@@ -93,16 +95,19 @@
       var file3 = new File(['<a id="e"><b id="f">heya!</b></a>'], "yetanotherfile.html",
                            {type: "text/html"});
       e.dataTransfer.mozSetDataAt("random/string", file3, 3);
       is(oldLength + 1, dtList.length,
          "Replacing the entry with a file should add it to the list!");
       is(dtList[oldLength].getAsFile(), file3, "It should be stored in the last index as a file");
       is(dtList[oldLength].type, "random/string", "It should have the correct type");
       is(dtList[oldLength].kind, "file", "It should have the correct kind");
+
+      todo(files.length == 3, "This test has chrome privileges, so the FileList objects aren't updated live");
+      files = e.dataTransfer.files;
       is(files[files.length - 1], file3, "It should also be in the files list");
 
       oldLength = dtList.length;
       var nonstring = {};
       e.dataTransfer.mozSetDataAt("jsobject", nonstring, 0);
       is(oldLength + 1, dtList.length,
          "Adding a non-string object using the mozAPIs to index 0 should add an item to the dataTransfer");
       is(dtList[oldLength].type, "jsobject", "It should have the correct type");