1319122 - (CVE-2016-9900) SVG-as-an-image sends requests for external files, if they're included in a data URI

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[insertscript]

Attachment: 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.

Comment 1

[:Gijs (out for now; he/him)]

Dan, am I right in thinking you might know what's going on here?

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[insertscript]

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

Comment 5

[insertscript]

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"

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Presumably the check on doc->IsBeingUsedAsImage() in nsDataDocumentContentPolicy::ShouldLoad should happen on doc->GetDisplayDocument() in the case when doc->GetDisplayDocument() is non-null.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Testcase 2 (reduced, zipped)

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

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>

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Daniel Veditz [:dveditz]]

We should try to get this fix into the 50.1(?) release planned for December 13.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8812960 [details] [diff] [review]
fix v1

r=me

Comment 17

[Daniel Holbert [:dholbert]]

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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: fix v1a, for landing (with explanatory code-comment removed)

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

Comment 19

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

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.

Comment 20

[Frederik Braun [:freddy]]

Nominating for bounty: sec-high, triggered from content. big regression window.

Comment 21

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/952640eb7c568ae5794667cd78c6b9a080d3baee

Comment 22

[Daniel Holbert [:dholbert]]

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.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/952640eb7c56

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 25

[Daniel Holbert [:dholbert]]

Uplifted to Aurora/Beta:
  https://hg.mozilla.org/releases/mozilla-aurora/rev/48d3a6fbc8823d3ebb194890f368ad1947967f7f
  https://hg.mozilla.org/releases/mozilla-beta/rev/9afe68360fa82c16b760b448b2156230a90caf11

Comment 26

[Daniel Holbert [:dholbert]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/994d9bd0e28d

Comment 29

[Daniel Holbert [:dholbert]]

Thanks, RyanVM!

Comment 30

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

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.

Comment 31

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

Ok. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: followup patch: add code-comment and mochitest

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

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: followup patch v2: add code-comment and mochitest

(updated patch to make some minor fixes to a comment in one of the test files)

Comment 34

[Daniel Holbert [:dholbert]]

(Also, dveditz: can we un-hide this bug, given that its fix has shipped on all supported branches?)

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 36

[Daniel Holbert [:dholbert]]

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

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: followup patch v3: add code-comment and mochitest [r=xidorn]

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