1532530 - Use of webRequest.onBeforeRequest with 'requestBody' causes large uploads to stall intermittently

Status: VERIFIED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Erik Moeller]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

1. Create an extension directory

2. Create the following file as main.js: https://hastebin.com/onikurixif.js

3. Create the file as manifest.json: https://hastebin.com/ixibecozaw.json

4. Import the extension in debug mode

5. Upload a large file (>150MB) through a simple multipart/form-data form, e.g., run app.py from https://github.com/micahflee/noscript-upload-bug via python3 app.py and upload through its form through localhost:5000 .

Actual results:

On the nth try (for me often on the first try, but it is intermittent), the upload will stall out just before completion.

Expected results:

The upload should complete normally.

Comment 1

[Erik Moeller]

It's possible that this is related to: https://bugzilla.mozilla.org/show_bug.cgi?id=1506562

Note, however, that there are no errors on the console, and that this does not require the use of XMLHttpRequest. I'm unable to reproduce the issue when the requestBody option is not set.

This bug is causing major problems for the Tor Browser ecosystem, including projects like SecureDrop and OnionShare. The reason is that the NoScript extension, which ships with Tor Browser by default, uses a feature called "Sanitize cross-site suspicious requests". This feature, in turn, uses exactly such an onBeforeRequest listener.

Effectively, this means that non-JavaScript uploads -- especially large uploads -- break intermittently for users of Tor browser in the default configuration.

The relevant downstream issue in NoScript is: https://github.com/hackademix/noscript/issues/64

The relevant downstream issue in SecureDrop is: https://github.com/freedomofpress/securedrop/issues/4078

The relevant downstream issue in OnionShare is: https://github.com/micahflee/onionshare/issues/899

All these issues are closed, but that's only because SecureDrop and OnionShare have implemented instructions to turn off the NoScript feature, and NoScript considers it entirely an upstream problem.

Comment 2

[Giorgio Maone [:ma1]]

This is causing the Tor Browser to temporarily disable the XSS filter, which seems very unfortunate to me:
https://trac.torproject.org/projects/tor/ticket/29733

Any chance for this to be looked into soon? Thanks!

Comment 3

[Rob Wu [:robwu]]

[Explanation of bug for those who are interested; I will attach a test case for QA in the next comment]

The links from the original report are broken, so I created a test extension.
To rule out any server-side influences, I also created a server that just shows the number of uploaded bytes (without parsing the form data).

Observations:

• I can reproduce on Firefox 65.0.2 and Firefox Nightly 68.0a1 buildID 20190321104132

• The bug is more likely to occur for larger files.

• I can barely reproduce the problem on localhost. Hosting the server on a different computer in my network with an upload speed capped at 80 Mbps increases the rate of reproduction (e.g. reproduced with a 100 MB file at localhost, but failed to reproduce with 4 MB at localhost after 1.8 million attempts).

• The number of missing bytes depends on the size of the file.

• The number of missing bytes is consistent for uploads of the same size.

• The number of missing bytes seems to be around 64 (ranging from 62 to 65).
  In Wireshark, I can see that the file itself has completely been uploaded, but the closing boundary of the multipart message is missing.

Example of failed request (test file created using seq -f "%9.f" 1 1000000):

POST / HTTP/1.1
Host: 192.168.2.1:5000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Content-Type: multipart/form-data; boundary=---------------------------8612789758546048021637194161
Content-Length: 10000233
Connection: keep-alive
Referer: http://192.168.2.1:5000/
Upgrade-Insecure-Requests: 1

-----------------------------8612789758546048021637194161
Content-Disposition: form-data; name="file"; filename="tenfile.bin"
Content-Type: application/octet-stream

        1
        2
...

   999999
  1000000

The size of the above request body is 10 000 171, 62 bytes are missing. Namely the following:

-----------------------------8612789758546048021637194161--

The chunk is supposedly generated by FSMultipartFormData::GetSubmissionBody and appended as a separate stream to the list of upload streams.

When I add an input element, <input name="afterfile" value="afterfile">, then I can see it in the details.requestBody object of the webRequest.onBeforeRequest event, which suggests that the data loss is caused by the request body extraction of the webRequest API.

And indeed, the request body parser of WebRequestUpload.jsm passes the input stream to a ConverterInputStream, and assumes that the input stream is not mutated except for the seek offset.
This assumption is incorrect; when a nsConverterInputStream is destructed / garbage-collected, the input stream is cleared.
WebRequestUpload.jsm also uses nsBinaryInputStream, but that doesn't appear to invalidate the input stream upon destruction.

I'm going to look into the best way to fix it. A possible way to fix it is to simply not call Close() in the nsConverterInputStream destructor; another potential fix is to stop using nsConverterInputStream since it's deprecated anyway.

Comment 4

[Rob Wu [:robwu]]

Attachment: server.js

A server that shows the received number of bytes.

To use it, simply run node server.js

then visit http://localhost:5000 .

Comment 5

[Rob Wu [:robwu]]

Attachment: manual-reproduction-extension.zip

STR:

1. Start a server with an upload form, for example comment 4.
2. Start Firefox, visit about:debugging and:
   • Load the attached extension
   • Click on the Debug button at the extension, and switch to the Console tab to see debugging output.
3. Open a page from where you can upload a file.
   For example, run the server from comment 4
4. Visit the URL of the server that has the upload form, e.g. http://localhost:5000/
5. Click on the file input and select any file.
6. Click on the "Upload" button.
7. Visit about:memory and click on "Minimize memory usage".
8. Click on the extension button in the browser (which resumes the upload request).
9. Look at the tab from where you started the upload form.
10. Look at the console (from step 2).

Expected:

• At step 9, the tab should no longer be busy / uploading.
• At step 10, the output should be:

 POST http://localhost:5000/
 Waiting for button click to resume request
 Resumed request
 Request completed

Actual:

• At step 9, the tab appears to be busy and still be uploading something.
• At step 10, the last line (Request completed) is missing.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1532530 - Avoid loss of upload data by webRequest API

When an extension requests access to the request body of a request,
nsConverterInputStream is used to parse the input streams that make up
a request body. These input streams are later (re)used to upload the
form data to the original destination (server).

nsConverterInputStream's destructor does however close the input
streams, which results in data loss when the object is garbage-collected
before the upload completes.

This patch fixes the issue by explicitly nulling the underlying stream
before returning from the form parser.

Comment 7

[Rob Wu [:robwu]]

See comment 5 for STR.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/574c141c8dd6
Avoid loss of upload data by webRequest API r=kmag

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/574c141c8dd6

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 9052943 [details]
Bug 1532530 - Avoid loss of upload data by webRequest API

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1201979
• User impact if declined: Form submissions involving files may fail to complete if the user has an extension that read the request body via the webRequest API. This failure is non-deterministic and triggered by garbage collection.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 5.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is minimal and only alters the behavior of the affected code, without side effects.
• String changes made/needed: N/A

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tor is built upon ESR, and the NoScript extension is installed by default in the Tor browser. All Tor browser users may experience failures to upload files, especially if the files are large and/or if the network is slow. The XSS protection feature of NoScript is currently disabled to work around this bug - https://trac.torproject.org/projects/tor/ticket/29733
• User impact if declined: Tor browser users may experience failures in file uploads, or they have to disable a security feature of NoScript.
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is minimal and only alters the behavior of the affected code, without side effects.
• String or UUID changes made by this patch: N/A

Comment 11

[Rob Wu [:robwu]]

I already verified using the steps in comment 5, but as I'm the developer of the patch, that doesn't really count and QA should verify too.

Comment 12

[CosminB]

Attachment: Bug1532530.zip

This issue is verified as fixed on Firefox 68.0a1 (20190326214944) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached screenshots.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9052943 [details]
Bug 1532530 - Avoid loss of upload data by webRequest API

P1 and fixes a dataloss bug, verified by QA on Nightly, issue is well understood and patch is minimal -> uplift approved for 67 beta 6, thanks!

Let's have QA also verify after it lands on Beta.

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/85efc85e2cf3

Comment 15

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/574c141c8dd619a00ffa90fc40c2ba614afb46da
Bug 1532530 - Avoid loss of upload data by webRequest API r=kmag

Comment 16

[CosminB]

Attachment: Bug1532530-Beta.gif

This issue is verified as fixed on Firefox 67.0b6 (20190328152334) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.

Comment 17

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

Comment on attachment 9052943 [details]
Bug 1532530 - Avoid loss of upload data by webRequest API

Verified in beta 67. Let's take this for 60.7.0esr as well.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/90a634a74145

Comment 19

[CosminB]

Attachment: Bug1532530.gif

This issue is verified as fixed on Firefox 60.6.2esr (20190416010130) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.