Bug 1436241 (CVE-2018-12364)

Firefox allows flash to follow 307 redirects to other origins with arbitrary content-types

VERIFIED FIXED in Firefox -esr52

Status

()

defect
VERIFIED FIXED
a year ago
2 days ago

People

(Reporter: dblack, Assigned: qdot, NeedInfo)

Tracking

({sec-high})

Trunk
mozilla62
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox-esr5261+ fixed, firefox-esr6061+ fixed, firefox59 wontfix, firefox60+ wontfix, firefox61+ verified, firefox62+ verified)

Details

(Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+])

Attachments

(4 attachments)

Reporter

Description

a year ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.119 Safari/537.36

Steps to reproduce:

Used https://github.com/sp1d3r/swf_json_csrf in latest available version of flash to send a post request cross-domain with a non-simple content type.


Actual results:

The request is sent in firefox.


Expected results:

The request should either not be sent or the content-type should not be allowed to be a non-simple content-type without a cors preflight request being made.
Might be a regression from bug 957403
See Also: → 957403

Comment 3

a year ago
Hi David, could you please provide clearer steps to reproduce this issue?
Flags: needinfo?(dblack)
OS: Unspecified → Linux
Hardware: Unspecified → x86_64

Comment 4

a year ago
Closing as Resolved:Incomplete due to no update from reporter. Please feel free to reopen the bug if issue still exists for you even with fresh profile and safe mode.
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Resolution: --- → INCOMPLETE
Reporter

Comment 5

a year ago
Hi Hani,
to reproduce this issue all you need to do is host the swf & html (test.html) file found in the repository at https://github.com/sp1d3r/swf_json_csrf (test.swf) using a webserver. You need to have php enabled if that's difficult or undesired (with regards to test.php) but you need to make the web server return with a 307 redirect when /test.php is requested. The 307 redirect needs to go to the location that you wish to send a non-simple content-type to using flash.
Flags: needinfo?(dblack)
Reporter

Comment 6

a year ago
Instead of using php I modified a copy ofthe python 2 SimpleHTTPServer.py file like so ->

--- SimpleHTTPServer.py.orig	2018-03-01 21:07:12.875014627 +1100
+++ SimpleHTTPServer.py	2018-03-01 21:06:47.455098696 +1100
@@ -49,6 +49,9 @@
             finally:
                 f.close()
 
+    def do_POST(self):
+        return self.do_GET()
+
     def do_HEAD(self):
         """Serve a HEAD request."""
         f = self.send_head()
@@ -68,6 +71,12 @@
         """
         path = self.translate_path(self.path)
         f = None
+        if '.php' in path:
+            self.send_response(307)
+            self.send_header("Location", 'https://example_location_here')
+            self.end_headers()
+            return None
+
         if os.path.isdir(path):
             parts = urlparse.urlsplit(self.path)
             if not parts.path.endswith('/'):
@@ -98,6 +107,7 @@
         try:
             self.send_response(200)
             self.send_header("Content-type", ctype)
+            self.send_header('Content-Disposition', 'inline')
             fs = os.fstat(f.fileno())
             self.send_header("Content-Length", str(fs[6]))
             self.send_header("Last-Modified", self.date_time_string(fs.st_mtime))
Ah, why was this closed? The steps in comment 0 are perfectly clear?
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INCOMPLETE → ---
FYI this is a security related bug - its only not private, since this is a publicly documented issue with flash/firefox.
Keywords: sec-other
Last couple of times we saw this bug Adobe had to fix it on their end (bug 957403 comment 22). We should verify that the NPAPI mechanism set up in bug 573873 didn't get damaged.

This is not a direct Firefox vulnerability, but it puts users at risk on web sites. If we have to we could just ban plugins from using redirects at all, or treat 307 redirects for plugins as 302 or even just an error. Flash is already click-to-play by default we can break it a bit more if that's the only option.
Component: Untriaged → Flash (Adobe)
Keywords: sec-othersec-high
Product: Firefox → External Software Affecting Firefox
Version: 60 Branch → unspecified
Andrew: who is responsible for plugin code these days?
Flags: needinfo?(overholt)
Nominally qDot
Flags: needinfo?(overholt) → needinfo?(kyle)
Not a hugely high priority issue, but is a publicly known issue (see the numerous blog posts linked a the bottom of the readme linked in comment 1). So we either need to fix, or decide that click-to-play is enough protection (which might be reasonable, given the attack vector here is a 3rd party site running flash), and thus that this is a wontfix.
We have code to explicitly stop cross-domain 307 POSTs (https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginStreamListenerPeer.cpp#695, part of which came in with bug 573873, as mentioned in comment 9), so the fact that block isn't running and catching this is a little worrisome. I'm not sure whether that's due to flash or us though.
Flags: needinfo?(kyle)
Assignee: nobody → kyle
Ok, did some research.

While we have a 307 check in the NPAPI redirect function mentioned in comment #13, it comes after we call HandleRedirectNotification. In the case of this bug, HandleRedirectNotification will handle the redirect itself and return true, meaning we never actually get to the redirect status code check.

Naive solution is to just stick the 307 check before the HandleRedirectNotification call, but I'm out of my depth in terms of what's happening around channels and necko here, so I'm going to try to hunt down someone who understands necko/channels/redirects better than I do, to discuss what the best solution is.
From what I can tell from checking commit logs, it looks like the fix for bug 573873 came first (2010-12), then was nullified by bug 475991 a month later (2011-01). Bug 475991 added the channel notification handling before the 307 same origin check made by bug 573873. Both flash and our own np_test plugin handle redirects (reflecting them to the browser), so we are trusting that the plugin does a 307 check during its redirect handling, which none of them actually do. I think we can just put our check before sending redirects out to the plugin now, and call it done?
Doing some more reading while I wait for a try run... It appears that letting plugins handle redirects before our 307 check was done as part of NPAPI redirect handling (https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling), via https://bugzilla.mozilla.org/show_bug.cgi?id=475991#c14. I'm fine breaking this, as that seems like a large amount of trust to give plugins now, and we're down to one plugin anyways.
Comment hidden (mozreview-request)
Ok, so the patch I've posted here goes slightly above and beyond the original scope. This blocks all cross-origin 307s from plugins, period, versus just ones with non-simple content types. I just want to make sure that's ok before I do reviews and all. ni?'ing security + bz for possible web breakage feedback
Flags: needinfo?(ptheriault)
Flags: needinfo?(dveditz)
Flags: needinfo?(bzbarsky)
I really have no special web breakage insight here, sorry.  :(  We could play it safe and block cross-origin things unconditionally and the other 307s behind a pref that we can flip in release if we have to...
Flags: needinfo?(bzbarsky)
Blocking corss-origin redirects would solve the security issue, if thats what you are asking me. 

BTW the github repo in comment 0 claims chrome 52 (at least) is still vulnerable to this, even though according to the chrome bug[2] says its fixed. I haven't tested though. (or actually I think [1] was more about preventing custom headers in general, not supplying non-simple content-type headers)

Anyways re: breakage, chrome did some telemetry back in 2014: see comment 50 in that bug [2]. They recorded "So far, 367k calls to PPB_Flash.Navigate, and only ~200 were rejected because of non-simple request headers". I don't know if they are including arbitrary content-types in non-simple request headers there though. 


[1] https://bugs.chromium.org/p/chromium/issues/detail?id=332023
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=332023#c50
Flags: needinfo?(ptheriault)
I tried on Chrome 65, still saw a post request at the endpoint, so I think Chrome is still vulnerable?

If we go by NPAPI, this should technically be stopped by the plugin, because plugins that handle redirects are expects to handle security. We could talk to the Adobe about having them block this in the flash plugin itself too, but I'm fine blocking it on our end first.

As for what bz said about content already out there... I'll see about wrapping this in a pref that we can flip if something goes catastrophically wrong, 'cause I honestly have no idea how much this happens in the wild now.
Flags: needinfo?(dveditz)
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Exploit already constructed, see Comment 0

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? There's already a public github repo with the problem written up.

Which older supported branches are affected by this flaw? Everything since Jan 2011.

If not all supported branches, which bug introduced the flaw? See Comment 15 for history.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, but they should be pretty easy to fix up. This code hasn't been touched in many years.

How likely is this patch to cause regressions; how much testing does it need? Manual QA would be good.
Attachment #8972157 - Flags: sec-approval?
Attachment #8972157 - Flags: review?(ptheriault)
Attachment #8972157 - Flags: review?(jmathies)
Adding :jimm as the plugins reviewer for this, because we're kinda short on plugins reviewers.
Since this is a completely public bug, sec-approval+ is kind of redundant (since it is about limiting exposure of checkins). So, sec-approval+.
Attachment #8972157 - Flags: sec-approval? → sec-approval+
We should probably see if we can get this in for a ride-along with an inevitable 60.0.x point release.

Comment 27

a year ago
mozreview-review
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

https://reviewboard.mozilla.org/r/240844/#review247752

Not a peer, but looks good to me.
Attachment #8972157 - Flags: review?(ptheriault) → review+

Comment 28

a year ago
mozreview-review
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

https://reviewboard.mozilla.org/r/240844/#review248030

looks good to me, I wonder though what assumptions flash content authors have made. If we see a lot of follout we might have to reconsider.
Attachment #8972157 - Flags: review?(jmathies) → review+

Comment 29

a year ago
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d595a423fb5
Check redirect status code before forwarding to NPAPI r=jimm,pauljt

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d595a423fb5
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Tested this myself but would like to get extra verification now that it's landed, since figuring out the test may take a bit. STR instructions are in the github repo in Comment 0.
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Component: Flash (Adobe) → Plug-ins
Product: External Software Affecting Firefox → Core
Target Milestone: --- → mozilla62
Version: unspecified → Trunk
is that something we want to uplift?
Yeah, I talked to RyanVM about this last night. As soon as we get verification, I figured I'd put in the uplift request, though since we're just starting the beta cycle, should I just request uplift now and fixes if/when needed?
Flags: needinfo?(kyle) → needinfo?(sledru)
I'd be OK with uplifting this to Beta now while we're still early in the cycle and have time for feedback still. I'd prefer we wait until later in the cycle for the ESR branches, though.
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

Approval Request Comment
[Feature/Bug causing the regression]: Been around since at least 2010
[User impact if declined]: Possible 307 CSRF attacks in plugins
[Is this code covered by automated tests?]: Not yet, STR is somewhat complex. May do in followup
[Has the fix been verified in Nightly?]: Manually by developer
[Needs manual test from QE? If yes, steps to reproduce]: See Comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Possibly, but not likely
[Why is the change risky/not risky?]: Instead of shutting down complex content type cross origin 307s from plugins, we just shut down ALL cross origin 307s from plugins. Some users could be be doing cross origin 307s on simple types, but I'm not aware of any.
[String changes made/needed]: None
Attachment #8972157 - Flags: approval-mozilla-beta?
just like Ryan said!
Flags: needinfo?(sledru)
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

Per the discussion in this bug, let's get this uplifted for 61.0b5 so we can get some wider feedback and opportunity for regressions to be found.
Attachment #8972157 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've tried to reproduce the issue on Release 60 and to verify it on latest Nightly 62 and 61 Beta5. I'm not sure however what the expected result is, since the results I've seen are similar for the versions I've checked on (with/without the fix): maybe my steps or where I look for results aren't appropriate.
 
I've used the modified SimpleHTTPServer.py from comment 5, using for 'https://example_location_here' test.swf or some random hosted swf.
Calling 127.0.0.1:8000/test.php would redirect to http://127.0.0.1:8000/test.swf , will give me a 307 GET for test.php and a 200 GET for test.swf.

Using a call like: http://127.0.0.1:8000/test.swf?jsonData={"test":1}&php_url=127.0.0.1:8000/test.php&endpoint=http://techno.org/electronic-music-guide/ returns a 200 GET.

For both 60, 61 and 62 I get roughly the same results so, David, could you point me to what am I doing wrong in order to reproduce and verify this issue?
Flags: needinfo?(dblack)
Reporter

Comment 40

a year ago
Hi,
To clarify I have been accessing http://127.0.0.1:8000/read.html (taken from https://github.com/sp1d3r/swf_json_csrf/blob/master/read.html) and not test.swf directly. For example, 

http://localhost:12345/read.html?endpoint=https://example.com/&php_url=http://localhost:12345/12.php&reqmethod=post&jsonData={%22example%22:%20%221%22}&ct=application/json .

If it helps I can attach a har file to this bug.
Flags: needinfo?(dblack)
Reporter

Comment 41

a year ago
Seems I still do something wrong:

I have the repo from https://github.com/sp1d3r/swf_json_csrf and using the same call as the one you exemplified (http://localhost:8000/read.html?endpoint=https://example.com/&php_url=http://localhost:8000/test.php&reqmethod=post&jsonData={%22example%22:%20%221%22}&ct=application/json) I get what you see in the two attachments. In your HAR, I see clearly the 307 status, which I do not get for my test. If I call directly the http://localhost:8000/test.php, I get the 307 status.  
I am attaching the HARs from my tests on 62 and 60, maybe you can figure out what I'm missing and advise further.
Flags: needinfo?(dblack)
For the record, I never used the python STR while reproducing this, so I'm not sure how things will work against that. I used what came off the github repo, on my own domain setup, and watched the http logs for confirmation.
Can we do something to unblock verification here?  This is tracked for ESR branches but I would feel better if this was verified before we uplift.
(In reply to Julien Cristau [:jcristau] from comment #46)
> Can we do something to unblock verification here?  This is tracked for ESR
> branches but I would feel better if this was verified before we uplift.
Not sure how to proceed further either. I can't use the git STR, since I don't have a domain available, but even so, in the best case scenario, I'd literally do the same route as Kyle and I doubt I could smoke any potential issue here other than an obvious breakage.
I would guess the fastest route would be to ask Kyle to verify the fix in Beta 61?
Flags: needinfo?(kyle)
Flags: needinfo?(jcristau)
Verification method I used to develop bug, and just tried with Beta 61:

- Bring up git STR across 2 different domains, with form on one and receiving file on the other.
- Tail logs of second domain.
- Aim exploit page from first domain at second domain. (https://example1.com/exploit/test.swf?jsonData={%22test%22:1}&php_url=https://example1.com/exploit/test.php&endpoint=https://example2.com/endpoint
- Logs showed no access on request.

I guess we'll call this verified?
Flags: needinfo?(kyle)
(In reply to Kyle Machulis [:qdot] [:kmachulis] > 
> I guess we'll call this verified?

Thanks a lot. Apologies for the hassle, but can you confirm the bug verified on Fx62 as well?
Meanwhile, marking the bug as verified for Fx61 as per comment 48.
Flags: needinfo?(kyle)
Yes, it works on 62 also, using the same test.
Flags: needinfo?(kyle)
Thanks Kyle for the help. Based on comment 48 and comment 50, marking the bug as verified and removing qe+. 
-removing also the Ni for Julien.
-leaving the flag for David, since I still want to know what I missed using the python based testcase.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(jcristau)
Whiteboard: [adv-main61+]
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

This grafts cleanly to esr52 and esr60, would you mind requesting approval?
Flags: needinfo?(kyle)
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec bug
User impact if declined: 307 redirects in flash can cause XSS attacks 
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8972157 - Flags: approval-mozilla-esr60?
Attachment #8972157 - Flags: approval-mozilla-esr52?
Comment on attachment 8972157 [details]
Bug 1436241 - Check redirect status code before forwarding to NPAPI

sec-high fix for redirects in flash, approved for 52.9 and 60.1esr
Attachment #8972157 - Flags: approval-mozilla-esr60?
Attachment #8972157 - Flags: approval-mozilla-esr60+
Attachment #8972157 - Flags: approval-mozilla-esr52?
Attachment #8972157 - Flags: approval-mozilla-esr52+
Whiteboard: [adv-main61+] → [adv-main61+][adv-esr52.9+][adv-esr60.1+]
Alias: CVE-2018-12364
You need to log in before you can comment on or make changes to this bug.