573873 - (CVE-2011-0059) CSRF from 307 redirects

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Lucas Adamski [:ladamski]]

Since plugins are not made aware of redirects they are vulnerable to CSRF via307 redirects.  Further details to be filled up by more knowledgeable folks.

Comment 1

[jimsonx]

I have outlined a new proposal below. This is similar to Ian's original proposal (see bug 475991 above). I believe this proposal fits the Flash Player's needs. Is this a feasible proposal for Mozilla folks?

---BEGIN PROPOSAL---

We propose adding a plugin variable that, if set, would prevent the browser from following any cross-domain HTTP redirects for the two calls NPN_GetURLNotify and NPN_PostURLNotify. The other non-notification calls NPN_GetURL and NPN_PostURL will not change. Non-HTTP redirects will not be affected by this change, such as file:/// URL redirects to another file via links/shortcuts.

We propose adding the plugin variable NPPVpluginBlockRedirectsXDomain = 3000 to the NPPVariable enum. This variable will be set by the plugin on a per instance basis by calling NPN_SetValue. The variable value is boolean. The variable value itself should be interpreted, not what it points to. This variable will be versioned by a new NPVERS value: NPVERS_HAS_BLOCK_XDOMAIN_REDIRECT = TBD.

If the variable value is set to TRUE, the browser will drop all cross-domain HTTP redirect requests for the plugin instance made through NPN_GetURLNotify or NPN_PostURLNotify. If a cross-domain redirect request is dropped, the browser will call NPP_URLNotify on this plugin instance with a new reason NPRES_RESOURCE_BLOCKED. This will allow the plugin to identify a blocked request scenario and will allow it to resend a modified request. If a cross domain redirect request is blocked, NPN_GetURLNotify and NPN_PostURLNotify will return NPERR_NO_ERROR, as the blocked request will be handled by NPP_URLNotify.

If the variable value is set to FALSE, then the browser will behave normally. That is, it will attempt to complete all redirect requests (including cross-domain redirects) for the plugin instance and report the appropriate reason back to NPP_URLNotify on the plugin instance.

---END PROPOSAL---

Comment 2

[Boris Zbarsky [:bzbarsky]]

That seems eminently doable, at least as far as necko is concerned...  no necko changes needed; just need to set up a redirect observer in the plug-ins code.

Comment 3

[jimsonx]

A clarification on the Firefox vulnerability:

Firefox is only vulnerable when the server response is a 307 redirect. This is because Firefox simply forwards the opaque data blob in this situation. Since Flash Player adds custom headers as part of this data blob, custom headers can be sent cross-domain. A violation of the security model.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Should the state of the variable be inspected at the time the request is made, or at the time the redirect happens? I.e. should we:

A) When NPN_GetURLNotify/NPN_PostURLNotify is called, inspect the current state of the variable, and if cross-origin redirects are disallowed, flag the request such that if a redirect happens later it is denied at that time.

or

B) When a redirect happens from a request created using NPN_GetURLNotify/NPN_PostURLNotify, check the variable and block the redirect if the variable indicates so.

I imagine B) is the way to go, otherwise the plugin has to keep the variable set for the duration of the request, potentially affecting requests made by other plugin instances.

If we go with A), the plugin can set the variable, make the request, and then immediately clear the variable. Knowing that no other requests will happen in the meantime.

Comment 5

[jimsonx]

Aren't NPP variables confined to a specific plugin instance? So other plugin instances should not be able to step on each other's toes.

It sounds like A is the best bet because as illustrated, the plugin instance can set the variable, make the request, and immediately clear the variable. Thus the request is complete at the moment of invocation, and the plugin simply has to wait for the response. This would allow a plugin instance to make several requests before a NPP_URLNotify has been invoked. These requests wouldn't overlap with each other, assuming the requests are made in a single threaded queue. 

Now imagine B in the same situation with several requests sent before a NPP_URLNotify. It would be unclear which request should have the variable set, and which shouldn't, as the variable will be applied to all of the requests. With B, we have to maintain the variable state through the entire request. Going with option A makes the most sense. The proposal will need to be modified to make this distinction clear.

Also, I just realized that the blocking bug for https://bugzilla.mozilla.org/show_bug.cgi?id=475991 has been fixed. This may make Ian's original proposal viable now, and it may be a more attractive solution over this proposal, as Ian's proposal is more scalable and flexible.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #5)
> Aren't NPP variables confined to a specific plugin instance? So other plugin
> instances should not be able to step on each other's toes.

Ah, could be, but you'd still have to worry about other requests from within the same instance, right?

As for bug 475991, I definitely think we should do that as well, but if we want something working on branches, such as Firefox 3.6, we'll have to use this simpler change for that.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, forgot to say that it seems like there is agreement that A is the way to go.

Comment 8

[Johnny Stenback (:jst)]

Josh, seems there's a plan here for older branches, and for trunk we now have an async redirect API that we can use. Could you look into this?

Comment 9

[Josh Aas]

Attachment: [1.9.2] disallow cross-origin redirects for plugin requests, v1.0

This patch is not well tested and I'm not sure that we want it, just saving it here for now. It is intended to disable all cross-origin redirects for plugin-initiated HTTP requests.

Comment 10

[Josh Aas]

The following NPAPI extension specification has been approved. We'll be implementing in Firefox 4.

https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling

Comment 11

[Josh Aas]

Attachment: [trunk] disallow cross-origin redirects for plugin requests, v1.0

Comment 12

[Benjamin Smedberg]

So the current solution is "disallow cross-origin by default, but allow the plugin to override the default behavior"?

Do we know how much of the existing web this will break? Is this only for data requested via NPN_Get* APIs, or does this also affect <embed src=""> and <object data=""> ?

Comment 13

[Josh Aas]

(In reply to comment #12)
> So the current solution is "disallow cross-origin by default, but allow the
> plugin to override the default behavior"?

The solution for the 1.9.2 branch is simply to disallow potentially dangerous redirects, I am not planning to backport the NPAPI HTTP redirect API to 1.9.2.

> Do we know how much of the existing web this will break? Is this only for data
> requested via NPN_Get* APIs, or does this also affect <embed src=""> and
> <object data=""> ?

We don't know how much content this would break but I have been unable to find anything that breaks in popular content like Hulu and YouTube. The fix as I have written it so far would affect the src stream as well as plugin-initiated streams.

I am working on an alternative patch that would only disallow 307 redirects and allow others. This would further limit the potential impact on existing content but I need to verify that it still addresses all of our concerns.

Comment 14

[Benjamin Smedberg]

What is different about 307 redirects compared with 302/303 which make them more dangerous in terms of CSRF? This feels to me like it's going to break a lot of minority Flash stuff.

Comment 15

[Boris Zbarsky [:bzbarsky]]

302/303 redirects are converted into GET requests and drop their upload stream (which in the case of plug-ins also means they drop the extra headers the plug-in stashed at the beginning of the stream, in addition to any POST data in it).

307 redirects redirect the reponse with its original method and original upload data.

Comment 16

[Benjamin Smedberg]

ok, so blocking 307 makes perfect sense given the RFC2616 spec: "If the 307 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued."

I would support that on the branch much more than a generic patch which blocks all redirects, unless we know of ways these redirects are being used harmfully.

Comment 17

[Benjamin Smedberg]

Oh, and, are we blocking all requests, or just non-GET/HEAD requests?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

307 redirects are probably very rarely used on the web since in many UAs, including firefox, it opens very ugly modal dialogs that the user has to interact with.

This modal dialog is of course ok if you are an evil attacker, but it's something that most legitimate websites stay away from.

(We're planning on removing this dialog in future releases, and so are other browsers, this bug is one of the things blocking that)

Comment 19

[Josh Aas]

Attachment: [1.9.2] disallow cross-origin 307 redirects for plugins, v1.0

I'm not sure this is the most efficient way to code this but it should block all cross-origin 307 redirects.

Comment 20

[Josh Aas]

(In reply to comment #13)
> The fix as I
> have written it so far would affect the src stream as well as plugin-initiated
> streams.

This is not true - the existing fixes do not affect the src stream but I don't think it matters. More on that coming soon.

Comment 21

[Josh Aas]

Attachment: [trunk] fix v1.0

I don't think we need to change any src stream handling because the plugin cannot insert headers into that request. The plugin can only insert headers into NPAPI-initiated POST requests, so we only need to block cross-origin 307 redirects for POST method requests. Thanks to bz for helping me with a more detailed understanding of certain aspects of HTTP transactions.

Comment 22

[Josh Aas]

Attachment: [1.9.2] fix v1.0

Comment 23

[Josh Aas]

Given the number of vendors and researchers that know about this we might want to block our next round of security releases on this. Requesting blocking 1.9.2 and 1.9.1.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 490953 [details] [diff] [review]
[trunk] fix v1.0

This looks good, couple of minor comments below:

- In content/base/public/nsIContentUtils.h:

 #define NS_ICONTENTUTILS_IID \
-{ 0x3682dd99, 0x8560, 0x44f4, \
-  { 0x9b, 0x8f, 0xcc, 0xce, 0x9d, 0x7b, 0x96, 0xfb } }
+{ 0x6f737455, 0xf226, 0x4b34, \
+{ 0xae, 0x2c, 0x84, 0x09, 0x53, 0x9f, 0x96, 0x6d } }

We're not supposed to change interfaces on trunk any more for 2.0, you should add a new interface here, even though that seems silly in this case...

- In nsPluginStreamListenerPeer::GetInterfaceGlobal():

+    nsCOMPtr<nsIDocument> doc;
+    nsresult rv = owner->GetDocument(getter_AddRefs(doc));
+    if (NS_SUCCEEDED(rv) && doc) {
+      nsIScriptGlobalObject* global = doc->GetScriptGlobalObject();
+      if (global) {

I know you just moved this code, but please make this use nsPIDOMWindow *window = doc->GetWindow() instead.

- In nsPluginStreamListenerPeer::AsyncOnChannelRedirect():

+  // Don't allow cross-origin 307 POST redirects. Fall back to event channel sink for
+  // script global object by default.

"window", not "script global object"

+      if (method.EqualsLiteral("POST")) {
+        nsCOMPtr<nsIContentUtils> contentUtils = do_GetService("@mozilla.org/content/contentutils;1");
+        NS_ENSURE_TRUE(contentUtils, NS_ERROR_OUT_OF_MEMORY);
+        
+        nsCOMPtr<nsIInterfaceRequestor> sameOriginChecker = contentUtils->GetSameOriginChecker();
+        NS_ENSURE_TRUE(sameOriginChecker, NS_ERROR_OUT_OF_MEMORY);

GetSameOriginChecker() is infallible, no need to null check here.

r=jst

Comment 25

[Josh Aas]

Attachment: [trunk] fix v1.1

Comment 26

[Josh Aas]

Attachment: [trunk] fix v1.2

Right after I posted the updated patch someone check a conflicting patch into mozilla-central. This is the same patch but updated for current trunk.

Comment 27

[Josh Aas]

Attachment: [1.9.2] fix v1.2

Comment 28

[LegNeato]

Too late for 3.6.13/3.5.16, we'll try to take this for the next ones.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to comment #13)
> The solution for the 1.9.2 branch is simply to disallow potentially dangerous
> redirects, I am not planning to backport the NPAPI HTTP redirect API to 1.9.2.

Why not? If you did we could at least tell people "If your plugin content breaks ask your users to upgrade to a plugin version that supports the new API". With the proposed patch the content just breaks, end of story, and we don't have any idea how much content we're actually breaking.

Comment 30

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8f35051eed78

Comment 31

[Josh Aas]

(In reply to comment #29)
> Why not? If you did we could at least tell people "If your plugin content
> breaks ask your users to upgrade to a plugin version that supports the new
> API". With the proposed patch the content just breaks, end of story, and we
> don't have any idea how much content we're actually breaking.

The backport would be very invasive, especially since we don't have an async API for the HTTP redirect callback (nsIChannelEventSink) on pre-2.0 branches.

The latest fix targets a pretty specific redirect situation: 307 status, POST request method, cross-origin, plugin and not src initiated. We can't know exactly how much content this will break but I think we have good reason to believe it won't be a major problem - the targeted situation should be quite rare outside of attack scenarios. Even if we did backport the NPAPI HTTP redirect API it would take some time for vendors to implement support for it and distribute their updated plugins. The fallback for plugins that didn't implement the API would be to simply disallow like the current patch does.

Comment 32

[Josh Aas]

Attachment: [1.9.2] fix v1.3

I accidentally updated the trunk patch again and labeled it 1.9.2. This is the real 1.9.2 patch, sorry.

Comment 33

[LegNeato]

Comment on attachment 491626 [details] [diff] [review]
[1.9.2] fix v1.3

a=LegNeato for 1.9.2.13. Josh says he is working on the 1.9.1 version.

Comment 34

[Josh Aas]

pushed to mozilla-1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/37a0416ff3ff

Comment 35

[Josh Aas]

Attachment: [1.9.1] fix v1.3

Comment 36

[Boris Zbarsky [:bzbarsky]]

For the case of an actual 307 POST, don't you still need to call into the original callbacks, in addition to making the same-origin call?

Comment 37

[Josh Aas]

The same origin checker makes the callback itself if it ever returns a non-failure nsresult.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Yes, but the problem is that you're failing to notify the original observer of the redirect.  Which means that whatever code it wants to run on redirect (e.g. updating member variables to point to the new channel) doesn't get to run, no?

Comment 39

[Josh Aas]

(In reply to comment #38)
> Yes, but the problem is that you're failing to notify the original observer of
> the redirect.  Which means that whatever code it wants to run on redirect (e.g.
> updating member variables to point to the new channel) doesn't get to run, no?

We don't (didn't, anyway) want to run any other code when a redirect happens. The added code here simply kills off a plugin-initiated network stream if it meets certain criteria. Previously we didn't do anything if there was a redirect, we didn't even know a redirect had happened until data started being delivered.

It is true that when a plugin calls NPN_GetURLNotify (or anything else that notifies) for a URL that results in a redirect we send the original URL in our notification on stream termination (NPP_URLNotify) but we did that before this patch too. The final URL which the data comes from is reflected during stream delivery in NPP_Write. Whether or not we want to send the original or the final URL in NPP_URLNotify is a question separate from this patch and I don't think it matters much. Plugins don't have a good reason to look at the URL argument to NPP_URLNotify (the opaque 'notifyData' identifier is what really matters) and they've been dealing with this behavior without reported issues for a while.

I think this addresses your concern, let me know if I am misunderstanding what you're getting at.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> The added code here simply kills off a plugin-initiated network stream if it
> meets certain criteria

Yes, and if it meets some but not all of the criteria it allows it to continue without notifying the redirect observers that the pre-patch code notified.

> Previously we didn't do anything if there was a redirect

That's false.  Previously, this code dispatched an AsyncOnChannelRedirect on the docshell, which sent progress listener notifications about the redirect.  Your new code no longer does that.  Why is that change desired or ok?

> I think this addresses your concern

No, I don't think it does.

Comment 41

[LegNeato]

Should we just back this out on 1.9.2, seeing how it isn't even fixed on 1.9.1? What's the consequences of leaving this fix in w/o the notifications?

Comment 42

[Josh Aas]

(In reply to comment #41)
> Should we just back this out on 1.9.2, seeing how it isn't even fixed on 1.9.1?
> What's the consequences of leaving this fix in w/o the notifications?

Yes, we should back it out until we're confident concerning the issue bz raised and to maintain consistency with 1.9.1. Done:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4f04b8a9e40
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7f500ea32c40

Comment 43

[Josh Aas]

I backed that out from default, does it need to come out of a release branch?

Comment 44

[Josh Aas]

I talked to bz, I see the problem now. New patches coming soon.

Comment 45

[Josh Aas]

Attachment: [trunk] fix v1.4

This patch addresses the problem bz brought up. Since the original patches are on trunk still it is only the fix for the issue bz brought up. I'll post new patches for 1.9.2 and 1.9.1 incorporating this fix.

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 494721 [details] [diff] [review]
[trunk] fix v1.4

r=jst, but bz should have a look at this as well.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 494721 [details] [diff] [review]
[trunk] fix v1.4

I'd rather stick with "old" instead of "trusted"; I'm not sure we want to imply that we trust anything here.  We don't.

sr=me with that fixed throughout.

Comment 48

[LegNeato]

What's the user impact if we ship 1.9.2.13 with the previous "fix" still in? Unknown? Kittens are slaughtered?

Comment 49

[Boris Zbarsky [:bzbarsky]]

> What's the user impact if we ship 1.9.2.13 with the previous "fix" still in?

Based on the minimal code auditing I did, the main impact is that some extensions will no longer get redirect notifications for loads when they used to, when all these criteria are satisfied:

1)  It's a plugin-initiated url load
2)  The load is a POST
3)  The server responds with 307
4)  The 307 target is same-origin with the original URI requested

It's really not a stop-ship kinda thing, imo.  ;)

Comment 50

[LegNeato]

That's what I wanted to know, thanks!

Comment 51

[LegNeato]

We're going to skip this for 1.9.2.13. I would like this backed out of mozilla-1.9.2 GECKO19213_20101122_RELBRANCH, as we will be rebuilding for a different issue.

We'll ship this updated fix in 1.9.2.14 (and 1.9.1.17 too please).

Comment 52

[Josh Aas]

Attachment: [trunk] fix v1.5

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/147071ae59cf

Comment 53

[Josh Aas]

Attachment: [1.9.2] fix v1.5

Comment 54

[Daniel Veditz [:dveditz]]

Gavin backed out of the relbranch per comment 42 and comment 51
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0c159bd1d600

Comment 55

[Josh Aas]

Attachment: [1.9.1] fix v1.5

Comment 56

[Daniel Veditz [:dveditz]]

Comment on attachment 495018 [details] [diff] [review]
[1.9.2] fix v1.5

Approved for 1.9.2.14, a=dveditz for release-drivers

Comment 57

[Daniel Veditz [:dveditz]]

Comment on attachment 495056 [details] [diff] [review]
[1.9.1] fix v1.5

Approved for 1.9.1.17, a=dveditz for release-drivers

Comment 58

[Josh Aas]

pushed to mozilla-1.9.2 and mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6bc072d1c545
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d8b179375f5b

Comment 59

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 version

Comment 60

[Daniel Veditz [:dveditz]]

We are about to release fixes for this problem with Firefox 3.6.14 and 3.5.17. Can we announce this fix, or should we keep it private while other browsers (presumably also affected?) work on it? If so is there a timeline of when we can publish an advisory for this?

Comment 61

[Peleus Uhley]

For now, please hold off on announcing the fix because we are still working with the other browser vendors. I will check to see how they are progressing on their end.

Comment 62

[Daniel Veditz [:dveditz]]

dchan verified this bug is fixed in 3.6.14, and that the CSRF happens in 3.6.13 if you don't cancel the redirect prompt.

Comment 63

[Daniel Veditz [:dveditz]]

(In reply to comment #61)
> For now, please hold off on announcing the fix because we are still working
> with the other browser vendors. I will check to see how they are progressing on
> their end.

Peleus: is that still true in light of http://www.djangoproject.com/weblog/2011/feb/08/security/ ? Kind of lets the cat out of the bag there. Currently we have not prepared an advisory for this for Monday's release (Feb 14) of 3.6.14 and 3.5.17 so we are effectively embargoing it. But we may have to say something anyway if this is public -- people will start asking (internally our websec guys have already asked the obvious question, not knowing about this client bug fix).

Comment 64

[Daniel Veditz [:dveditz]]

Also posted with the subject
  [WEB SECURITY] CSRF: Flash + 307 redirect = Game Over
by Phillip Purviance of whitehatsec.com at
http://lists.webappsec.org/pipermail/websecurity_lists.webappsec.org/2011-February/007533.html

Comment 65

[jimsonx]

Is there a bug to implement the NPAPI for HTTP Redirect Handling? https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling

Is this already implemented for Firefox 4 Nightly? If not, is there an ETA?

Comment 66

[jimsonx]

Sorry, didn't notice attached bug. Please ignore.