Closed Bug 1319122 (CVE-2016-9900) Opened 7 years ago Closed 7 years ago

SVG-as-an-image sends requests for external files, if they're included in a data URI

Categories

(Core :: SVG, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 50+ fixed
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: alex, Assigned: dholbert)

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(5 files, 2 obsolete files)

Attached file svg_leak_poc.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

Tested: 
Windows 8.1 - Firefox Nightly 53.0a1
Windows 7 - 50.0


SVG images loaded via an img tag are restricted on the support for external resources.
See Bug https://bugzilla.mozilla.org/show_bug.cgi?id=628747

This restriction fails as soon as the SVG file loads a second SVG file via the fill attribute.
Note that it could be that other SVG attributes could be used as well, I haven't checked all of them so far. 

As soon as the second SVG is parsed, Firefox fetches different remote resources. 
The PoC will load different resources from example.com to proof that.
Additionally the data: protocol was used inside the fill attribute, to have the PoC in one place.


Actual results:

After the SVG was rendered remote files are fetched.


Expected results:

Taking into consideration that developers assume that images can not trigger different connections to external resources, this could cause privacy issues.
Dan, am I right in thinking you might know what's going on here?
Group: firefox-core-security → core-security
Component: Untriaged → SVG
Flags: needinfo?(dholbert)
Product: Firefox → Core
(In reply to insertscript from comment #0)
> As soon as the second SVG is parsed, Firefox fetches different remote
> resources. 
> The PoC will load different resources from example.com to proof that.

Can you provide more specific steps, and clarify what precise behavior you're seeing & how you're sure that resources from example.com are being requested?

I tried the following steps, locally:
 (1) Extract the attached PoC zip file.
 (2) In Firefox, open devtools (F12) and click Network devtools-tab.
 (3) In this browser-tab, open the "load_svg.html" from the extracted zip.

EXPECTED RESULTS:
 No resources from example.org should show up in the network requests devtools.

(Presumably the buggy ACTUAL RESULTS might be something like "a bunch of loads appear for resources from example.com" -- but that's not what I see. I only see example.com loads if I load the SVG file directly -- but that's expected, because we give SVG more privileges as a top-level-document than we do as an image.)

FWIW, I tried loading load_svg.html as a local file (over file:// protocol) and also over HTTP (using "python -m SimpleHTTPServer 8000" to start a local HTTP server in its directory).  In the former case, devtools shows me zero requests (because the files are all local, presumably). In the latter case, devtools shows me loads for load_svg.html and leak.svg, and nothing else.
Flags: needinfo?(dholbert) → needinfo?(alex)
I'm testing using Firefox 50 on Linux, BTW (which is the same version that the reporter was testing, though he's on Windows 7).
I checked by attaching a proxy and via Access Logs of my Apache Server.
Sorry I should have mentioned that I saw no requests in the dev tools, I only spotted them via logs/proxy.
I hosted a version of the PoC here:
http://wargame.000webhostapp.com/load_svg.html (sry for the **** free hoster but my other server is down) 

I just tried and verified, that I see some requests hitting my web server.

After attaching an proxy you should see a connection to 84.112.240.90 (my local webserver).

While testing I suggest using a private window everytime opening the app.

I watch the apache logs and should see as soon as my PoC is opened. 
I haven't tested Linux, but I will have a look at it now!

I hope it will work for you now

best regards
Just came in:

63.245.221.32 - - [21/Nov/2016:21:09:58 +0100] "GET /css-import-url HTTP/1.1" 404 508 "-" "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0"
Thanks. I believe I can reproduce locally, using a second python-SimpleHTTPServer instance for the "canary" 3rd-party resources (replacing example.com in your testcase with a local URL to the second webserver).  The "canary" SimpleHTTPServer does indeed log some requests for external resources.  (And those loads indeed aren't shown in devtools. :-/  Maybe they're speculative or something? I guess I'll see after I dig in deeper with a debugger.)

I'll post a simple testcase later today, that others can run locally using SimpleHTTPServer.

I can reproduce in older builds, too (e.g. I tested Firefox 42 and triggered it there), so this is not a recent regression & is probably unrelated to bug 1317641.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alex)
Summary: SVG-as-an-image loads external files → SVG-as-an-image sends requests for external files, if they're included in a data URI
Presumably the check on doc->IsBeingUsedAsImage() in nsDataDocumentContentPolicy::ShouldLoad should happen on doc->GetDisplayDocument() in the case when doc->GetDisplayDocument() is non-null.
STR with testcase 2:
 (1) Unzip the testcase into some directory.
 (2) Start two HTTP Servers in that directory:
    cd path/to/files
    python -m SimpleHTTPServer 8000
    python -m SimpleHTTPServer 9999

 (3) View the PoC on the first HTTP server -- i.e. visit:
   http://localhost:8000/loadme.html

 (4) Check your terminal output for the second HTTP server.

EXPECTED RESULTS:
No terminal output for second HTTP server. It shouldn't have received any requests.

ACTUAL RESULTS:
The second server has this output:
127.0.0.1 - - [21/Nov/2016 12:52:20] code 404, message File not found
127.0.0.1 - - [21/Nov/2016 12:52:20] "GET /canary HTTP/1.1" 404 -

This indicates that your browser made a request for http://localhost:9999/canary which is an external resource referenced in a data URI within the SVG image that was being served by the first server.
The data URI inside the SVG image in Testcase 2 is just an encoding of the following:
<svg xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg"><feImage xlink:href="http://localhost:9999/canary"/></svg>
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> Presumably the check on doc->IsBeingUsedAsImage() in
> nsDataDocumentContentPolicy::ShouldLoad should happen on
> doc->GetDisplayDocument() in the case when doc->GetDisplayDocument() is
> non-null.

Yeah, I think this is exactly right.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached patch fix v1Splinter Review
I'll add a second patch with an automated testcase.

(I've included a brief code-comment to assist with review, but I plan on ripping out that comment before landing -- and I'll add it back eventually as part of the patch that adds automated tests, once we've shipped the fix to all affected users.)

bz, Bugzilla tells me you're not currently accepting review requests, but I'm wondering if you'd make an exception & review this, since this is a simple patch to a cascade of security checks that you understand better than most.
Flags: needinfo?(bzbarsky)
BTW: I tested the attached patch with my reduced PoC as well as with the original PoC (modified to use a local webserver that I can monitor), and I verified that I'm not seeing any external requests in a build with this patch applied.
We should try to get this fix into the 50.1(?) release planned for December 13.
Comment on attachment 8812960 [details] [diff] [review]
fix v1

r=me
Flags: needinfo?(bzbarsky)
Attachment #8812960 - Flags: review+
Thanks!  I'm partway through coming up with an automated test here. Once I've got that, I'll reupload the patch with comments removed and request security approval & uplift approval.
Flags: needinfo?(dholbert)
(Automated test still isn't quite done, but its partially-completed state does successfully test the bug & verify the fix.  So, let's get the ball rolling on approvals/landing here. --> reuploading patch with a (vague) commit message, and with the code-comment removed, to be on the safe/obscure side.

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
  Not super easily. Depends on how much an attacker can figure out that "display document" points to external-resources-docs and filters/use/mask/etc.  But even then, it's not easy to come up with a PoC that does anything particularly nasty.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  No. I've intentionally removed the code-comment that explains the new check (to be added back when the fix has reached users), and the commit message is intentionally hand-wavy. (just directly describing the code-change).

* Which older supported branches are affected by this flaw?
 Everything back to Firefox 4, I think. :-/

* If not all supported branches, which bug introduced the flaw?
 N/A.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  This patch should apply cleanly to all supported branches (and in the unlikely event that I'm wrong, it's trivial to adapt it).

* How likely is this patch to cause regressions; how much testing does it need?
  Unlikely. Not much testing needed, other than the local testing that I've already done to ensure that it fixes the bug.
Attachment #8813465 - Flags: sec-approval?
Attachment #8813465 - Flags: review+
Attachment #8813465 - Attachment description: fix v1a, for landing (comment removed → fix v1a, for landing (with explanatory code-comment removed)
Comment on attachment 8813465 [details] [diff] [review]
fix v1a, for landing (with explanatory code-comment removed)

sec-approval+. We should backport this patch to branches.
Attachment #8813465 - Flags: sec-approval? → sec-approval+
Group: core-security → layout-core-security
Nominating for bounty: sec-high, triggered from content. big regression window.
Flags: sec-bounty?
Comment on attachment 8813465 [details] [diff] [review]
fix v1a, for landing (with explanatory code-comment removed)

Requesting approval to uplift this fix to all supported branches (including release & latest ESR).

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high
User impact if declined: Possibility for cross-domain data leakage via SVG images.
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: SVG as an image (and the security restrictions we tried to place in bug 628747, which weren't quite sufficient, per this bug)
[User impact if declined]: Possibility for cross-domain data leakage via SVG images.
[Is this code covered by automated tests?] Not yet - it's a security bug. I'll land an automated test once we're ready to open this up though.
[Has the fix been verified in Nightly?] Not yet; only just landed there.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 9.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This just makes an existing security check a bit more strict, in a very edge-casey scenario which actual web content is extremely unlikely to depend on.
[String changes made/needed]: None.
Attachment #8813465 - Flags: approval-mozilla-release?
Attachment #8813465 - Flags: approval-mozilla-esr45?
Attachment #8813465 - Flags: approval-mozilla-beta?
Attachment #8813465 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/952640eb7c56
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8813465 [details] [diff] [review]
fix v1a, for landing (with explanatory code-comment removed)

Sec-high, approving for uplift to all affected branches.
Attachment #8813465 - Flags: approval-mozilla-release?
Attachment #8813465 - Flags: approval-mozilla-release+
Attachment #8813465 - Flags: approval-mozilla-esr45?
Attachment #8813465 - Flags: approval-mozilla-esr45+
Attachment #8813465 - Flags: approval-mozilla-beta?
Attachment #8813465 - Flags: approval-mozilla-beta+
Attachment #8813465 - Flags: approval-mozilla-aurora?
Attachment #8813465 - Flags: approval-mozilla-aurora+
Unless I hear otherwise, I'll act on the approvals & uplift this to mozilla-release/mozilla-esr45 sometime tomorrow, so as not to get this confused with the 50.0.2 landings (for another bug) that are happening tonight.

(I believe this particular bug is targeted to ship in 50.1.0, which isn't going out quite yet.)
Landing on release is fine. The 50.0.2 build is happening off a relbranch so there's no worries there. Please *do* hold off on the esr45 uplift, though. That's a bit more of a complicated situation. Once the 45.5.1 build gets signoffs, you should be OK to push.

https://hg.mozilla.org/releases/mozilla-release/rev/7fde3fa7a210
Thanks, RyanVM!
Can't write a 50.1 advisory for this without 0day'ing ESR45 since ESR45 doesn't ship until January. This should not have been accepted into 50.1.
Flags: needinfo?(abillings)
Ok. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.
Flags: needinfo?(abillings)
Whiteboard: [adv-main50.1+][adv-esr45.6+]
Alias: CVE-2016-9900
Group: layout-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Flags: needinfo?(dholbert)
Here's a mochitest for this bug, and the code-comment that I omitted from the actual patch (to avoid giving away the reason for the code change at the time).

xidorn, I'm hoping you can give this a sanity-check review -- this is similar in structure to the HSTS testcase that you recently reviewed some changes to, over in bug 1329045.

I've verified that the mochitest passes with recent mozilla-central, and that it fails if I back out the fix (as expected). (Specifically, the "svg in img" vs "lime reference div" snapshot-comparison fails, in a build without the fix.)
Attachment #8826876 - Flags: review?(xidorn+moz)
(updated patch to make some minor fixes to a comment in one of the test files)
Attachment #8826876 - Attachment is obsolete: true
Attachment #8826876 - Flags: review?(xidorn+moz)
Attachment #8826879 - Flags: review?(xidorn+moz)
(Also, dveditz: can we un-hide this bug, given that its fix has shipped on all supported branches?)
Flags: needinfo?(dveditz)
Comment on attachment 8826879 [details] [diff] [review]
followup patch v2: add code-comment and mochitest

Review of attachment 8826879 [details] [diff] [review]:
-----------------------------------------------------------------

This test looks fine to me.

I wonder whether it's worth adding a test to ensure that we really don't trigger any connection (rather than just don't render it). I don't have a good idea about how to test that. Probably we can have a test using a url with non-local address, and expect the process to crash when it tries to establish the external connection.

::: dom/svg/test/svg-with-external-resource-datauri.svg
@@ +16,5 @@
> +       should be allowed to load (in which case the rect below renders as red).
> +  -->
> +
> +  <rect width="100" height="100" stroke="blue" fill="orange"
> +        filter="url(data:image/svg+xml;base64,PHN2ZyB4bWxucz0naHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmcnIHhtbG5zOnhsaW5rPSdodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rJz48ZGVmcz48ZmlsdGVyIGlkPSdNeUZpbHRlcicgeD0nMCUnIHk9JzAlJyB3aWR0aD0nMTAwJScgaGVpZ2h0PScxMDAlJz48ZmVJbWFnZSB4bGluazpocmVmPSdodHRwOi8vZXhhbXBsZS5vcmcvdGVzdHMvZG9tL3N2Zy90ZXN0L3JlZC5wbmcnLz48L2ZpbHRlcj48L2RlZnM%2BPC9zdmc%2BCg%3D%3D#MyFilter)">

I don't think you need to encode it in base64 form.

data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'>
  <defs>
    <filter id='MyFilter' x='0%' y='0%' width='100%' height='100%'>
      <feImage xlink:href='http://example.org/tests/dom/svg/test/red.png'/>
    </filter>
  </defs>
</svg>#MyFilter

should work fine, and it can save some description from the comment. But it's up to you.

::: dom/svg/test/test_bug1319122.html
@@ +59,5 @@
> +
> +  // Convenience helper which sets elem.src to the given uri. Returns a promise
> +  // that resolves when load fires on elem. This allows for handy async loading
> +  // in <img> and <elem> using 'await' instead of onload callback hell.
> +  async function SetSrcAndLoad(elem, uri) {

In our js code, I think function names should use lower camelCase.
Attachment #8826879 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #35)
> I wonder whether it's worth adding a test to ensure that we really don't
> trigger any connection (rather than just don't render it). I don't have a
> good idea about how to test that.

Perhaps! I bet we could come up with something using a .sjs file, with assertions in the .sjs (direct or indirect) that no connection request was received. For now, I think this rendering-based test is sufficient, though.

> Probably we can have a test using a url
> with non-local address, and expect the process to crash when it tries to
> establish the external connection.

Hmm, I suppose that could work too, though I feel odd making a test *directly rely* on that "no remote connections" check of the test harness.  (Plus: such a test wouldn't be able to compare against the <embed> behavior, because that would crash as well due to the external connection.  And I think it's important that we're able to include a direct comparison against <embed> for this sort of testcase, to prove that the SVG content we're using *is indeed capable* of issuing a request for the external resource, when loaded in the proper context.)

> I don't think you need to encode it in base64 form.

Yeah, I agree it's preferable to avoid base64, but I had a hard time getting valid non-base64 data URIs to be accepted in the "filter" property. Your comment prompted me to try again and figure out why it wasn't working for me, though, and I figured it out -- CSS "<string>"/"<uri>" tokens cannot include newline characters, apparently.  (Also, SVG attributes cannot include angle-braces -- so we can't put a data URI with angle-brackets directly into the filter="..." or style="filter:..." attribute.)

So, with that figured out, I ended up getting rid of base64 by pulling this CSS up into a <style> element, and placing the human-readable-data-URI all on a single line (no newlines) in that style element.  Hooray! I kept the existing comment mostly-intact, though perhaps it's a bit overly verbose now. *shrug*  The one-liner-data-URI is still obfuscated enough that it's nice to explain it clearly.

> In our js code, I think function names should use lower camelCase.

Ah, right you are - thanks! Fixed.
Here's the followup test-patch again, with review comments addressed as noted above. Thanks for the review!

I'll hold off on landing this until we're officially cleared to open up this bug, since usually the conditions for unhiding & testcase-publishing are the same. (I'm pretty sure we're at a place where both actions are OK, as I noted in comment 34, since the fix has shipped on all supported branches including esr45.)
Attachment #8826879 - Attachment is obsolete: true
Attachment #8826909 - Flags: review+
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.