Closed Bug 821955 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + fixed
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

(Depends on 1 open bug)

Details

(Keywords: crash, regression, sec-critical, Whiteboard: [adv-main20-])

Attachments

(3 files)

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.
Attached 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: 8 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.