From e4a2ac3b825effa3ac6b3bd036f92e70a30f8c50 Mon Sep 17 00:00:00 2001 From: Michael Layzell Date: Tue, 23 Aug 2016 11:29:48 -0400 Subject: [PATCH] Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer, r=enndeakin MozReview-Commit-ID: 8LRBekaOUJR --- dom/events/DataTransfer.cpp | 55 ++++++----- dom/events/DataTransfer.h | 6 +- dom/events/DataTransferItem.cpp | 48 ++++++--- dom/events/DataTransferItem.h | 11 +++ dom/events/DataTransferItemList.cpp | 99 ++++++++++++++----- dom/events/DataTransferItemList.h | 5 +- .../test/test_DataTransferItemList.html | 5 + 7 files changed, 169 insertions(+), 60 deletions(-) diff --git a/dom/events/DataTransfer.cpp b/dom/events/DataTransfer.cpp index 40aaa0e2b2c9..1781a9ebc928 100644 --- a/dom/events/DataTransfer.cpp +++ b/dom/events/DataTransfer.cpp @@ -283,10 +283,10 @@ DataTransfer::GetMozUserCancelled(bool* aUserCancelled) return NS_OK; } -FileList* +already_AddRefed 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 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 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 files = mItems->Files(); + RefPtr files = mItems->Files(nsContentUtils::SubjectPrincipal()); if (NS_WARN_IF(!files)) { return nullptr; } diff --git a/dom/events/DataTransfer.h b/dom/events/DataTransfer.h index 9b0b68e84ea7..8783bf86b5fe 100644 --- a/dom/events/DataTransfer.h +++ b/dom/events/DataTransfer.h @@ -146,7 +146,7 @@ public: void ClearData(const mozilla::dom::Optional& aFormat, mozilla::ErrorResult& aRv); - FileList* GetFiles(mozilla::ErrorResult& aRv); + already_AddRefed GetFiles(mozilla::ErrorResult& aRv); already_AddRefed 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 diff --git a/dom/events/DataTransferItem.cpp b/dom/events/DataTransferItem.cpp index 68f01ded1569..95f275a5d0e8 100644 --- a/dom/events/DataTransferItem.cpp +++ b/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 supports; nsresult rv = aData->GetAsISupports(getter_AddRefs(supports)); if (NS_SUCCEEDED(rv) && supports) { @@ -113,8 +117,7 @@ DataTransferItem::SetData(nsIVariant* aData) if (nsCOMPtr(do_QueryInterface(supports)) || nsCOMPtr(do_QueryInterface(supports)) || nsCOMPtr(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 DataTransferItem::GetAsFile(ErrorResult& aRv) +{ + return GetAsFileWithPrincipal(nsContentUtils::SubjectPrincipal(), aRv); +} + +already_AddRefed +DataTransferItem::GetAsFileWithPrincipal(nsIPrincipal* aPrincipal, ErrorResult& aRv) { if (mKind != KIND_FILE) { return nullptr; } - nsCOMPtr 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 data = Data(aPrincipal, aRv); if (NS_WARN_IF(!data || aRv.Failed())) { return nullptr; } - nsCOMPtr 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 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 domBlob = do_QueryInterface(supports)) { Blob* blob = static_cast(domBlob.get()); mCachedFile = blob->ToFile(); @@ -275,7 +288,14 @@ DataTransferItem::GetAsFile(ErrorResult& aRv) already_AddRefed DataTransferItem::GetAsEntry(ErrorResult& aRv) { - RefPtr file = GetAsFile(aRv); + return GetAsEntryWithPrincipal(nsContentUtils::SubjectPrincipal(), aRv); +} + +already_AddRefed +DataTransferItem::GetAsEntryWithPrincipal(nsIPrincipal* aPrincipal, + ErrorResult& aRv) +{ + RefPtr file = GetAsFileWithPrincipal(aPrincipal, aRv); if (NS_WARN_IF(aRv.Failed()) || !file) { return nullptr; } diff --git a/dom/events/DataTransferItem.h b/dom/events/DataTransferItem.h index 5c5f393a9fbd..f24c7126fe3e 100644 --- a/dom/events/DataTransferItem.h +++ b/dom/events/DataTransferItem.h @@ -48,7 +48,10 @@ public: already_AddRefed Clone(DataTransfer* aDataTransfer) const; virtual JSObject* WrapObject(JSContext* aCx, JS::Handle 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 GetAsFile(ErrorResult& aRv); + already_AddRefed GetAsFileWithPrincipal(nsIPrincipal* aPrincipal, + ErrorResult& aRv); + // NOTE: This accesses the subject principal, and should not be called from C++ already_AddRefed GetAsEntry(ErrorResult& aRv); + already_AddRefed GetAsEntryWithPrincipal(nsIPrincipal* aPrincipal, + ErrorResult& aRv); DataTransfer* GetParentObject() const { @@ -120,6 +129,8 @@ public: mChromeOnly = aChromeOnly; } + static eKind KindFromData(nsIVariant* aData); + private: ~DataTransferItem() {} already_AddRefed CreateFileFromInputStream(nsIInputStream* aStream); diff --git a/dom/events/DataTransferItemList.cpp b/dom/events/DataTransferItemList.cpp index d134fd336482..8ffa4962e498 100644 --- a/dom/events/DataTransferItemList.cpp +++ b/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 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 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 +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 files; + if (nsContentUtils::IsSystemPrincipal(aPrincipal)) { + files = new FileList(static_cast(mDataTransfer)); + GenerateFiles(files, aPrincipal); + return files.forget(); + } + if (!mFiles) { mFiles = new FileList(static_cast(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 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 item = IndexedGetter(i, found, rv); + if (NS_WARN_IF(!found || rv.Failed())) { + continue; + } + + if (item->Kind() == DataTransferItem::KIND_FILE) { + RefPtr file = item->GetAsFileWithPrincipal(aFilesPrincipal, rv); + if (NS_WARN_IF(rv.Failed() || !file)) { continue; } - - if (item->Kind() == DataTransferItem::KIND_FILE) { - RefPtr file = item->GetAsFile(rv); - if (NS_WARN_IF(rv.Failed() || !file)) { - continue; - } - mFiles->Append(file); - } + aFiles->Append(file); } } } diff --git a/dom/events/DataTransferItemList.h b/dom/events/DataTransferItemList.h index b62d61033b22..2288cea74707 100644 --- a/dom/events/DataTransferItemList.h +++ b/dom/events/DataTransferItemList.h @@ -71,7 +71,7 @@ public: uint32_t aIndex, nsIPrincipal* aPrincipal, bool aInsertOnly, bool aHidden, ErrorResult& aRv); - FileList* Files(); + already_AddRefed 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 mDataTransfer; bool mIsExternal; RefPtr mFiles; + // The principal for which mFiles is cached + nsCOMPtr mFilesPrincipal; nsTArray> mItems; nsTArray>> mIndexedItems; }; diff --git a/dom/events/test/test_DataTransferItemList.html b/dom/events/test/test_DataTransferItemList.html index e35d6b0d0e4e..f04799a706ec 100644 --- a/dom/events/test/test_DataTransferItemList.html +++ b/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;