Closed Bug 573873 (CVE-2011-0059) Opened 14 years ago Closed 14 years ago

CSRF from 307 redirects

Categories

(Core Graveyard :: Plug-ins, defect)

x86
All
defect
Not set
normal

Tracking

(blocking2.0 betaN+, blocking1.9.2 needed, status1.9.2 .14-fixed, blocking1.9.1 needed, status1.9.1 .17-fixed)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
blocking1.9.2 --- needed
status1.9.2 --- .14-fixed
blocking1.9.1 --- needed
status1.9.1 --- .17-fixed

People

(Reporter: ladamski, Assigned: jaas)

References

Details

(Keywords: verified1.9.2, Whiteboard: [sg:high] Okay to publish? embargo?)

Attachments

(6 files, 9 obsolete files)

15.51 KB, patch
jst
: review+
Details | Diff | Splinter Review
6.50 KB, patch
jst
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
6.46 KB, patch
Details | Diff | Splinter Review
16.19 KB, patch
jst
: review+
Details | Diff | Splinter Review
15.79 KB, patch
jst
: review+
Details | Diff | Splinter Review
9.64 KB, patch
Details | Diff | Splinter Review
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.
See Also: → 475991
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---
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.
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.
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.
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.
(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.
Sorry, forgot to say that it seems like there is agreement that A is the way to go.
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?
Assignee: nobody → joshmoz
blocking2.0: --- → betaN+
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.
The following NPAPI extension specification has been approved. We'll be implementing in Firefox 4. https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling
Attachment #488314 - Attachment is patch: true
Attachment #488314 - Attachment mime type: application/octet-stream → text/plain
Attachment #488314 - Flags: review?(jst)
Attachment #490562 - Flags: review?(jst)
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=""> ?
(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.
Attachment #488314 - Flags: review?(jst)
Attachment #490562 - Flags: review?(jst)
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.
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.
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.
Oh, and, are we blocking all requests, or just non-GET/HEAD requests?
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)
I'm not sure this is the most efficient way to code this but it should block all cross-origin 307 redirects.
Attachment #488314 - Attachment is obsolete: true
Attachment #490562 - Attachment is obsolete: true
Attachment #490680 - Flags: review?(jst)
Attachment #490680 - Flags: review?(jst)
(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.
Attached patch [trunk] fix v1.0 (obsolete) — Splinter Review
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.
Attachment #490680 - Attachment is obsolete: true
Attachment #490953 - Flags: review?(jst)
Attached patch [1.9.2] fix v1.0 (obsolete) — Splinter Review
Attachment #490954 - Flags: review?(jst)
Attachment #490954 - Attachment is patch: true
Attachment #490954 - Attachment mime type: application/octet-stream → text/plain
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
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
Attachment #490953 - Flags: review?(jst) → review+
Attachment #490954 - Flags: review?(jst)
Attached patch [trunk] fix v1.1 (obsolete) — Splinter Review
Attachment #490953 - Attachment is obsolete: true
Attachment #491094 - Flags: review?(jst)
Attachment #490954 - Attachment is obsolete: true
Attached patch [trunk] fix v1.2Splinter Review
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.
Attachment #491094 - Attachment is obsolete: true
Attachment #491103 - Flags: review?(jst)
Attachment #491094 - Flags: review?(jst)
Attachment #491103 - Flags: review?(jst) → review+
Attached patch [1.9.2] fix v1.2 (obsolete) — Splinter Review
Attachment #491184 - Flags: review?(jst)
Too late for 3.6.13/3.5.16, we'll try to take this for the next ones.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
(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.
(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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #491184 - Flags: review?(jst) → review+
Attached patch [1.9.2] fix v1.3 (obsolete) — Splinter Review
I accidentally updated the trunk patch again and labeled it 1.9.2. This is the real 1.9.2 patch, sorry.
Attachment #491184 - Attachment is obsolete: true
Attachment #491626 - Flags: review?(jst)
Attachment #491626 - Flags: review?(jst) → review+
Attachment #491626 - Flags: approval1.9.2.13?
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.
Attachment #491626 - Flags: approval1.9.2.13? → approval1.9.2.13+
Attached patch [1.9.1] fix v1.3 (obsolete) — Splinter Review
Attachment #491712 - Flags: review?(jst)
Depends on: 613376
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?
The same origin checker makes the callback itself if it ever returns a non-failure nsresult.
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?
(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.
> 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.
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?
(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
I backed that out from default, does it need to come out of a release branch?
I talked to bz, I see the problem now. New patches coming soon.
Attachment #491712 - Flags: review?(jst)
Attached patch [trunk] fix v1.4Splinter Review
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.
Attachment #491103 - Attachment is obsolete: true
Attachment #491626 - Attachment is obsolete: true
Attachment #491712 - Attachment is obsolete: true
Attachment #494721 - Flags: review?(jst)
Comment on attachment 494721 [details] [diff] [review] [trunk] fix v1.4 r=jst, but bz should have a look at this as well.
Attachment #494721 - Flags: superreview?(bzbarsky)
Attachment #494721 - Flags: review?(jst)
Attachment #494721 - Flags: review+
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.
Attachment #494721 - Flags: superreview?(bzbarsky) → superreview+
What's the user impact if we ship 1.9.2.13 with the previous "fix" still in? Unknown? Kittens are slaughtered?
> 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. ;)
That's what I wanted to know, thanks!
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).
Attachment #491103 - Attachment is obsolete: false
Attached patch [trunk] fix v1.5Splinter Review
Attached patch [1.9.2] fix v1.5Splinter Review
Attachment #495018 - Flags: review?(jst)
Attached patch [1.9.1] fix v1.5Splinter Review
Attachment #495056 - Flags: review?(jst)
Attachment #495018 - Flags: review?(jst) → review+
Attachment #495056 - Flags: review?(jst) → review+
Attachment #495018 - Flags: approval1.9.2.14?
Attachment #495056 - Flags: approval1.9.1.17?
Comment on attachment 495018 [details] [diff] [review] [1.9.2] fix v1.5 Approved for 1.9.2.14, a=dveditz for release-drivers
Attachment #495018 - Flags: approval1.9.2.14? → approval1.9.2.14+
Comment on attachment 495056 [details] [diff] [review] [1.9.1] fix v1.5 Approved for 1.9.1.17, a=dveditz for release-drivers
Attachment #495056 - Flags: approval1.9.1.17? → approval1.9.1.17+
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?
Alias: CVE-2011-0059
Whiteboard: [sg:high] → [sg:high] Okay to publish? embargo?
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.
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.
Keywords: verified1.9.2
(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).
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
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?
Sorry, didn't notice attached bug. Please ignore.
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: