1436241 - (CVE-2018-12364) Firefox allows flash to follow 307 redirects to other origins with arbitrary content-types

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[David Black]

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.

Comment 1

[David Black]

Seemingly related https://bugzilla.mozilla.org/show_bug.cgi?id=573873.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Might be a regression from bug 957403

Comment 3

[Hani Yacoub, Desktop QA]

Hi David, could you please provide clearer steps to reproduce this issue?

Comment 4

[Hani Yacoub, Desktop QA]

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.

Comment 5

[David Black]

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.

Comment 6

[David Black]

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

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ah, why was this closed? The steps in comment 0 are perfectly clear?

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

FYI this is a security related bug - its only not private, since this is a publicly documented issue with flash/firefox.

Comment 9

[Daniel Veditz [:dveditz]]

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.

Comment 10

[Daniel Veditz [:dveditz]]

Andrew: who is responsible for plugin code these days?

Comment 11

[Andrew Overholt [:overholt]]

Nominally qDot

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Try seems clear:

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=176339360

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1436241 - Check redirect status code before forwarding to NPAPI

NPAPI may handle a 307 redirect across different origins, while they
should only happen on same origin requests. Have the browser check
this before forwarding to NPAPI.

Review commit: https://reviewboard.mozilla.org/r/240844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240844/

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 20

[Boris Zbarsky [: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...

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Adding :jimm as the plugins reviewer for this, because we're kinda short on plugins reviewers.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Since this is a completely public bug, sec-approval+ is kind of redundant (since it is about limiting exposure of checkins). So, sec-approval+.

Comment 26

[Daniel Veditz [:dveditz]]

We should probably see if we can get this in for a ride-along with an inevitable 60.0.x point release.

Comment 27

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 28

[Jim Mathies [:jimm]]

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.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d595a423fb5
Check redirect status code before forwarding to NPAPI r=jimm,pauljt

Comment 30

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6d595a423fb5

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

is that something we want to uplift?

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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 35

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 36

[Sylvestre Ledru [:Sylvestre]]

just like Ryan said!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7002b1d37d0c

Comment 39

[Adrian Florinescu [:aflorinescu]]

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?

Comment 40

[David Black]

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.

Comment 41

[David Black]

Attachment: Archive 18-05-16 12-11-08.har

Comment 42

[Adrian Florinescu [:aflorinescu]]

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.

Comment 43

[Adrian Florinescu [:aflorinescu]]

Attachment: 60.har

Comment 44

[Adrian Florinescu [:aflorinescu]]

Attachment: 62.har

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 46

[Julien Cristau [:jcristau]]

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.

Comment 47

[Adrian Florinescu [:aflorinescu]]

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

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 49

[Adrian Florinescu [:aflorinescu]]

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

Comment 50

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yes, it works on 62 also, using the same test.

Comment 51

[Adrian Florinescu [:aflorinescu]]

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.

Comment 52

[Julien Cristau [:jcristau]]

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?

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 54

[Julien Cristau [:jcristau]]

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

Comment 55

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/67b2d8924841a4c44e2907628d35ee8a022ecd2d
https://hg.mozilla.org/releases/mozilla-esr60/rev/3101edb8003d6a3341aeef23d40d4550ab525f24