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

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Romain FLIEDEL]

Attachment: 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

Comment 1

[:Gijs (out for now; he/him)]

(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")?

Comment 2

[:Gijs (out for now; he/him)]

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.

Comment 3

[Romain FLIEDEL]

> 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.

Comment 4

[Daniel Veditz [:dveditz]]

EXCEPTION_BREAKPOINT usually means we've inserted an intentional abort, to keep this from turning into a security bug.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

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#

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

(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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Andrea owns blobs and slices, I think.

Comment 8

[Andrea Marchesini [:baku]]

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.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - Entries API

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - async File::CreateFromNsIFile/CreateFromFileName

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - Entries API

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 3 - File created on the parent side

Comment 13

[Andrea Marchesini [:baku]]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># 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

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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?

Comment 18

[:Gijs (out for now; he/him)]

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.

Comment 19

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Because File.createFromNsIFile API changes

Comment 20

[Andrea Marchesini [:baku]]

> 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.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 4 - get rid of wrapDOMFile

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 4 - get rid of wrapDOMFile (and refactoring of MockFilePicker.returnFiles)

Comment 23

[:Gijs (out for now; he/him)]

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)".

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 4 - get rid of wrapDOMFile (and refactoring of MockFilePicker.returnFiles)

Comment 25

[:Gijs (out for now; he/him)]

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?

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 5 - assertions

Comment 27

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

Comment 28

[Andrea Marchesini [:baku]]

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.

Comment 29

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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.

Comment 30

[Andrea Marchesini [:baku]]

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.

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to Andrea Marchesini [:baku] from comment #30)
> The crash is easy to trigger, but it's not exploitable.

Unhiding.

Comment 32

[Daniel Veditz [:dveditz]]

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

Clearing unnecessary sec-approval? request.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8753c535d7
Entries API must create BlobImpl in the parent process, r=smaug

Comment 34

[Andrea Marchesini [:baku]]

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

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 37

[Gerry Chang [:gchang]]

Hi Andrei,
could you help verify if this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 38

[Andrei Vaida [:avaida]]

Forwarding this to Brindusa, who's taking care of requests on the nightly channel.

Comment 39

[ovidiu boca[:Ovidiu]]

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.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Crash fix, let's at least get this onto 53 aurora. baku magic!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f9e9f92c6d4

Comment 42

[Julien Cristau [:jcristau]]

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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/af4cb99b8c76

Comment 44

[Andrei Vaida [:avaida]]

Flagging this for verification on 52 and 53.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/af4cb99b8c76

Comment 46

[Emil Ghitta, QA [:emilghitta]]

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.