Closed Bug 821955 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

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

Attachments

(4 files)

testcase (crashes firefox)
199 bytes, text/html
Details
backtrace of crash
5.87 KB, text/plain
Details
Patch
41.87 KB, patch
longsonr
: review+
lsblakk
: approval-mozilla-aurora+
abillings
: sec-approval+
Details | Diff | Splinter Review
Bug 821955 - Add crashtest
47 bytes, text/x-phabricator-request
Details | Review 
      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.
status-firefox18: --- → unaffected
status-firefox19: --- → unaffected
status-firefox20: --- → affected
status-firefox21: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
Attached patch Patch — — Splinter 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: 11 years ago

          status-firefox21:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

        Attachment #702671 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Approving for aurora so we don't ship this regression.
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
No longer depends on: 1459905
Regressions: 1459905
Regressions: 1661509
Flags: in-testsuite? → in-testsuite+

          You need to log in
          before you can comment on or make changes to this bug.