Closed Bug 1317641 (CVE-2016-9078) Opened 8 years ago Closed 8 years ago

http redirect to data: inherits principal (SVG image cookie setting; object XSS)

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox50 + verified
firefox51 + verified
firefox52 + verified
firefox53 + fixed

People

(Reporter: alex, Assigned: dragana)

References

(
URL
)

Details

(4 keywords, Whiteboard: [adv-main50+] see comments 16 and 20 [necko-active])

Attachments

(2 files, 2 obsolete files)

bug_1317641.patch
1.72 KB, patch
dragana
: review+
dveditz
: approval-mozilla-aurora+
dveditz
: approval-mozilla-beta+
dveditz
: approval-mozilla-release+
dveditz
: sec-approval+
Details | Diff | Splinter Review
testcase for <object> XSS
551 bytes, text/html
Details
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce: Tested on: Windows 7 x 64 Bit - Firefox 49.0.2 Tested on: Windows 8.1 x 32 Bit - Firefox Nightly: 53.0a1 A domain eg(example.com) loads an image file from an attacker controlled webserver: <img src="//attacker.com/redirect_svg.php"> The attacker server response with a redirect with an data: uri, which specifies an svg image file: //redirect_svg.php HTTP 302 Found Location: data:image/svg+xml,%3Csvg%20xmlns%3D%27http%3A//www.w3.org/2000/svg%27%3E%3Ccircle%20r%3D%27100%27%3E %3C/circle%3E%3CforeignObject%3E%3Chtml%20xmlns%3D%27http%3A//www.w3.org/1999/xhtml%27%3E%3Cmeta%20http-equiv %3D%27Set-Cookie%27%20content%3D%27ppp%3Dqqq%27%20/%3E%3C/html%3E%3C/foreignObject%3E%3C/svg%3E Decoded Payload: data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'><circle r='100'></circle><foreignObject><html xmlns='http://www.w3.org/1999/xhtml'><meta http-equiv='Set-Cookie' content='ppp=qqq' /></html></foreignObject></svg> The SVG file is interpreted and displayed on example.com and a cookie ppp=qqq is set for example.com //Poc: # set DNS Host entry for attacker.com: 84.112.240.90 View: http://84.112.240.90/view_svg.html Actual results: Firefox detects and interprets the Set-Cookie value specified in the meta tag. This cookie is then stored for the domain example.com. This makes it possible to set arbitrary cookies for any domain, which supports external images from users, which are viewed via the img tag. As developers do not expect images to be able to set cookies for their own domain, this behavior can introduce security vulnerabilities. Expected results: Firefox should not interpret Set-Cookie values in SVG meta tags, as this is the behavior in other browsers.
CCing Jonathan to figure out how to solve this.
Group: firefox-core-security → layout-core-security
Component: Untriaged → SVG
Flags: needinfo?(jwatt)
Keywords: sec-high
Product: Firefox → Core
(This reminds me a bit of bug 628747; it's another case where SVG images are getting powers beyond what normal images can do, by virtue of having a document under the hood.) We probably need to add an IsBeingUsedAsImage() safeguard in whatever cookie-setting codepath gets invoked here. (Or maybe we might want to check "IsResourceDoc()", which is broader & to covers other sorts of external-resource-documents as well, for e.g. <use> and SVG filters. Those are less worrisome & probably can't be used to create a PoC like the one here, because they're guarded by same-origin checks; but it might be nice to be consistent between flavors of resource document, if it's web-compatible.) I'll leave the needinfo for jwatt open, but I'm up for taking this as well.
See nsContentSink::ProcessMETATag and nsContentSink::ProcessHeaderData. But this is using the principal of the document, which in this case should be a nullprincipal, since this is a data: result of an HTTP redirect. Why are we ending up with a cookie for example.com?
Note that per spec https://html.spec.whatwg.org/multipage/semantics.html#attr-meta-http-equiv-set-cookie says to set the cookie for the document URL, so in general this should in fact set a cookie if someone loads an SVG image from foo.com, and the cookie should be set for foo.com.
(In reply to Daniel Holbert [:dholbert] from comment #2) > (This reminds me a bit of bug 628747; it's another case where SVG images are > getting powers beyond what normal images can do, by virtue of having a > document under the hood.) > > We probably need to add an IsBeingUsedAsImage() safeguard in whatever > cookie-setting codepath gets invoked here. (Or maybe we might want to check > "IsResourceDoc()", which is broader & to covers other sorts of > external-resource-documents as well, for e.g. <use> and SVG filters. Those > are less worrisome & probably can't be used to create a PoC like the one > here, because they're guarded by same-origin checks; but it might be nice to > be consistent between flavors of resource document, if it's web-compatible.) > > I'll leave the needinfo for jwatt open, but I'm up for taking this as well. I know it is off-topic but I had a look at your posted bug report and I have a variation of the same behavior, which still works. I will report it soon. So maybe your idea could help to prevent such kind of vulnerability.
(In reply to insertscript from comment #5) > I know it is off-topic but I had a look at your posted bug report and I have > a variation of the same behavior, which still works. I will report it soon. > So maybe your idea could help to prevent such kind of vulnerability. (Interesting! Please do report that, and CC me - thanks!)
(In reply to insertscript from comment #0) > //Poc: > # set DNS Host entry for attacker.com: 84.112.240.90 > View: http://84.112.240.90/view_svg.html I'm getting "Connection refused" / "Unable to connect" when trying to download or view the PoC page here. Is the PoC still available there?
Flags: needinfo?(alex)
Never mind, I've set up my own version of the PoC here: https://dholbert.org/sekrit/test.html That loads an SVG file from 3rd-party-site people-mozilla.org (which is playing the role of "attacker" in this scenario), and I have an .htaccess file on people-mozilla.org which redirects to the data URI quoted in comment 0. With that setup: STR: 1. load https://dholbert.org/sekrit/test.html in a fresh profile. 2. Right-click the page & choose "View page info" 3. Click "Security" and then "View Cookies" ACTUAL RESULTS: The cookies UI shows that I have a cookie for dholbert.org, with name "ppp" and value "qqq".
Flags: needinfo?(alex)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3) > But this is using the principal of the document, which in this case should > be a nullprincipal, since this is a data: result of an HTTP redirect. Why > are we ending up with a cookie for example.com? Uh oh. In nsContentSink::ProcessHeaderData, we have: - mDocument is a SVGDocument* - mDocument->NodePrincipal() is an instance of nsPrincipal, whose mCodebase member is a nsStandardURL for "https://dholbert.org/sekrit/test.html". So we don't actually have a null principal here (for the SVG document) after all... We're inheriting the hosting document's principal somehow.
Status: UNCONFIRMED → NEW
Ever confirmed: true
mDocument->NodePrincipal() switches from a null principal to the "real" principal here: https://dxr.mozilla.org/mozilla-central/rev/28e2a6dde76ab6ad4464a3662df1bd57af04398a/dom/base/nsDocument.cpp#1982-1983 ...with this backtrace: #0 0x00007fd28cdc8274 in nsDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7fd256550000, aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010) at dom/base/nsDocument.cpp:1983 #1 0x00007fd28ed22380 in mozilla::dom::XMLDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7fd256550000, aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010) at dom/xml/XMLDocument.cpp:255 #2 0x00007fd28cdc9c83 in nsDocument::StartDocumentLoad(char const*, nsIChannel*, nsILoadGroup*, nsISupports*, nsIStreamListener**, bool, nsIContentSink*) (this=0x7fd256550000, aCommand=0x7fd29220d3db "external-resource", aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010, aContainer=0x0, aDocListener=0x7ffd22c05388, aReset=true, aSink=0x0) at dom/base/nsDocument.cpp:2379 #3 0x00007fd28ed23438 in mozilla::dom::XMLDocument::StartDocumentLoad(char const*, nsIChannel*, nsILoadGroup*, nsISupports*, nsIStreamListener**, bool, nsIContentSink*) (this=0x7fd256550000, aCommand=0x7fd29220d3db "external-resource", aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010, aContainer=0x0, aDocListener=0x7ffd22c05388, aReset=true, aSink=0x0) at dom/xml/XMLDocument.cpp:513 #4 0x00007fd28f84c07f in nsContentDLF::CreateDocument(char const*, nsIChannel*, nsILoadGroup*, nsIDocShell*, nsID const&, nsIStreamListener**, nsIContentViewer**) (this=0x7fd26e1d1840, aCommand=0x7fd29220d3db "external-resource", aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010, aContainer=0x0, aDocumentCID=..., aDocListener=0x7ffd22c05388, aContentViewer=0x7ffd22c05390) at layout/build/nsContentDLF.cpp:375 #5 0x00007fd28f84ba63 in nsContentDLF::CreateInstance(char const*, nsIChannel*, nsILoadGroup*, nsACString_internal const&, nsIDocShell*, nsISupports*, nsIStreamListener**, nsIContentViewer**) (this=0x7fd26e1d1840, aCommand=0x7fd29220d3db "external-resource", aChannel=0x7fd24aff4000, aLoadGroup=0x7fd258934010, aContentType=..., aContainer=0x0, aExtraInfo=0x0, aDocListener=0x7ffd22c05388, aDocViewer=0x7ffd22c05390) at layout/build/nsContentDLF.cpp:200 #6 0x00007fd28ca9b8f0 in mozilla::image::SVGDocumentWrapper::SetupViewer(nsIRequest*, nsIContentViewer**, nsILoadGroup**) (this=0x7fd260d80ec0, aRequest=0x7fd24aff4000, aViewer=0x7fd260d80ef0, aLoadGroup=0x7fd260d80ef8) at image/SVGDocumentWrapper.cpp:344 At this point, aChannel is an instance of nsDataChannel. Inside of nsScriptSecurityManager::GetChannelResultPrincipal, we check some the channel's loadInfo->GetSecurityMode(), and that returns 4, which is nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS (1 << 2), documented here: https://dxr.mozilla.org/mozilla-central/rev/28e2a6dde76ab6ad4464a3662df1bd57af04398a/netwerk/base/nsILoadInfo.idl#77-81 Quoting the documentation for that security flag: "Loads from data: will inherit the principal of the origin that triggered the load." That's indeed the behavior we're exhibiting here. So we probably want to *not* be using that flag... The next one (SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL) seems perhaps more appropriate & the behavior that bz was anticipating in comment 3 ("Loads from data: will be allowed, but the resulting resource will get a null principal")...
Er, let me quote the full documentation for the security flag that we're using here: > * Allow loads from other origins. Loads from data: will inherit > * the principal of the origin that triggered the load. > * Commonly used by plain <img>, <video>, <link rel=stylesheet> etc. > */ > const unsigned long SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS = (1<<2); Note the "commonly used by plain <img>" there. So this flag is what we do indeed intend to be using for <img>. bz, do you know if we should consider *not* using this flag for images? I don't know offhand what the fallout would be from using null principals (as opposed to inherited principals) for image loads... (Side note -- we could easily close off this attack vector by adding an mDocument->IsBeingUsedAsImage() check to the string of checks in nsContentSink::ProcessHeaderData. But perhaps we should really be rethinking the nsILoadInfo security-flags that we use for image loads, so that the correct behavior falls out naturally? If there wouldn't be unwanted side effects, of such a change, at least. bz, what do you think?)
Flags: needinfo?(jwatt) → needinfo?(bzbarsky)
(In reply to Daniel Holbert [:dholbert] from comment #7) > (In reply to insertscript from comment #0) > > //Poc: > > # set DNS Host entry for attacker.com: 84.112.240.90 > > View: http://84.112.240.90/view_svg.html > > I'm getting "Connection refused" / "Unable to connect" when trying to > download or view the PoC page here. Is the PoC still available there? sry that IP points to my laptop and I turned it off, sorry (I assumed correctly that it is quite easy to creaty a PoC in the case mine is not reachable)
I tried printing out the image document's principal during painting in a few simple cases. I printed out the following value in a GDB breakpoint within mozilla::VectorImage::Draw() -- the URI associated with the image document's principal: mSVGDocumentWrapper.get()->mViewer.get()->GetDocument()->mNodeInfo->NodeInfoManager()->DocumentPrincipal()->mCodebase.get()->mSpec Scenarios tested: (1) HTML with <img src="data:image/svg+xml,...">: image doc's principal has URI of the hosting page. (2) HTML with <img src="some-remote-svg-file.svg">: image doc's principal has the URI of the the remote SVG resource (after following 302 redirects) ...and then of course with (1) and (2) combined, you see the ACTUAL RESULTS described in comment 0 and comment 8. (The image doc's principal follows 302 redirects [per (2)] and ends up at a data URI, which [per (1)] makes us use the URI of the hosting page for the principal, given our security flags.) So: SVG-as-an-image documents always ends up with a "real" principal (not a null principal) of some sort. I'm not sure if that "real principal" empowers extra badness beyond the ability to set cookies... bz, I'm curious if this worries you or not. (I did verify that the image document *does* have a null pointer for its mScriptGlobalObject, which is confirmation that we've disabled JS at least. Maybe that closes off all other avenues of badness?)
SEC_ALLOW_ORIGIN_DATA_INHERITS is the right flag to use for an <img src="data:stuff">. It's completely broken if we're using it for the result of an HTTP redirect, though! HTTP redirects very carefully did NOT inherit loading principal when redirecting to data:, but it sounds like someone broke that during the various loadinfo changes. Christoph is on PTO, I guess, so pinging Tanvi...
Flags: needinfo?(bzbarsky) → needinfo?(tanvi)
> I'm not sure if that "real principal" empowers extra badness beyond the ability to set cookies... Unclear. That said, if we could give the SVG document a nullprincipal while leaving the _image_ with the relevant principal (so canvas drawImage works correctly) that would be fine. But even then, the data: behavior on HTTP redirect sounds busted.
So yeah, in HttpBaseChannel::SetupReplacementChannel we do: nsCOMPtr<nsILoadInfo> newLoadInfo = static_cast<mozilla::LoadInfo*>(mLoadInfo.get())->Clone(); and then we change nothing about newLoadInfo except maybe its origin attributes, and set it on newChannel. This copies the security flags and the triggering principal, which is very much wrong. If only we had better necko regression tests...
Component: SVG → Networking: HTTP
Flags: needinfo?(honzab.moz)
Flags: needinfo?(ckerschb)
Oh, and this probably regressed with bug 1206961. :(
Blocks: 1206961
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Keywords: regression
Looks like when we started propagating the loadinfo to the post-redirect channel (bug 965413) it wouldn't cause inheritance if the initial channel itself was not inheriting... We should probably be changing SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS to SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL during redirects, but we should carefully audit all other loadinfo state to see whether it makes sense to propagate across a redirect...
(I confirmed that this is a new regression in Firefox 49. Cannot reproduce in Firefox 48. So, that supports the theory that bug 1206961 regressed this.)
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → unaffected
To be clear, the bug here is much more serious than just SVG images. The only reason this is not horrible universal XSS is because docshell doesn't use AsyncOpen2 yet (see bug 1182569). Even given that, a cross-site <object> load would allow the target site to redirect to data:text/html and execute arbitrary script with the permissions of the page loading the <object>. _That_ regressed with bug 1188642 for Firefox 50.
[I suspect this is sec-critical, given comment 20 --> bumping security rating.]
Keywords: sec-high → sec-critical
Good candidate for 50.1 if we get a fix soon
tracking-firefox50: ? → +
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → +
Top level page https://example.com <img src="https://people.mozilla.org/redirect.html"> redirect.html redirects to data:...<svg>...<meta http-equiv='Set-Cookie' content='ppp=qqq' />...</svg> (In reply to Daniel Holbert [:dholbert] from comment #10) > Quoting the documentation for that security flag: > "Loads from data: will inherit the principal of the origin that triggered > the load." > Who triggered the data: load? Right now it sounds like the loadInfo->loadingPrincipal and the loadInfo->triggeringPrincipal are both example.com. But one could say that the loadInfo->loadingPrincipal is example.com and the loadInfo->triggeringPrincipal is people.mozilla.org. Since people.mozilla.org "triggered" the data: load. What is the referrer for the data: load? If we changed the trigger to place the redirect came from (people.mozilla.org), the ppp=qqq cookie woudl get set for people.mozilla.org. Would that be acceptable? And would changing the trigger break other things? --- Regardless, it sounds like the better/safer option is to change the loadInfo security flags from SEC_ALLOW_ORIGIN_DATA_INHERITS to SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL when a non-internal redirect is involved. And then we should do a followup effort to audit the other values of loadInfo that copied over in HttpBaseChannel::SetupReplacementChannel, as per Comment 16.
Blocks: 1188642
Keywords: csectype-sop
Summary: SVG - set cookie in viewing domain in img tag → http redirect to data: inherits principal (SVG image cookie setting; object XSS)
Whiteboard: see comments 16 and 20
> Who triggered the data: load? Is the triggering principal the thing that triggered the _channel_, or the entire _load_? These are obviously not the same thing when redirects are involved. > What is the referrer for the data: load? In dholbert's testcase, example.com; referrer is preserved across http redirects.
Group: layout-core-security → core-security-release
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15) > > I'm not sure if that "real principal" empowers extra badness beyond the ability to set cookies... > > Unclear. Looking at comment 5, I'd say yes :/
I'm happy to throw some necko folks at this, but I'm not 100% sure of the fix. For now are we just going to clear SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS and set SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL on the new channel during redirects?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16) > So yeah, in HttpBaseChannel::SetupReplacementChannel we do: > > nsCOMPtr<nsILoadInfo> newLoadInfo = > static_cast<mozilla::LoadInfo*>(mLoadInfo.get())->Clone(); > > and then we change nothing about newLoadInfo except maybe its origin > attributes, and set it on newChannel. This copies the security flags and > the triggering principal, which is very much wrong. If only we had better > necko regression tests... This sounds to me like a suggestion for a fix here. The principal stuff has always been a bit clouded for me, so sorry if I write something stupid. I presume that when: example.com/ refers an <img> at foo.com/img.svg that redirects to bar.com/other-img.svg that redirects to data:, the final data load (when SEC_ALLOW_ORIGIN_DATA_INHERITS is set on the load info) should be codebase-principal of bar.com. Right? To me it sounds, when redirecting, we should replace triggeringPrincipal on the cloned load info to the most recent channel's codebase-principal. It's actually the channel that is redirecting which now triggers the log. But as I said, I really don't know what all implications this can have.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #27) > actually the channel that is redirecting which now triggers the log s/log/load/
> Looking at comment 5, I'd say yes :/ Reserving judgement until I see the bug report in question. > the final data load (when SEC_ALLOW_ORIGIN_DATA_INHERITS is set on the load info) > should be codebase-principal of bar.com. Right? No. The final data load should end up with a nullprincipal, not a codebase principal. Otherwise any open redirector becomes an XSS vector on the site the redirector is on. > To me it sounds, when redirecting, we should replace triggeringPrincipal That's possible, but orthogonal to the main issue. To be clear, the behavior we used to have before the loadinfo setup was as follows: 1) Redirects did not copy the "owner" to the post-redirect channel. 2) As a result, the post-redirect channel just ended up with a principal from its URI. In the case of data: that was a nullprincipal. We want to restore that behavior.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #29) > To be clear, the behavior we used to have before the loadinfo setup was as > follows: > > 1) Redirects did not copy the "owner" to the post-redirect channel. > 2) As a result, the post-redirect channel just ended up with a principal > from its URI. > In the case of data: that was a nullprincipal. > > We want to restore that behavior. Boris, do we want to distinguish between internal and external redirects? Is there any reason we would do an internal redirect to data:? Should we just change the security flag from SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS to SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL for any time of redirect?
Flags: needinfo?(tanvi) → needinfo?(bzbarsky)
(In reply to insertscript from comment #5) > (In reply to Daniel Holbert [:dholbert] from comment #2) > > (This reminds me a bit of bug 628747 [...] > > I know it is off-topic but I had a look at your posted bug report and I have > a variation of the same behavior, which still works. I will report it soon. (insertscript has now filed this as Bug 1319122, BTW.)
> Boris, do we want to distinguish between internal and external redirects? I don't think so. At least not in the HTTP code. > Is there any reason we would do an internal redirect to data: That's more a question for Honza than me; I don't remember all the circumstances that lead to internal redirects very well. > Should we just change the security flag from SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS to > SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL for any time of redirect? Probably yes. Note that it's not clear to me that this is enough. For example, what should happen with SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS on redirects? Should it become SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED or something else? Someone needs to test what other browsers do in the cases when we use that flag. We should also be clearing SEC_FORCE_INHERIT_PRINCIPAL on redirect, right? And SEC_ABOUT_BLANK_INHERITS? And so on, and so on. Someone needs to actually go through all the stuff on loadinfo and make these decisions. And then someone else needs to do it _again_ to review that patch.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33) > > Is there any reason we would do an internal redirect to data: > > That's more a question for Honza than me; I don't remember all the > circumstances that lead to internal redirects very well. No, all internal redirects are only to secure-updated or same URI as the source channel. > > > Should we just change the security flag from SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS to > > SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL for any time of redirect? > > Probably yes. > > Note that it's not clear to me that this is enough. For example, what > should happen with SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS on redirects? > Should it become SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED or something else? > Someone needs to test what other browsers do in the cases when we use that > flag. > > We should also be clearing SEC_FORCE_INHERIT_PRINCIPAL on redirect, right? > And SEC_ABOUT_BLANK_INHERITS? And so on, and so on. Someone needs to > actually go through all the stuff on loadinfo and make these decisions. And > then someone else needs to do it _again_ to review that patch. And tests would be great to have..
> And then someone else needs to do it _again_ to review that patch. To be clear, one of the things the resulting patch should have is a commit message that describes why all the things that are safe to propagate across redirects are safe to propagate. In an ideal world, the patch author and reviewer would both write down such a commit message independently and then compare notes.
I will have a patch tomorrow.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: see comments 16 and 20 → see comments 16 and 20 [necko-active]
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33) > Someone needs to > actually go through all the stuff on loadinfo and make these decisions. And > then someone else needs to do it _again_ to review that patch. I am working on this. And will likely need help/have questions for Boris. Will continue my analysis tomorrow.
Rather than change security flags upon redirects. Can we instead check the redirect chain in GetChannelResultPrincipal? I've also been thinking that we should change the triggeringPrincipal upon redirect. That would at least have ensured that we inherited the principal from the http-server which provided data: url here. It wouldn't have resulted in the right behavior, but it would have been less security critical I think. But this is probably a separate, defence-in-depth, bug.
> Can we instead check the redirect chain in GetChannelResultPrincipal? That's possible, if we just want to ignore all the "inherit the principal" bits for all redirects (not just HTTP ones). > That would at least have ensured that we inherited the principal from the http-server Note that the triggering principal is not what always gets inherited; see principalToInherit. But we could null that out on redirects.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #39) > > Can we instead check the redirect chain in GetChannelResultPrincipal? > > That's possible, if we just want to ignore all the "inherit the principal" > bits for all redirects (not just HTTP ones). > I do not know principals that well. Would we miss some cases if we implement it in this way. I think not but I will write what I had in mind and someone can confirm it: If we have a redirect similar to the example in this bug, but different to this bug the redirected channel triggers another request(channel), also with inherit flags. The last channel would not be a redirect so we will not detected as such but will have wrong principals. Probably that cannot happen, because the last channel will have the redirected uri as triggering uri?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #35) > > And then someone else needs to do it _again_ to review that patch. > > To be clear, one of the things the resulting patch should have is a commit > message that describes why all the things that are safe to propagate across > redirects are safe to propagate. > > In an ideal world, the patch author and reviewer would both write down such > a commit message independently and then compare notes. I can write a patch, but I am not the right person to evaluate what is safe to propagate and what not. Side note: It would be really good for me to learn the principals behavior. Are there any notes? :)
(In reply to Dragana Damjanovic [:dragana] from comment #41) > Side note: It would be really good for me to learn the principals behavior. > Are there any notes? :) ++1 !
(In reply to Dragana Damjanovic [:dragana] from comment #36) > I will have a patch tomorrow. I am not write person to decide what is the right approach. I will wait for a consensus :) And Tanvi's analysis (comment 37)
> We should also be clearing SEC_FORCE_INHERIT_PRINCIPAL on redirect, right? To answer that, for posterity: no. If we did that, we would break cross-site XHR across redirects because the resulting document would no longer have the principal of the page that did the XHR. This is also a reason to not change the triggeringPrincipal on redirect, or at least to set the principalToInherit in XHR code or something...
Attached patch bug_1317641.patch (obsolete) — — Splinter Review
(In reply to Dragana Damjanovic [:dragana] from comment #45) > Created attachment 8813357 [details] [diff] [review] > bug_1317641.patch This is a patch I have discussed with Tanvi.
(In reply to Dragana Damjanovic [:dragana] from comment #47) > (In reply to Dragana Damjanovic [:dragana] from comment #45) > > Created attachment 8813357 [details] [diff] [review] > > bug_1317641.patch > > This is a patch I have discussed with Tanvi. Sorry Dragana, I was not clear over IRC. We want to skip this block in the case we see a redirect in the redirect chain.
I've started a list of testcases we should test in Firefox (pre 49, 52, 53) and other browsers (Chrome to start with) and document - https://public.etherpad-mozilla.org/p/testcases. Boris, please add more test cases to the list. Some have live examples linked. Some of the results don't make much sense. We need to print the principals in nsScriptSecurityManager:GetChannelResultPrincipal to get a better idea of what is going on. Freddy, if you have time today during EU time, can you start printing these principals[1] and documenting their values for the given test cases. I will pick up where you left off tomorrow. Although I tried to figure out which LoadInfo flags to propagate and which not to propagate across redirects, it sounds like the better approach here is to check for redirects and set the appropriate principals in nsScriptSecurityManager:GetChannelResultPrincipal. Hence, we should skip the code here when redirectChain (or more conservatively redirectChainIncludingInternalRedirects) is set: http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/caps/nsScriptSecurityManager.cpp#329-348 Dragana's patch removes this code, since I didn't properly communicate that it should only be removed on redirect. Dragana, can you update your patch and repost to try? If try looks good, please flag Boris for review. [1] loadInfo->TriggeringPrincipal() loadInfo->PrincipalToInherit() loadInfo->LoadingPrincipal() and the aPrincipal that will get returned. loadInfo->GetExternalContentSecurityPolicyType() - and the content type
Flags: needinfo?(fbraun)
(In reply to Dragana Damjanovic [:dragana] from comment #40) > The last channel would not be a redirect so we will not > detected as such but will have wrong principals. Why won't this last channel not be a redirect? Maybe I'm not clear on what you mean by "triggers another request"...
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #50) > (In reply to Dragana Damjanovic [:dragana] from comment #40) > > The last channel would not be a redirect so we will not > > detected as such but will have wrong principals. > > Why won't this last channel not be a redirect? Maybe I'm not clear on what > you mean by "triggers another request"... I was also confused by this and talked to Dragana. She said something about the targeted URL (after redirect) opening a new channel and how that new channel wouldn't have a redirect chain. We aren't sure this is possible. The question boils down to: Can a redirected channel open another channel without proprogating the redirect chain and before GetChannelResultPrincipal is called?
Attached patch bug_1317641.patch (obsolete) — — Splinter Review
Attachment #8813357 - Attachment is obsolete: true
(In reply to Dragana Damjanovic [:dragana] from comment #52) > Created attachment 8813626 [details] [diff] [review] > bug_1317641.patch The try run is green. Bz, do you want review it? This patch does not fix any of the failing tests from Tanvi's etherpad.
Flags: needinfo?(bzbarsky)
I've been likely wasting too much time on writing a patch that prints the principal during GetChannelResultPrincipal, which I couldn't get it to print something useful for all(!) redirects, likely because I did not fully understand the whole function and touched the wrong parts. But I did write some test cases linked in the etherpad. Will update with results and additional test URLs while making progress on the patch on the side.
Flags: needinfo?(fbraun)
> Can a redirected channel open another channel without proprogating the redirect chain > and before GetChannelResultPrincipal is called? If it's some extension-implemented channel, presumably yes, by doing a redirect but not propagating the loadinfo at all or whatever. But then all bets are off anyway. Otherwise, I'd think no. What mechanism would allow such a thing?
Comment on attachment 8813626 [details] [diff] [review] bug_1317641.patch >+ if (!loadInfo->RedirectChain().Length() && if (loadInfo->RedirectChain().IsEmpty() && Apart from that, please add a comment saying that the data: inheritance flags should only apply to the initial load, not to loads that it might have redirected to. r=me with that.
Flags: needinfo?(bzbarsky)
Attachment #8813626 - Flags: review+
Attachment #8813626 - Attachment is obsolete: true
Attachment #8813819 - Flags: review+
Comment on attachment 8813819 [details] [diff] [review] bug_1317641.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? probably not that hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no Which older supported branches are affected by this flaw? from and including ff49 If not all supported branches, which bug introduced the flaw? bug 1206961 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Probably it will apply to all affected branches, and it is easy to to create backports if needed. low How likely is this patch to cause regressions; how much testing does it need? I am not sure, I am not the right person to answer that.
Attachment #8813819 - Flags: sec-approval?
(In reply to Dragana Damjanovic [:dragana] from comment #59) > Do comments in the patch, the check-in comment, or tests included in the > patch paint a bulls-eye on the security problem? > no Yes they do -- both the commit message and the code comment say "data: inherits privs on redirects". Doesn't explicitly mention the specific problems we've found but there's a finite number of interesting things to test. Since we're pushing out an out-of-cycle fix it's probably OK.
Note that the code change is clearly about redirects and clearly about data: and inheritance... Speaking of which, I think the commit message should be more like: Some loadinfo security flags should not apply in case of a redirect.
Comment on attachment 8813819 [details] [diff] [review] bug_1317641.patch [Triage Comment] sec-approval=dveditz a=dveditz for checking into branches including mozilla-release
Attachment #8813819 - Flags: sec-approval?
Attachment #8813819 - Flags: sec-approval+
Attachment #8813819 - Flags: approval-mozilla-release+
Attachment #8813819 - Flags: approval-mozilla-beta+
Attachment #8813819 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9935254c39eef9d14de419dd6163ff453cc1ce16 Also, should we really have been using a public etherpad for discussing testcases for a sec-crit?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #63) > Also, should we really have been using a public etherpad for discussing > testcases for a sec-crit? The testcases in the etherpad are not the ones in this bug and don't directly expose the vulnerability. The etherpad is expected to be expanded to include numerous test cases and behavior across browsers. I'm happy to move it to a google doc just in case.
This is a 50.0.1 dot release driver. Let's land this on m-r relbranch. I wil still wait for tanvi and team to finish up their testing before we decide to go live on this build.
Here's a testcase for the XSS <object> aspect of this bug. The key part is: <object style="border:1px solid black" data="https://people-mozilla.org/~dholbert/sekrit/object.html"></object> ...and that URL gets a 302 redirect to a data URI which uses scripts to read & display your cookie.
I verified that attachment 8813819 [details] [diff] [review] fixes the testcase in comment 8 (for setting cookies in SVG images) as well as the testcase in comment 67 (for XSS via <object>).
dholbert and I have created a number of testcases. I'll paste them in the bug shortly. dholbert and ekr tested with dragana's patch applied and all work as expected (no excessive cookie exposure). I am going to do a little more testing and will post to the bug shortly (within the next hour) with the go ahead.
Test cases: For each test case, start with a fresh profile. Open about:preferences#privacy and click "Use Custom Settings for History" under the History drop down. There is a Show Cookies button to access the Cookie Manager, which is whatt you will have to use to verify these test cases. You should clear all cookies between each test case. 1) https://dholbert.org/sekrit/test.html dholberts original test case. <img src=svg> with an svg that redirects to a data: that tries to sets a cookie on the parent page. Bad Outcome: In the Cookie Manager you see a ppp=qqq cookie for dholbert.com Good Outcome: In the Cookie Manager you see no cookie for dholbert.com 2) https://subdomain.bin.coffee/testObject.html <object data=svg> with svg that redirects to a data: that tries to set a cookie on the parent page. Bad Outcome: In the Cookie Manager you see a ppp=qqq cookie for subdomain.bin.coffee Good Outcome: Click Show Cookies and see no cookie for subdomain.bin.coffee 3) https://people-mozilla.org/~tvyas/testObject2.html (Pretty much identical to example 2) <object data=svg> with svg that redirects to a data: that tries to set a cookie on the parent page. Bad Outcome: Click Show Cookies and see a ccc=ddd cookie for people-mozilla.org Good Outcome: Click Show Cookies and see no cookie for people-mozilla.org 4) Visit https://subdomain.bin.coffee/index.html. This will set a cookie aaa=bbb on subdomain.bin.coffee. Confirm this in the Cookie Manager. Then visit https://subdomain.bin.coffee/testObject4.html. This has an <object data=html> that redirects to a data: uri that prints document.cookie. Bad outcome: aaa=bbb cookie is read by the cross domain object and printed to the page. Good outcome: no cookie printed to the page
I tested comment 70 with mozilla-central from today with this most recent changeset[1] and observed all the bad outcomes listed. Then I applied dragana's patch https://bugzilla.mozilla.org/attachment.cgi?id=8813819 applied. And observed all the good outcomes. With a quick look at the test cases in https://public.etherpad-mozilla.org/p/testcases, they seem to have the same result before and after the patch. Though note that we haven't spent enough time understanding the behavior of the test cases listed there. release management - if everything looks good on your end (treeherder, etc), then we are good to go here. We've done all we can in the time we have and have to hope that any regressed behavior would have been caught in our automated tests. [1] changeset: 323902:47f42f21541b tag: qparent parent: 323856:2a47a071271f parent: 323901:4772a0f4bed2 user: Carsten "Tomcat" Book <cbook@mozilla.com> date: Wed Nov 23 16:39:02 2016 +0100 summary: merge mozilla-inbound to mozilla-central a=merge
Nominating for bounty: critical vulnerability, can be triggered from content.
Flags: sec-bounty?
I investigated this issue on Windows 7 x64, Windows 8.1 x32, Ubuntu 16.04 x64 and Mac OS X 10.11.6, using latest Aurora 53.0a1 (2016-11-24), 51.0b3 build1 (20161124073320), 50.0.1 build1 (20161123182536) and the test cases provided in comment 70. The first three test cases generate the good outcomes. The fourth test case generates the bad outcome: after visiting https://subdomain.bin.coffee/index.html, the aaa=bbb cookie is set on subdomain.bin.coffee; but after accessing https://subdomain.bin.coffee/testObject4.html, COOKIE:'aaa=bbb' or COOKIE:'' is displayed. According to the fourth good outcome, no cookie should be printed to the page. Should a separate bug be filed for this issue?
I made a mistake in the previous comment. I intended to state "latest Aurora 52.0a2 (2016-11-24)" instead of "latest Aurora 53.0a1 (2016-11-24)". Sorry for that.
I cannot reproduce it on m-c. Is t possible that 50.0.1 build1 (20161123182536) does not have the patch? The patch landed on 24.11.
(In reply to Dragana Damjanovic [:dragana] from comment #76) > I cannot reproduce it on m-c. Is t possible that 50.0.1 build1 > (20161123182536) does not have the patch? The patch landed on 24.11. It does have it: http://ftp.mozilla.org/pub/firefox/candidates/50.0.1-candidates/build1/win32/en-US/firefox-50.0.1.txt 20161123182536 https://hg.mozilla.org/releases/mozilla-release/rev/d936940d5476b86717a43367dc2de820c7d171fa which is your patch.
Hi Dragana, Dan, based on SV testing and the fact that one of their test cases is failing, could you please let me know whether we need a follow up patch or we can push this build (50.0.1 build1) as is to the release channel? Thanks!
Flags: needinfo?(dveditz)
Flags: needinfo?(dd.mozilla)
Like Dragana, I can't reproduce the fourth-test-case issue that IuliaC described in comment 74. I just see COOKIE:'' when loading the second page there. (That's the expected/good outcome.) I'm using http://ftp.mozilla.org/pub/firefox/candidates/50.0.1-candidates/build1/linux-x86_64/en-US/firefox-50.0.1.tar.bz2 . IuliaC, could you double-check that you're really seeing 'aaa=bbb' when loading the second page of the fourth testcase? (If so, that is concerning & means this isn't fixed.)
Flags: needinfo?(iulia.cristescu)
(You said in comment 74: "COOKIE:'aaa=bbb' or COOKIE:'' is displayed". The former 'aaa=bbb' is bad; the latter '' is good. Which builds generated the former output, with aaa=bbb? If any supposedly-patched builds produced that output, then those are the builds we need to be worried about.)
I cannot reproduce it on linux or mac using builds from http://ftp.mozilla.org/pub/firefox/candidates/50.0.1-candidates/build1/ IuliaC, if you can reproduce it, I will need your help to debug it.
Tested again on Ubuntu 14.04 x86, Mac OS X 10.11.6, Windows 8.1 x86, Windows 7 x64, using 52.0a2 (2016-11-28), 51.0b3 (20161124073320) and 50.0.1 20161123182536. I couldn't reproduce anymore the COOKIE:'aaa=bbb' result. Therefore, the fourth testcase is passed.
Flags: needinfo?(iulia.cristescu)
(In reply to Ritu Kothari (:ritu) from comment #78) > based on SV testing and the fact that one of their test cases is failing, > could you please let me know whether we need a follow up patch of we can > push this build (50.0.1 build1) as is to the release channel? Further testing shows we are good to go.
Flags: needinfo?(dveditz)
Alias: CVE-2016-9078
Flags: needinfo?(dd.mozilla)
Whiteboard: see comments 16 and 20 [necko-active] → [adv-main50+] see comments 16 and 20 [necko-active]
Thanks Dragana for fixing. Clearing my needinfo.
Flags: needinfo?(ckerschb)
We can close this bug.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
status-firefox53: affected → fixed
Target Milestone: --- → mozilla53
Flags: sec-bounty? → sec-bounty+
See Also: → CVE-2017-7837
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: