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

RESOLVED FIXED in Firefox 20

Status

()

RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

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

Trunk
mozilla21
crash, regression, sec-critical
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)

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;
};
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Comment 3

6 years ago
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
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
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.
status-firefox18: --- → unaffected
status-firefox19: --- → unaffected
status-firefox20: --- → affected
status-firefox21: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
(Assignee)

Comment 8

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

Updated

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

Comment 11

6 years ago
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?

Updated

6 years ago
tracking-firefox20: ? → +
tracking-firefox21: ? → +
Keywords: regression
Keywords: crash
Attachment #702671 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/78e41d936992
Status: NEW → RESOLVED
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.
(Assignee)

Comment 15

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6ae2d02d417
status-firefox20: affected → fixed

Updated

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 moz.build files)
Flags: needinfo?(dzbarsky)
(Assignee)

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

Updated

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