SVGPointList shouldn't throw when methods are called with points that are not DOMSVGPoint

RESOLVED FIXED in Firefox 20



6 years ago
5 months ago


(Reporter: dzbarsky, Assigned: dzbarsky)


(Depends on: 1 bug, {crash, regression, sec-critical})

crash, regression, sec-critical
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18 unaffected, firefox19 unaffected, firefox20+ fixed, firefox21+ fixed, firefox-esr17 unaffected, b2g18 unaffected)


(Whiteboard: [adv-main20-])


(3 attachments)

Comment hidden (empty)
Bindings will throw before SVGPointList methods see the parameter unless overloaded methods are added such as:
  SVGPoint initialize(SVGPointInit newItem);

SVGPointInit would be a disctionary somthing like:
dictionary SVGPointInit {
  float x;
  float y;

Comment 2

6 years ago
nsSVGTranslatePoint::DOMVal is an SVGPoint, but not a DOMSVGPoint so SVGPointList methods will throw for it.
Blocks: 816778

Comment 3

6 years ago
In fact, this crashes:

document.createElementNS("", "polyline").points.appendItem(document.createElementNS("", "svg").currentTranslate)
Assignee: nobody → dzbarsky
I can confirm that crash. In my debug build, we end up calling a virtual method (nsSVGElement::GetAnimatedPointList) on a deleted pointer. (0x0x5a5a5a5a5a5a5a5a)

 --> Marking security-sensitive
Group: core-security
Flags: in-testsuite?
Version: unspecified → Trunk
Keywords: sec-critical
Created attachment 702550 [details]
testcase (crashes firefox)

Here's the testcase from comment 3, just with <!DOCTYPE html><script>...</script> added.
Created attachment 702554 [details]
backtrace of crash
Regression range for crash, unsurprisingly, points to bug 816778:
Last good nightly: 2012-12-24
First bad nightly: 2012-12-25
(which agrees w/ comment 2)

So, Firefox 20 (currently on Aurora) is vulnerable to the crash, but earlier releases are not. Marking flags accordingly.
status-firefox18: --- → unaffected
status-firefox19: --- → unaffected
status-firefox20: --- → affected
status-firefox21: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?

Comment 8

6 years ago
Created attachment 702671 [details] [diff] [review]
Attachment #702671 - Flags: review?


6 years ago
Attachment #702671 - Flags: review? → review?(longsonr)
Attachment #702671 - Flags: review?(longsonr) → review+
Testcase needs turning into a crashtest to be checked in at a later date.
Note that, since this affects a release branch (Aurora), I'm pretty sure you need to request security-approval before landing.

Comment 11

6 years ago
Comment on attachment 702671 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
We call a virtual method on a deleted pointer

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. test will be checked in later.

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?
Bug 823394

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?
Not likely
Attachment #702671 - Flags: sec-approval?
Attachment #702671 - Flags: approval-mozilla-aurora?


6 years ago
tracking-firefox20: ? → +
tracking-firefox21: ? → +
Keywords: regression
Keywords: crash
Attachment #702671 - Flags: sec-approval? → sec-approval+
Last Resolved: 6 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
Attachment #702671 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Approving for aurora so we don't ship this regression.

Comment 15

6 years ago
status-firefox20: affected → fixed


6 years ago
Depends on: 846710
Whiteboard: [adv-main20-]
This bug's checkin (in comment 12) added a redundant copy of the file "nsISVGPoint.cpp" in content/svg/content.
     1.1 copy from content/svg/content/src/DOMSVGPoint.cpp
     1.2 copy to content/svg/content/nsISVGPoint.cpp
[...]    12.1 copy from content/svg/content/src/DOMSVGPoint.cpp
    12.2 copy to content/svg/content/src/nsISVGPoint.cpp

So now we've got two files named nsISVGPoint.cpp.

I suspect the one that's directly in content/svg/content/ should just be removed, correct?  (I don't think it's compiled, since it's not mentioned in any files)
Flags: needinfo?(dzbarsky)

Comment 19

5 years ago
Oops!  Yep, the one in content/svg/content is bogus. r=me if you want to remove it.
Flags: needinfo?(dzbarsky)
Depends on: 907565
Cool - I filed bug 907565 to remove it, and just pushed a patch to do that.
Group: core-security


5 months ago
Depends on: 1459905
You need to log in before you can comment on or make changes to this bug.