Closed Bug 1462979 Opened 6 years ago Closed 6 years ago

POST requests with file upload from Flash plugin fail - connection closed early (Firefox 60 regression)

Categories

(Core :: Security: Process Sandboxing, defect, P2)

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ verified
firefox60 --- wontfix
firefox61 + verified
firefox62 + verified

People

(Reporter: calvin.walton, Assigned: handyman)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180517141400

Steps to reproduce:

Using the 64-bit Windows version Firefox 60 or later, use a flash application that performs file upload via POST request, for example the BigBlueButton client.

For testing, you can use https://demo.bigbluebutton.org/ - create a new meeting and join it, then click the upload icon below the presentation (cloud icon). Choose a document in the 10-300KB range for best chance of triggering the effect.


Actual results:

The upload dialog hangs.

We ended up having to look at the traffic in Wireshark to figure out what was going wrong:

In Firefox 60 or later on 64bit Windows, the TCP connection for the post request is closed by the Flash client before the web server sends the response. Depending on processing time and latency, this may cause the web server (Nginx in this case) to see that the connection has been closed, and it closes its side as well before sending the response.

The result is an application error, since the flash application doesn't receive an HTTP response.

Firefox on other operating systems is not affected, and the 32bit version of Firefox on Windows is not affected.

Note that this works in Firefox 59 with the same flash version installed, which makes us think that this is likely an issue with Firefox, perhaps in the plugin sandboxing?


Expected results:

The upload completes, and the uploaded document is visible.

The TCP connection for the post request should remain open at least until the response from the POST request is received. (It may even stay open longer because of HTTP 1.1 pipelining).
We've set up a new testing server with HTTPS disabled to make it easier to see what's going wrong, here: http://ff60-bug.bigbluebutton.org
(Enter a name in the front page form, and you'll join a unique meeting by yourself)
Note that because this server has no HTTPS, the WebRTC audio might be blocked - this is expected :) There's no reason to join the audio for this test, just click the 'x' box.

I'm attaching some pcap files demonstrating the issue. One is Firefox 59.0 (working), the other is Firefox 61.0b6 (failing). In both cases the Adobe Flash plugin is version 29.0.0.171. In Firefox 59.0, you can see the pipelined HTTP 1.1 requests (it does a preflight request then a real one, both in a single TCP connection). In Firefox 61.0b6, you can see that as soon as it has completed sending the body of the POST request, the client sends a TCP FIN, closing the connection (the web server aborts the request when it receives this).
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=003668afcef5a0919792fc1331d44e6e8c2fbb40&tochange=d94c15bb443556229f6a433c4a3cf775fd342ada

Regressed by : Bug 1382251


dparks,
The bunch of patch causes the regression. Can you please look into this?
Your bunch of patch seems to cause the regression. Can you please look into this issue?
Blocks: 1382251
Status: UNCONFIRMED → NEW
Component: Untriaged → Security: Process Sandboxing
Ever confirmed: true
Flags: needinfo?(davidp99)
Keywords: regression
Product: Firefox → Core
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: -- → P2
not sure it's exactly the same bug, but I can confirm there's a regression since version 60.0
We mainly use Filereference.upload in flash to post mp3 and mp4 files.
I do receive the COMPLETE event from flash, but not the DataEvent.UPLOAD_COMPLETE_DATA which gives the response from the server.

The full request is received and I the response is sent server-side
Hey Calvin,

Your test case was helpful but your site, demo.bigbluebutton.org, now blocks uploads from Firefox 60 and your example site, ff60-bug.bigbluebutton.org, has gone down.  Would it be possible to restore one of these for debugging?
Flags: needinfo?(calvin.walton)
ff60-bug.bigbluebutton.org is back up.  

Thanks David for looking into this issue and don't hesitate to let us know if there is anything we can do to help.
As fred mentioned, the test site is back up.
Flags: needinfo?(calvin.walton)
Thanks for restoring it.  I've got a few leads now on what might be failing.
Jimm, you are probably the only person who can review this but it'll keep until you get back (lucky you).

The reports spell out what failing in some detail.  They point to HttpSendRequest commands succeeding but the connection being closed by the client (InternetCloseHandle) before a response is received.  The API point that would be expected between them is HttpEndRequest, which somewhat unsurprisingly handles the  acknowledge part of the protocol.  It works in other plugins because, I assume, they don't look at transmission errors _after_ the send.  Regardless, this makes it all work.  The http://ff60-bug.bigbluebutton.org/ demo is a very easy way to tell.

We'll also need to figure out what to do in a dot-release -- use this or a simple patch to disable the system (I can just remove the QUIRK_FLASH_HOOK_SSL).  I was going to do the later anyway but this patch is pretty simple.

----

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c32c22f113b468ed68bf95768d151057a6f94e9
Attachment #8981600 - Flags: review?(jmathies)
Printf format fixes some logging I used in debugging.
Attachment #8981611 - Flags: review?(jmathies)
refresh
Attachment #8981600 - Attachment is obsolete: true
Attachment #8981600 - Flags: review?(jmathies)
Attachment #8981612 - Flags: review?(jmathies)
Attachment #8981611 - Flags: review?(jmathies) → review+
Attachment #8981612 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e158f25bd0da
Fix printf formatting in FunctionHook logging. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e920b9c3d553
Broker HttpEndRequestA for plugin process. r=jimm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e158f25bd0da
https://hg.mozilla.org/mozilla-central/rev/e920b9c3d553
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I'm curious about why esr60 is marked as "wontfix" here. Does that mean this fix won't go into a future 60.x ESR release? BigBlueButton is often used in academic settings with managed PCs, so we do see some usage of ESR series browsers.
Lets go ahead and get this uplifted to esr.
Flags: needinfo?(davidp99)
Per comment 17, the reviewer of the patches in the bug has already asked for backport requests to be made. We appreciate the comments in favor, but at this point, the most important information needed is the risk assessment from the engineer who wrote the patches. Please allow the process to proceed without adding more "me too" comments to the bug.
We are definitely attempting beta uplift of this patch and also definitely providing a patch for esr60 -- the discussion has been about what patch to provide for esr60.  I have a patch that I would prefer for esr60 for simplicity, although this could be paranoia.  That part will be resolved in the next day or two.
Flags: needinfo?(davidp99)
Comment on attachment 8981612 [details] [diff] [review]
2. Broker HttpEndRequestA for plugin process

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1382251

[User impact if declined]:
Some sites will fail to operate correctly if they interpret POST responses in Flash plugins

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes.  Calvin's STR in comment 1, using ff60-bug.bigbluebutton.org, is very easy to run.  Of most interest would be testing on a variety of versions of Windows.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Yes -- medium

[Why is the change risky/not risky?]:
The patch is small but what it does is complex.  The patch uses DLL interception of a Win32 function, which has worked for this function in every run we've seen but there has been a mysterious (and very rare) bug where some system DLLs have different instructions for some of the other APIs we intercept -- which shouldn't happen.  I am not seriously concerned about this.

[String changes made/needed]:
None
Attachment #8981612 - Flags: approval-mozilla-beta?
I can confirm that the issue no longer occurs on the latest Nightly 62.0a1 (2018-06-11) (64-bit) - 20180611220254 - across Windows 10 x64, Ubuntu 16.04, MacOS High Sierra 10.13. platforms.
By following the steps provided in Comment 1, I successfully uploaded a 100kb sized PDF document without any problems. 

Flash Version: 29.0 r0
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8981612 [details] [diff] [review]
2. Broker HttpEndRequestA for plugin process

Fixes a pretty severe regression with Flash-based file uploads. Verified on Nightly. Approved for 61.0b14.
Attachment #8981612 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Rebased to beta as requested.  There was a somewhat cosmetic type change in nightly that was blocking this patches application in beta -- I manually updated the code in this patch so it uses the old (valid in beta) type.

I haven't built yet but I'm pretty confident this build will be green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee819bbaf54f0f2512e305f3b3b974123325f3c5
Flags: needinfo?(davidp99) → needinfo?(ryanvm)
Comment on attachment 8985083 [details] [diff] [review]
Broker HttpEndRequestA for plugin process - Beta rebase

Probably best to just re-use the beta approval flag.  For beta notes, see comment 24
Flags: needinfo?(ryanvm)
Attachment #8985083 - Flags: approval-mozilla-beta?
Attachment #8981612 - Flags: approval-mozilla-beta+
Comment on attachment 8985083 [details] [diff] [review]
Broker HttpEndRequestA for plugin process - Beta rebase

Thanks for rebasing.
Attachment #8985083 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I was able to reproduce this issue on Firefox 61.0b6, under Windows 10 x64, by using the steps and the Flash version mentioned in comment 1. 
I can confirm, that this behavior no longer occurs on Firefox 61.0b14, under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64 platforms.
Flags: qe-verify+
This is the lower-risk patch, based to current esr60.  It simply removes the quirk that controls the remoting of these methods (QUIRK_FLASH_HOOK_SSL).  I've tried this build and the STR in comment 1 works for me:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd5720cc805d2629f1a8401aeebbd064fcdfe99
Attachment #8986057 - Flags: review?(jmathies)
Attachment #8986057 - Attachment description: Broker HttpEndRequestA for plugin process - esr60 version → Do not hook network functions in plugin process - esr60 version
Attachment #8986057 - Flags: review?(jmathies) → review+
Comment on attachment 8986057 [details] [diff] [review]
Do not hook network functions in plugin process - esr60 version

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Disables a feature that isn't quite working right.
User impact if declined: 
buggy flash
Fix Landed on Version:
all current releases
Risk to taking this patch (and alternatives if risky): 
low, restores previous behavior and code paths
String or UUID changes made by this patch: 
n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8986057 - Flags: approval-mozilla-esr60?
Comment on attachment 8986057 [details] [diff] [review]
Do not hook network functions in plugin process - esr60 version

disable some flash sandboxing to fix a regression, approved for 60.1esr
Attachment #8986057 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
I can confirm this issue is fixed also, on Firefox  60.1.0esr (20180619173714), under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64 platforms.
Flags: qe-verify+
Hi Everyone,

I just want to say a big THANKS from the BigBlueButton project to everyone's work to fix this in FireFox 60.1.0esr and FireFox 61.  Really impressed with your responsiveness and speed.  FireFox (and Mozilla) rocks!

Regards,... Fred
BigBlueButton Project Manager
Also, we've turned off http://ff60-bug.bigbluebutton.org/, but let us know if you still need it and we'll spin it up again.
You need to log in before you can comment on or make changes to this bug.