941887 - (CVE-2014-1505) feDisplacementMap should check taintedness of the image with the displacements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Security sensitive for now since there's an information leak here that we're plugging.

There is really no way to prevent timing attacks revealing information about the contents of the displacements input to feDisplacementMap. These displacements control the addresses of memory fetches into the other input image. These fetches can be made fast or slow by concentrating them into one location or by spreading them out, due to cache effects. Making all the fetches equally slow would be difficult to do and result in horrible performance.

However, in many important use-cases of feDisplacementMap, we can easily prove that the displacements input image does not depend on content from a different origin than the page. So we can restrict feDisplacementMap to only work when we can prove that, instead of disabling it completely.

I suggest we do the following:
-- Assign an origin-clean flag to each filter primitive. By default, a filter primitive is origin-clean if and only if all of its inputs are origin-clean. (It follows that a filter primitive with no inputs is origin-clean.)
-- An feImage is origin-clean if drawing <img> element with the same source to an origin-clean canvas would leave the canvas origin-clean.
-- If the feDisplacementMap's displacements input is not origin-clean, it should produce transparent black.

Comment 1

[Dirk Schulze]

Note that the new security section in the Filter Effects specification defines when primitives should be tainted and how it effects feDisplacementMap http://dev.w3.org/fxtf/filters/#security. This section will be in a new working draft very soon. I would very much welcome feedback on this section.

Comment 2

[Markus Stange [:mstange]]

Attachment: v1

Sorry for the delay in getting to this.

I copied most of the security-relevant code from http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasUtils.cpp#33 .
Am I comparing the right principals?

I still need to write some tests.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8350589 [details] [diff] [review]
v1

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

::: content/svg/content/src/nsSVGFilters.h
@@ +104,5 @@
>    virtual bool AttributeAffectsRendering(
>            int32_t aNameSpaceID, nsIAtom* aAttribute) const;
>  
> +  virtual bool OutputIsTainted(const nsTArray<bool>& aInputsAreTainted,
> +                               nsIPrincipal* aReferencePrincipal);

Document this!

::: layout/svg/nsSVGFilterInstance.cpp
@@ +142,5 @@
> +  if (aInputIndex < 0) {
> +    // SourceGraphic, SourceAlpha, FillPaint and StrokePaint are tainted.
> +    return true;
> +  }
> +  return mPrimitiveDescriptions[aInputIndex].IsTainted();

Might as well inline this into its only caller

::: layout/svg/nsSVGFilterInstance.h
@@ +273,5 @@
>  
> +  bool InputIsTainted(int32_t aInputIndex);
> +
> +  void GetInputsAreTainted(const nsTArray<int32_t>& aInputIndices,
> +                           nsTArray<bool>& aOutInputsAreTainted);

Document this

Comment 4

[Markus Stange [:mstange]]

Attachment: v2

The spec says that feDisplacementMap with a tainted map should act as a pass-through filter, and not result in transparent black.

Also, when I wrote the tests I found out that the CORS check doesn't actually do anything because we never request the image with CORS headers because SVGFEImageElement does not override GetCORSMode to return something other than CORS_NONE. I've kept the check anyway.

Comment 5

[Markus Stange [:mstange]]

Attachment: tests

I haven't found a way to test cross-origin images with reftests, so I wrote a mochitest that has reftest-like behavior and uses the example.com trick to make cross-origin requests.

Comment 6

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a31d585b193

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a31d585b193

Does this affect other branches? If so, it should have had security approval before landing since it's sec-high.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8356650 [details] [diff] [review]
v2

Oops. I wasn't aware of the security patch landing protocol. Sorry for messing up there :(

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Medium to hard. The timing measurement part would probably be the hardest part.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really. The tests show what's not allowed any more, but they don't show how to exploit it if it's unpatched.

Which older supported branches are affected by this flaw?
All of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply on Aurora without problems because Aurora already contains the filter refactoring (bug 924103). Beta and Release would need a different patch, and I don't have one for them. The backport to Beta/Release would not be that hard to create. Risk of breaking something is low; displacement filters are not very commonly encountered on the web.

How likely is this patch to cause regressions; how much testing does it need?
Not that risky, the tests should cover it mostly.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8356650 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none, the problem has existed ever since we implemented SVG filters (I think in Firefox 3)
User impact if declined: potential privacy leak
Testing completed (on m-c, etc.): m-c for a day + manual testing + automated tests
Risk to taking this patch (and alternatives if risky): low, no alternatives
String or IDL/UUID changes made by this patch: none

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This issue is already mostly-public. See https://dvcs.w3.org/hg/FXTF/raw-file/default/filters/index.html#security and the email "[CSSWG] Minutes TPAC F2F 2013-11-12 Tues III: Filter Effects, CSS Transforms, Geometry API Spec".

So I think we should just go ahead and land wherever. This bug may as well remain private for now.

Comment 11

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

Comment on attachment 8356650 [details] [diff] [review]
v2

sec-approval+ for trunk and approval for aurora. Let's get it in.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/625da02b33fb

Go ahead and work on those branches :)

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/625da02b33fb along with the other three commits from Ryan's uplift because one of the four caused windows m5 test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=32842361&tree=Mozilla-Aurora

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Doubtful this was at fault. Re-landed under that assumption.
https://hg.mozilla.org/releases/mozilla-aurora/rev/35578d712031

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Well this is fun. This patch apparently did "cause" the failures in comment 13 (again, only on Windows debug). So it's backed out again.
https://hg.mozilla.org/releases/mozilla-aurora/rev/10f42977b6b0

That said, we hit a very similar version of this failure on OSX a couple days ago, filed as bug 957428. And to make matters more fun, we hit the *exact* same failure on Windows debug on a push earlier yesterday. What changed with this patch is the frequency. The below link shows Aurora's Windows debug M5 runs over the last couple days, along with all the retriggers. You can see the one isolated failure on 30ab64d5e3f4 (and none on the retriggers), and you can see how the frequency went to near perma-fail levels on all the pushes with this patch in included.

https://tbpl.mozilla.org/?tree=Mozilla-Aurora&jobname=Win.*debug%20test%20mochitest-5&fromchange=031d05383ab8&tochange=tip

So I guess this is somehow exacerbating a preexisting issue or something?

Comment 16

[Markus Stange [:mstange]]

Huh, interesting. I don't think it's the code change that causes the test to fail, but the new test that the patch adds (test_taintedfilters.html), because that also runs in mochitest-5, though much earlier.

Maybe we should land the patch on Aurora without the tests (i.e. leave out all the changes under layout/generic/test/).
We can still debug bug 957428 by experimenting on top of Aurora revision 625da02b33fb, for example.

Comment 17

[Dirk Schulze]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)

I am curious what the spec does if the input of feDisplacementMap is tainted with this patch. Does it make feDisplacementMap a pass through as demanded in the spec? Ignore the filter chain? Do the following?

> -- If the feDisplacementMap's displacements input is not origin-clean, it
> should produce transparent black.

I think returning transparent black is the least desirable result for tainted filters. I am not sure if a passthrough is desirable either. At this point I tend to ignoring the filter chain completely.

Comment 18

[Markus Stange [:mstange]]

See comment 4; the first patch produced transparent black but the revised and landed patch follows the spec, in that it makes feDisplacementMap with tainted map act as a pass through filter.

Comment 19

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #18)
> See comment 4; the first patch produced transparent black but the revised
> and landed patch follows the spec, in that it makes feDisplacementMap with
> tainted map act as a pass through filter.

Ok, thanks for the info.

Comment 20

[Daniel Veditz [:dveditz]]

Given the landing troubles on Aurora there's no way we're going to get this into Beta (27) at this late date. Nor do we really want to risk it on b2g18 which isn't getting a lot of testing and is almost EOL. We may want this on ESR-24 if we're confident this patch won't cause too much trouble

Comment 21

[Markus Stange [:mstange]]

I talked to Jet about the Aurora troubles here. His stance was that since this issue has been public for a while now, there's no particular rush to fix it on Aurora if it creates additional work, so we should just let it ride the trains.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

What about branches that aren't on the trains? Namely, esr24, b2g26, and b2g18?

Comment 23

[Markus Stange [:mstange]]

I don't know. Who can make this choice? Roc maybe?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

And actually, the fix for bug 948782 may fix the orange we are seeing on orange. Running it through Try now.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Fun story, something else managed to perturb that test into failing regularly, and according to Try, this patch now re-stabilizes it! That said, the run with bug 948782 included is also solid green, so I'm going to say that makes life better and push it as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7124d82a8ef

Comment 26

[Markus Stange [:mstange]]

Yay! Thanks for the help, Ryan.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> What about branches that aren't on the trains? Namely, esr24, b2g26, and
> b2g18?

I don't think this is worth uplifting.