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

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

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

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main20-])

Attachments

(3 attachments)

No description provided.
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;
};
nsSVGTranslatePoint::DOMVal is an SVGPoint, but not a DOMSVGPoint so SVGPointList methods will throw for it.
See https://bug816778.bugzilla.mozilla.org/attachment.cgi?id=691718
Blocks: 816778
In fact, this crashes:

document.createElementNS("http://www.w3.org/2000/svg", "polyline").points.appendItem(document.createElementNS("http://www.w3.org/2000/svg", "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
Here's the testcase from comment 3, just with <!DOCTYPE html><script>...</script> added.
Regression range for crash, unsurprisingly, points to bug 816778:
{
Last good nightly: 2012-12-24
First bad nightly: 2012-12-25
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d348dbf1dab4&tochange=dc2abccc2adb
}
(which agrees w/ comment 2)

So, Firefox 20 (currently on Aurora) is vulnerable to the crash, but earlier releases are not. Marking flags accordingly.
Posted patch PatchSplinter Review
Attachment #702671 - Flags: review?
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 on attachment 702671 [details] [diff] [review]
Patch


[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?
Aurora

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?
Yes

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?
Keywords: crash
Attachment #702671 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/78e41d936992
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Attachment #702671 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Approving for aurora so we don't ship this regression.
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 moz.build files)
Flags: needinfo?(dzbarsky)
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
Depends on: 1459905
You need to log in before you can comment on or make changes to this bug.