Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer, r=enndeakin

MozReview-Commit-ID: 8LRBekaOUJR

dom/events/DataTransfer.cpp

@@ -283,10 +283,10 @@ DataTransfer::GetMozUserCancelled(bool* aUserCancelled)
   return NS_OK;
 }
 
-FileList*
+already_AddRefed<FileList>
 DataTransfer::GetFiles(ErrorResult& aRv)
 {
-  return mItems->Files();
+  return mItems->Files(nsContentUtils::SubjectPrincipal());
 }
 
 NS_IMETHODIMP
@@ -296,11 +296,13 @@ DataTransfer::GetFiles(nsIDOMFileList** 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;
@@ -666,6 +668,27 @@ DataTransfer::MozGetDataAt(JSContext* aCx, const nsAString& aFormat,
   }
 }
 
+/* static */ bool
+DataTransfer::PrincipalMaySetData(const nsAString& aType,
+                                  nsIVariant* aData,
+                                  nsIPrincipal* aPrincipal)
+{
+  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");
+      return false;
+    }
+
+    if (aType.EqualsASCII(kFileMime) ||
+        aType.EqualsASCII(kFilePromiseMime)) {
+      NS_WARNING("Disallowing adding x-moz-file or x-moz-file-promize types to DataTransfer");
+      return false;
+    }
+  }
+  return true;
+}
+
 nsresult
 DataTransfer::SetDataAtInternal(const nsAString& aFormat, nsIVariant* aData,
                                 uint32_t aIndex,
@@ -697,20 +720,8 @@ DataTransfer::SetDataAtInternal(const nsAString& aFormat, nsIVariant* aData,
     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;
-    }
+  if (!PrincipalMaySetData(aFormat, aData, aSubjectPrincipal)) {
+    return NS_ERROR_DOM_SECURITY_ERR;
   }
 
   return SetDataWithPrincipal(aFormat, aData, aIndex, aSubjectPrincipal);
@@ -836,7 +847,7 @@ DataTransfer::GetFilesAndDirectories(ErrorResult& aRv)
     return nullptr;
   }
 
-  RefPtr<FileList> files = mItems->Files();
+  RefPtr<FileList> files = mItems->Files(nsContentUtils::SubjectPrincipal());
   if (NS_WARN_IF(!files)) {
     return nullptr;
   }

dom/events/DataTransfer.h

@@ -146,7 +146,7 @@ public:
   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);
 
@@ -271,6 +271,10 @@ public:
   // Text and text/unicode become text/plain, and URL becomes text/uri-list
   void GetRealFormat(const nsAString& aInFormat, nsAString& aOutFormat) const;
 
+  static bool PrincipalMaySetData(const nsAString& aFormat,
+                                  nsIVariant* aData,
+                                  nsIPrincipal* aPrincipal);
+
 protected:
 
   // caches text and uri-list data formats that exist in the drag service or

dom/events/DataTransferItem.cpp

@@ -103,9 +103,13 @@ DataTransferItem::SetData(nsIVariant* aData)
     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) {
@@ -113,8 +117,7 @@ DataTransferItem::SetData(nsIVariant* aData)
     if (nsCOMPtr<nsIDOMBlob>(do_QueryInterface(supports)) ||
         nsCOMPtr<BlobImpl>(do_QueryInterface(supports)) ||
         nsCOMPtr<nsIFile>(do_QueryInterface(supports))) {
-      mKind = KIND_FILE;
-      return;
+      return KIND_FILE;
     }
   }
 
@@ -126,8 +129,10 @@ DataTransferItem::SetData(nsIVariant* aData)
   // 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
@@ -234,27 +239,35 @@ DataTransferItem::FillInExternalData()
 
 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();
@@ -275,7 +288,14 @@ DataTransferItem::GetAsFile(ErrorResult& aRv)
 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;
   }

dom/events/DataTransferItem.h

@@ -48,7 +48,10 @@ public:
   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) {
@@ -79,9 +82,15 @@ public:
     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
   {
@@ -120,6 +129,8 @@ public:
     mChromeOnly = aChromeOnly;
   }
 
+  static eKind KindFromData(nsIVariant* aData);
+
 private:
   ~DataTransferItem() {}
   already_AddRefed<File> CreateFileFromInputStream(nsIInputStream* aStream);

dom/events/DataTransferItemList.cpp

@@ -153,11 +153,17 @@ DataTransferItemList::Add(const nsAString& aData,
   nsAutoString format;
   mDataTransfer->GetRealFormat(aType, format);
 
+  nsIPrincipal* subjectPrincipal = nsContentUtils::SubjectPrincipal();
+
+  if (!DataTransfer::PrincipalMaySetData(format, data, subjectPrincipal)) {
+    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+    return nullptr;
+  }
+
   // We add the textual data to index 0. We set aInsertOnly to true, as we don't
   // want to update an existing entry if it is already present, as per the spec.
   RefPtr<DataTransferItem> item =
-    SetDataWithPrincipal(format, data, 0,
-                         nsContentUtils::SubjectPrincipal(),
+    SetDataWithPrincipal(format, data, 0, subjectPrincipal,
                          /* aInsertOnly = */ true,
                          /* aHidden = */ false,
                          aRv);
@@ -183,15 +189,22 @@ DataTransferItemList::Add(File& aData, ErrorResult& aRv)
   nsAutoString type;
   aData.GetType(type);
 
+  nsIPrincipal* subjectPrincipal = nsContentUtils::SubjectPrincipal();
+
+  if (!DataTransfer::PrincipalMaySetData(type, data, subjectPrincipal)) {
+    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+    return nullptr;
+  }
 
   // We need to add this as a new item, as multiple files can't exist in the
   // same item in the Moz DataTransfer layout. It will be appended at the end of
   // the internal specced layout.
   uint32_t index = mIndexedItems.Length();
   RefPtr<DataTransferItem> item =
-    SetDataWithPrincipal(type, data, index,
-                         nsContentUtils::SubjectPrincipal(),
-                         true, false, aRv);
+    SetDataWithPrincipal(type, data, index, subjectPrincipal,
+                         /* aInsertOnly = */ true,
+                         /* aHidden = */ false,
+                         aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
@@ -200,15 +213,48 @@ DataTransferItemList::Add(File& aData, ErrorResult& aRv)
   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
@@ -491,22 +537,31 @@ DataTransferItemList::RegenerateFiles()
     // 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);
     }
   }
 }

dom/events/DataTransferItemList.h

@@ -71,7 +71,7 @@ public:
                        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,
@@ -94,12 +94,15 @@ private:
                                   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;
 };

dom/events/test/test_DataTransferItemList.html

@@ -79,6 +79,8 @@
                            {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),
@@ -98,6 +100,9 @@
       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;