Crash while sending File slice on WebSocket on Windows when File has been obtained via FileSystemDirectoryReader

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: romain.fliedel, Assigned: baku)

Tracking

({crash, testcase})

50 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified, firefox-esr52 fixed, firefox53 verified, firefox54 verified)

Details

(Whiteboard: [sg:dos], crash signature)

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

2 years ago
Posted file report.zip
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.100 Safari/537.36

Steps to reproduce:

- implement a javascript file drop handler
- walk through the dropped files and folder to get the list of files
- open a WebSocket
- send the obtained files by chunks through the WebSocket as Blob using file.slice

I've attached:
- a index.html with sample code that causes Firefox to crash when you drop a folder containing a file in the "Drop some file here" zone. 
- a "dir" folder that you can use as a source example for dropping
- a wireshark dump of an invalid WebSocket frame sent by Firefox before crashing (pkt 12 contains the invalid frame starting with 0xF1 0xD0)

The crash is systematic on Windows. Under Linux it works as expected.

see some crash logs: 
https://crash-stats.mozilla.com/report/index/7acb3642-943b-4a9f-a404-c023f2170118 for 50.1
https://crash-stats.mozilla.com/report/index/ea4cbf46-7b20-4fcf-93ec-f1a832170118 for nightly Firefox 53.0a1

oddly, it seems fine on the 51 beta release (at least it does not crash), so maybe it as been fixed, but then I don't understand why it failed in nightly.

I don't know if there is a way to exploit this, but I in doubt, I prefer marking this as security problem



Actual results:

- Firefox crashed
- Firefox send malformed WebSocket frames



Expected results:

- slices should have been sent correctly to remote server
Reporter

Updated

2 years ago
Crash Signature: IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing
Reporter

Updated

2 years ago
Crash Signature: IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing → IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing error: message was deserialized

Comment 1

2 years ago
(In reply to Romain FLIEDEL from comment #0)
> Created attachment 8828008 [details]
> report.zip
> 
> User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
> Gecko) Chrome/54.0.2840.100 Safari/537.36
> 
> Steps to reproduce:
> 
> - implement a javascript file drop handler
> - walk through the dropped files and folder to get the list of files
> - open a WebSocket
> - send the obtained files by chunks through the WebSocket as Blob using
> file.slice
> 
> I've attached:
> - a index.html with sample code that causes Firefox to crash when you drop a
> folder containing a file in the "Drop some file here" zone. 
> - a "dir" folder that you can use as a source example for dropping
> - a wireshark dump of an invalid WebSocket frame sent by Firefox before
> crashing (pkt 12 contains the invalid frame starting with 0xF1 0xD0)
> 
> The crash is systematic on Windows. Under Linux it works as expected.
> 
> see some crash logs: 
> https://crash-stats.mozilla.com/report/index/7acb3642-943b-4a9f-a404-
> c023f2170118 for 50.1
> https://crash-stats.mozilla.com/report/index/ea4cbf46-7b20-4fcf-93ec-
> f1a832170118 for nightly Firefox 53.0a1
> 
> oddly, it seems fine on the 51 beta release (at least it does not crash), so
> maybe it as been fixed, but then I don't understand why it failed in nightly.

These crashes look like IPC crashes (ie related to cross-process communication). Is there some chance that when you tested on beta, e10s was off (check in about:support, the value for "Multiprocess windows")?
Flags: needinfo?(romain.fliedel)

Comment 2

2 years ago
Blake, would you be able to (find someone to) look at these crashes and get somewhere? The stacks look... odd... to me. The release and nightly stacks are very different, and the signature crash-stats gives is different from the one in the stack it gives for the "crashing thread". Signatures are in websocket IPC land. Smells funny.
Flags: needinfo?(mrbkap)
Reporter

Comment 3

2 years ago
> These crashes look like IPC crashes (ie related to cross-process
> communication). Is there some chance that when you tested on beta, e10s was
> off (check in about:support, the value for "Multiprocess windows")?

I have "Multiprocess Windows 1/1 (Enabled by default)", basically I haven't changed any default setting.
Reporter

Updated

2 years ago
Flags: needinfo?(romain.fliedel)
EXCEPTION_BREAKPOINT usually means we've inserted an intentional abort, to keep this from turning into a security bug.
IPC crash reporting has been changed between 51 and 53 so the signatures will look different. IPCError-browser type crash usually means content process sent some malformed value to parent and parent in turn killed the content process. Crash-stats only displays the stacks from content process when it was killed but for IPCError-browser crash type we also generate a minidump for the parent process which usually contains the line of code where we decided to kill the content process.

The only point we report a failure from WebSocketChannelParent::RecvSendBinaryStream is here:

http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/netwerk/protocol/websocket/WebSocketChannelParent.cpp#
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> IPC crash reporting has been changed between 51 and 53 so the signatures
> will look different. IPCError-browser type crash usually means content
> process sent some malformed value to parent and parent in turn killed the
> content process. Crash-stats only displays the stacks from content process
> when it was killed but for IPCError-browser crash type we also generate a
> minidump for the parent process which usually contains the line of code
> where we decided to kill the content process.
> 
> The only point we report a failure from
> WebSocketChannelParent::RecvSendBinaryStream is here:
> 
> http://searchfox.org/mozilla-central/rev/
> 30fcf167af036aeddf322de44a2fadd370acfd2f/netwerk/protocol/websocket/
> WebSocketChannelParent.cpp#

I mean http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/netwerk/protocol/websocket/WebSocketChannelParent.cpp#188
Andrea owns blobs and slices, I think.
Flags: needinfo?(mrbkap) → needinfo?(amarchesini)
Assignee

Updated

2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee

Comment 8

2 years ago
We crash because we create a nsIInputStream from a Blob/File, and this nsIInputStream is a nsPartialFileInputStream or a nsFileInputStream. The use of File Descriptors in the child process is not allowed because it means that we end up doing I/O somehow on the parent process. There is a specific assertion of the number of FD are use when we serialize a nsIInputStream and it _must_ be 0.

All of this happens because we have several calls of File::CreateFromFile or File::CreateFromNsIFile in our codebase.
I'm removing all of these, moving the creation of File in the parent process.
Assignee

Comment 9

2 years ago
Posted patch part 1 - Entries API (obsolete) — Splinter Review
Assignee

Comment 11

2 years ago
Attachment #8829553 - Attachment is obsolete: true
Attachment #8830304 - Flags: review?(bugs)
Assignee

Comment 12

2 years ago
Attachment #8830305 - Flags: review?(bugs)
Assignee

Comment 13

2 years ago
We should avoid the creation of BlobImplFile objects in the content process because this class ends up doing I/O: the crash happens because there is a security check about having or not FileDescriptors involved in the serialization of a nsIInputStream in the content process. BlobImplFile, of course, creates a nsIFileInputStream and this is not allowed in the content process (or at least in the IPC serialization).

These patches (and more are coming) fix this issue.

1. The first part is about moving the creation of File objects in the parent process in the Entries API. In this way, the child process receives a RemoteImpl object and everything works as it should.

2. The second patch is about make File.createFromNsIFile and File.createFromFileName to work in the content process. This is done using a Promise. This second patch is just about fixing tests and gecko code.

3. Third patch is the real implementation of File.createFromNsIFile and File.createFromFileName.

The next patches are about: DataTransferItem, nsIDOMWindowUtils::WrapDOMFile and other details.
Assignee

Updated

2 years ago
Duplicate of this bug: 1333973
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  0f78485918febd289707f3638bf70af257dbf54a
>
>diff --git a/dom/filesystem/CreateFileTask.cpp b/dom/filesystem/CreateFileTask.cpp
>--- a/dom/filesystem/CreateFileTask.cpp
>+++ b/dom/filesystem/CreateFileTask.cpp
>@@ -135,20 +135,21 @@ CreateFileTaskChild::GetRequestParams(co
> void
> CreateFileTaskChild::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
>                                              ErrorResult& aRv)
> {
>   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> 
>   const FileSystemFileResponse& r = aValue.get_FileSystemFileResponse();
> 
>-  aRv = NS_NewLocalFile(r.realPath(), true, getter_AddRefs(mTargetPath));
>-  if (NS_WARN_IF(aRv.Failed())) {
>-    return;
>-  }
>+  RefPtr<BlobImpl> blobImpl =
>+    static_cast<BlobChild*>(r.blobChild())->GetBlobImpl();
>+  MOZ_ASSERT(blobImpl);
>+
>+  mCreatedFile = File::Create(mFileSystem->GetParentObject(), blobImpl);
> }
Feels too complicated to store mCreatedFile here, when HandlerCallback is called just afterwards
CreateFileTaskChild::HandlerCallback().

Could we either just reuse CreateFileTaskChild's mBlobImpl here or pass FileSystemResponseValue* to 
HandlerCallback in success case? I guess mBlobImpl is kind of input and mCreatedFile output

Though, looks like GetDirectoryListingTaskChild::SetSuccessRequestResult has similar setup. But consider if we could simplify this.


>   RefPtr<BlobImpl> mBlobImpl;
>+  RefPtr<File> mCreatedFile;
And mBlobImpl definitely needs some documentation, and certainly also mCreatedFile if it is actually needed.



>diff --git a/dom/filesystem/GetDirectoryListingTask.cpp b/dom/filesystem/GetDirectoryListingTask.cpp
>--- a/dom/filesystem/GetDirectoryListingTask.cpp
>+++ b/dom/filesystem/GetDirectoryListingTask.cpp
>@@ -89,47 +89,72 @@ GetDirectoryListingTaskChild::GetRequest
>   mFileSystem->AssertIsOnOwningThread();
> 
>   nsAutoString path;
>   aRv = mTargetPath->GetPath(path);
>   if (NS_WARN_IF(aRv.Failed())) {
>     return FileSystemGetDirectoryListingParams();
>   }
> 
>+  nsAutoString directoryPath;
>+  mDirectory->GetPath(directoryPath, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return FileSystemGetDirectoryListingParams();
>+  }
>+
>   return FileSystemGetDirectoryListingParams(aSerializedDOMPath, path,
>-                                             mFilters);
>+                                             directoryPath, mFilters);
Why we need both path and directoryPath?
Please add a comment
Attachment #8830304 - Flags: review?(bugs) → review+
Assignee

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → DOM
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8830303 [details] [diff] [review]
part 2 - async File::CreateFromNsIFile/CreateFromFileName

>@@ -239,16 +223,28 @@ FilePicker.prototype = {
>       msg.mode = "mimeType";
>       msg.mimeType = this._mimeTypeFilter;
>     }
> 
>     EventDispatcher.instance.sendRequestForResult(msg).then(file => {
>       this._filePath = file || null;
>       this._promptActive = false;
> 
>+      if (!file) {
>+        return null;
>+      }
>+
>+      if (this._domWin) {
>+        return this._domWin.File.createFromNsIFile(file);
>+      }
>+
>+      return File.createFromNsIFile(file);
>+    }).then(domFile => {
>+      this._domFile = domFile;
>+
>       if (this._callback) {
>         this._callback.done(this._filePath ?
>             Ci.nsIFilePicker.returnOK : Ci.nsIFilePicker.returnCancel);
Shouldn't you returnCancel if domFile is null or createFromNsIFile promise is rejected

>+        Promise.all(promises).then(function() {
>+          aMessage.target
>+                  .QueryInterface(Ci.nsIFrameLoaderOwner)
>+                  .frameLoader
>+                  .messageManager
>+                  .sendAsyncMessage("SpecialPowers.FilesCreated", filePaths);
What if some Promise fails, shouldn't you handle error case too?

>+++ b/toolkit/crashreporter/CrashSubmit.jsm
Might be worth someone familiar with this code to look at this too.
I guess in general I'd prefer some toolkit/ peer to take a look at changes to toolkit/*
>+      function readFile(aFile) {
>+        let file = aFile;
>+        try {
>+          if (file instanceof Ci.nsILocalFile) {
>+            if (!file.exists())
>+              throw new Error("The file pointed by aFile does not exist");
> 
>-          file = File.createFromNsIFile(file);
>+            File.createFromNsIFile(file).then(function(aFile) {
>+              readFile(aFile);
This is super weird code. first, aFile points initially to nsIFile but later, same named variable points to (DOM)File. Then, readFile is called
first with nsIFile and later with File.
Please split this to two functions.

Those fixed and get some toolkit peer to look at this too.
Attachment #8830303 - Flags: review?(bugs) → review+
Comment on attachment 8830305 [details] [diff] [review]
part 3 - File created on the parent side


> File::CreateFromFileName(const GlobalObject& aGlobal,
>                          const nsAString& aData,
>                          const ChromeFilePropertyBag& aBag,
>                          ErrorResult& aRv)
> {
>   if (!nsContentUtils::ThreadsafeIsCallerChrome()) {
>     aRv.ThrowTypeError<MSG_MISSING_ARGUMENTS>(NS_LITERAL_STRING("File"));
>     return nullptr;
>   }
> 
>-  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports());
>-
>-  RefPtr<MultipartBlobImpl> impl = new MultipartBlobImpl(EmptyString());
>-  impl->InitializeChromeFile(window, aData, aBag, aRv);
>-  if (aRv.Failed()) {
>-    return nullptr;
>-  }
>-  MOZ_ASSERT(impl->IsFile());
>-
>-  if (aBag.mLastModified.WasPassed()) {
>-    impl->SetLastModified(aBag.mLastModified.Value());
>-  }
>-
>-  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
>-  RefPtr<Promise> promise = Promise::Create(global, aRv);
>+  nsCOMPtr<nsIFile> file;
>+  aRv = NS_NewLocalFile(aData, false, getter_AddRefs(file));
>   if (NS_WARN_IF(aRv.Failed())) {
>     return nullptr;
>   }
So calling NS_NewLocalFile is still ok in child process? Wouldn't it be better to just send the filename to the parent and get blob back?
But I guess this is fine too, since we anyhow create the blobimpl in parent

Could you please rename aData to aPath or aFilePath or such, since it is not data.


>+  // Content process.
>+
>+  RefPtr<FileCreatorHelper> helper = new FileCreatorHelper(promise, window);
>+  helper->SendRequest(aData, aBag, aIsFromNsIFile, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+
Please add a comment what keeps helper alive and when it is released
    

>+     async FileCreationRequest(nsID aID, nsString aPath, nsString aType,
>+                               nsString aName, bool lastModifiedPassed,
>+                               int64_t lastModified, bool aIsFromNsIFile);
>+
It is very much unclear (without reading callers) what kind of path aPath is. Is it absolute path in file system or DOM path or what?
Attachment #8830305 - Flags: review?(bugs) → review+

Comment 18

2 years ago
Comment on attachment 8830303 [details] [diff] [review]
part 2 - async File::CreateFromNsIFile/CreateFromFileName

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

Is this going to have add-on impact?

::: browser/components/migration/MSMigrationUtils.jsm
@@ +563,5 @@
> +        }
> +      };
> +      fileReader.addEventListener("loadend", onLoadEnd);
> +      fileReader.readAsText(aFile);
> +    });

This should call the callback (with false) if the promise is rejected, too.


Why does this code need to change, anyway? It already runs in the parent process.
Because File.createFromNsIFile API changes
Assignee

Comment 20

2 years ago
> Is this going to have add-on impact?

There is only 1 addon using File.createFromNsIFile. That addon needs to be updated.
 
> This should call the callback (with false) if the promise is rejected, too.

Done.

> Why does this code need to change, anyway? It already runs in the parent
> process.

Because File.createFromNsIFile was exposed to parent and content process. But in content process it was doing the wrong thing (it was creating a Blob from a nsIFile, but that kind of blob/file can exist only on the parent process because only the parent process is allowed to do I/O).
The new API works everywhere because it returns a Promise and it can be used safely everywhere.
Assignee

Comment 21

2 years ago
Attachment #8831618 - Flags: review?(bugs)
Assignee

Updated

2 years ago
Attachment #8831618 - Attachment is obsolete: true
Attachment #8831618 - Flags: review?(bugs)

Comment 23

2 years ago
Comment on attachment 8831667 [details] [diff] [review]
part 4 - get rid of wrapDOMFile (and refactoring of MockFilePicker.returnFiles)

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

::: devtools/client/webide/test/test_simulators.html
@@ +189,5 @@
>            is(form.name.value, customName + "2.0", "Simulator name was updated to new version");
>  
>            // Pick custom binary, but act like the user aborted the file picker.
>  
> +          MockFilePicker.setFiles([]);

This is a no-op now. It doesn't remove the existing files or anything like that. Does this need specialcasing in the setFiles code?

@@ +197,5 @@
>            ok(!form.version.classList.contains("custom"), "Version selector is not customized after customization abort");
>  
>            // Pick custom binary, and actually follow through. (success, verify value = "custom" and textContent = custom path)
>  
> +          yield MockFilePicker.useAnyFile();

As noted elsewhere, this doesn't return a promise so adding the yield doesn't achieve anything.

::: dom/file/File.cpp
@@ +602,5 @@
>                           ErrorResult& aRv)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!nsContentUtils::IsCallerChrome()) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Why the change to how we throw exceptions here? The previous code looks like it produces the correct style DOM error, whereas this looks like it might expose NS_ERROR_FAILURE to web code, which shouldn't happen.

Also, why the switch to a debug-only assert? What happens if we call IsCallerChrome on non-main-thread?

This should probably be reviewed by a DOM peer.

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +239,5 @@
> +      Promise.all(MockFilePicker.pendingPromises).then(() => {
> +        // Nothing else has to be done.
> +        MockFilePicker.pendingPromises = [];
> +
> +        let result = Components.interfaces.nsIFilePicker.returnCancel;

This is unused now. That doesn't seem right.


It looks like the semantics here changed because the try...catch is now around a slightly different set of code.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
@@ +34,5 @@
>    const manifest = {background, manifest: {permissions: ["downloads"]}};
>    const extension = ExtensionTestUtils.loadExtension(manifest);
>  
>    MockFilePicker.init(window);
> +  yield MockFilePicker.useAnyFile();

This doesn't do anything, as useAnyFile does not return a promise.

::: toolkit/components/filepicker/nsFilePicker.js
@@ +232,5 @@
> +        };
> +
> +        for (let i = 0; i < this.mFilesEnumerator.mFiles.length; ++i) {
> +          if (this.mFilesEnumerator.mFiles[i].exists()) {
> +            promises.push(this.mParentWindow.File.createFromNsIFile(this.mFilesEnumerator.mFiles[i]).then(file => {

Can we make this less long?

@@ +241,3 @@
>        }
> +
> +      promises.all(promises).then(() => {

Eh? promises is an array, so promises.all is undefined. I assume you meant "Promise.all(promises)".
Attachment #8831667 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 24

2 years ago
Attachment #8831667 - Attachment is obsolete: true
Attachment #8831725 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED

Comment 25

2 years ago
Comment on attachment 8831725 [details] [diff] [review]
part 4 - get rid of wrapDOMFile (and refactoring of MockFilePicker.returnFiles)

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

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +120,4 @@
>    },
>  
> +  setFiles(files) {
> +    this.returnData = [];

Should we reset pendingPromises as well?
Attachment #8831725 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Updated

2 years ago
Depends on: 1334975
Assignee

Comment 26

2 years ago
Posted patch part 5 - assertions (obsolete) — Splinter Review
Attachment #8832038 - Flags: review?(bugs)
Comment on attachment 8832038 [details] [diff] [review]
part 5 - assertions


> /* static */ already_AddRefed<File>
> File::CreateFromFile(nsISupports* aParent, nsIFile* aFile)
> {
>+  MOZ_ASSERT(XRE_IsParentProcess());
>   RefPtr<File> file = new File(aParent, new BlobImplFile(aFile));
>   return file.forget();
> }
> 
> /* static */ already_AddRefed<File>
> File::CreateFromFile(nsISupports* aParent, nsIFile* aFile,
>                      const nsAString& aName, const nsAString& aContentType)
> {
>+  MOZ_ASSERT(XRE_IsParentProcess());
Could we make these MOZ_ASSERTs above MOZ_DIAGNOSTIC_ASSERTs


> File::CreateFromFileName(const GlobalObject& aGlobal,
>                          const nsAString& aPath,
>                          const ChromeFilePropertyBag& aBag,
>                          ErrorResult& aRv)
> {
>-  if (!nsContentUtils::ThreadsafeIsCallerChrome()) {
>-    aRv.ThrowTypeError<MSG_MISSING_ARGUMENTS>(NS_LITERAL_STRING("File"));
>+  MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread());
>+  if (!nsContentUtils::IsCallerChrome()) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>     return nullptr;
>   }
This doesn't look right. CreateFromFileName can be called on chrome workers, right?

> 
>+BlobImplFile::BlobImplFile(nsIFile* aFile)
>+  : BlobImplBase(EmptyString(), EmptyString(), UINT64_MAX, INT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
>+{
>+  MOZ_ASSERT(mFile, "must have file");
>+  MOZ_ASSERT(XRE_IsParentProcess());
Shouldn't this be MOZ_DIAGNOSTIC_ASSERT

>+BlobImplFile::BlobImplFile(const nsAString& aName,
>+                           const nsAString& aContentType,
>+                           uint64_t aLength, nsIFile* aFile)
>+  : BlobImplBase(aName, aContentType, aLength, UINT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
>+{
>+  MOZ_ASSERT(mFile, "must have file");
>+  MOZ_ASSERT(XRE_IsParentProcess());
ditto

>+BlobImplFile::BlobImplFile(const nsAString& aName,
>+                           const nsAString& aContentType,
>+                           uint64_t aLength, nsIFile* aFile,
>+                           int64_t aLastModificationDate)
>+  : BlobImplBase(aName, aContentType, aLength, aLastModificationDate)
>+  , mFile(aFile)
>+  , mWholeFile(true)
>+{
>+  MOZ_ASSERT(mFile, "must have file");
>+  MOZ_ASSERT(XRE_IsParentProcess());
and here


>+BlobImplFile::BlobImplFile(nsIFile* aFile, const nsAString& aName,
>+                           const nsAString& aContentType)
>+  : BlobImplBase(aName, aContentType, UINT64_MAX, INT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
>+{
>+  MOZ_ASSERT(mFile, "must have file");
>+  MOZ_ASSERT(XRE_IsParentProcess());
and here

> MultipartBlobImpl::InitializeChromeFile(nsIFile* aFile,
>                                         const nsAString& aType,
>                                         const nsAString& aName,
>                                         bool aLastModifiedPassed,
>                                         int64_t aLastModified,
>                                         bool aIsFromNsIFile)
> {
>+  MOZ_ASSERT (XRE_IsParentProcess());
MOZ_DIAGNOSTIC_ASSERT
Attachment #8832038 - Flags: review?(bugs) → review-
Assignee

Comment 28

2 years ago
The bug can be fixed just landing patch 1. All the rest is nice to have and important to avoid other issues in gecko.
I suggest to land patch 1 and the rest in a separate bug.
Flags: needinfo?(bugs)
yeah, two separate bugs would make tracking easier. 
Not sure whether to use this bug to land part 1 or have separate bug for that and dealing branch patches there too. Whatever makes this easier to understand and track.
Flags: needinfo?(bugs)
Assignee

Updated

2 years ago
Attachment #8830303 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8830305 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8831725 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8832038 - Attachment is obsolete: true
Assignee

Comment 30

2 years ago
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Here we crash because of an assertion + a wrong IPC serialization: on IPC we don't want to do I/O, and this means that we don't want to have BlobImpl objects pointing to real files.
The crash is easy to trigger, but it's not exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, the patch is about moving part of Entries API to the parent process.

Which older supported branches are affected by this flaw?

Entries API is just landed in release. We need this patch everywhere.

If not all supported branches, which bug introduced the flaw?

Entries API.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to create.

How likely is this patch to cause regressions; how much testing does it need?

Not too much. The patch is big because it also covers the DeviceStorage API, not enabled in release. Itself, the patch for the Entries API is simple enough to be safe to land.
Attachment #8830304 - Flags: sec-approval?
Group: core-security → dom-core-security
(In reply to Andrea Marchesini [:baku] from comment #30)
> The crash is easy to trigger, but it's not exploitable.

Unhiding.
Group: dom-core-security
Keywords: crash, testcase
Whiteboard: [sg:dos]
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

Clearing unnecessary sec-approval? request.
Attachment #8830304 - Flags: sec-approval?

Comment 33

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8753c535d7
Entries API must create BlobImpl in the parent process, r=smaug
Assignee

Comment 34

2 years ago
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

Approval Request Comment
[Feature/Bug causing the regression]: Entries API
[User impact if declined]: a crash
[Is this code covered by automated tests?]: no. the existing tests pass.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, follow comment 0. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not too much
[Why is the change risky/not risky?]: We just create BlobImpl in the parent process instead doing it in the content process. The logic is more or less the same.
[String changes made/needed]: none
Attachment #8830304 - Flags: approval-mozilla-beta?
Attachment #8830304 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Duplicate of this bug: 1335524

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea8753c535d7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Hi Andrei,
could you help verify if this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(andrei.vaida)
Forwarding this to Brindusa, who's taking care of requests on the nightly channel.
Flags: needinfo?(andrei.vaida) → needinfo?(brindusa.tot)
I verified this on Windows 10 x32 with FF Nighlty 54.0a1 (2017-01-31) and I can reproduce it. With the latest Nighlty 54.0a1(2017-02-02) the issue is fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Crash Signature: IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing error: message was deserialized → [IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing error: message was deserialized]
Crash Signature: [IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing error: message was deserialized] → [@ IPCError-browser | (msgtype=0x11C0005,name=PWebSocket::Msg_SendBinaryStream) Processing error: message was deserialized]
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

Crash fix, let's at least get this onto 53 aurora. baku magic!
Attachment #8830304 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8830304 [details] [diff] [review]
part 1 - Entries API

potential crash fix, has been in aurora for a week, let's try on beta52.
Attachment #8830304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flagging this for verification on 52 and 53.
Flags: qe-verify+
I've reproduced the issue described in comment 0 using old 53.0a1 Nightly (Build Id:20170118030214 , Crash Id: bp-bca74c2c-041c-4a5f-9f94-f292c2170221) on Windows 7 32bit.

I have verified that the issues is not reproducible anymore using Firefox 52 beta 8 (Build ID:20170220070057), latest Developer Edition 53.0a2 (Build ID: 20170221004019) and the latest Nightly 54.0a1 (Build Id:20170220030205) on Windows 7 32bit and Windows 10 x64.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.