Bug 39253 - NULL deref in SVG FilterEffect::getEffectContext()
Summary: NULL deref in SVG FilterEffect::getEffectContext()
Status: RESOLVED FIXED
Alias: None
Product: Security
Classification: Unclassified
Component: Security (show other bugs)
Version: Other
Hardware: All All
: P2 Minor
Assignee: WebKit Security Group
URL: http://upload.wikimedia.org/wikipedia...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-17 17:45 PDT by Justin Schuh
Modified: 2012-05-10 08:17 PDT (History)
10 users (show)

See Also:


Attachments
Crashing SVG (77.36 KB, image/svg+xml)
2010-05-17 17:46 PDT, Justin Schuh
no flags Details
Reduced testcase (1.62 KB, image/svg+xml)
2010-06-24 02:15 PDT, Nikolas Zimmermann
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Schuh 2010-05-17 17:45:47 PDT
Originally reported by delcypher to http://crbug.com/43529

This looks like a simple NULL deref where FilterEffect::getEffectContext() should be checking the return value of ImageBuffer::create(). However, there does seem to be an issue with the filter processing, because the results seem well outside the range of where they should be.

The file is a Wikipedia graphic; I've attached a copy in case the version at the server changes.
Comment 1 Justin Schuh 2010-05-17 17:46:28 PDT
Created attachment 56296 [details]
Crashing SVG
Comment 2 David Kilzer (:ddkilzer) 2010-05-17 17:49:09 PDT
<rdar://problem/7995433>
Comment 3 Abhishek Arya 2010-06-21 21:07:56 PDT
+ccing the SVG folks @krit, zimmermann
Comment 4 Dirk Schulze 2010-06-21 22:58:17 PDT
(In reply to comment #3)
> +ccing the SVG folks @krit, zimmermann

We had a crash report about the same graphic but without filters at bug 40837. Strange. Indeed there is a check for ImageBuffer missing, but if the ImageBuffer is not created, the filter effect won't be applied to the shape. There is a bug in <use>, it doesn't give back the correct size, so that the image size is often as big as the viewport size and in the case of the graphic it would be huge. Maybe this can influence the image creation somehow. I'll check it.
Comment 5 Justin Schuh 2010-06-23 07:20:37 PDT
+wjmaclean@chromium.org from bug 40837

krit - The image definitely changed at the server. Last time I looked at this I was almost certain it was a simple NULL deref (so, not a security issue). However, I hadn't worked out what was causing the extremely large filter values, so I filed as security just to be safe. It sounds like I was hitting the use bug you're describing. If that's the case, and it's just a simple NULL deref / resource consumption thing, then I think it's safe to drop the security flags.
Comment 6 W. James MacLean 2010-06-23 09:10:55 PDT
(In reply to comment #5)

The cause of the extremely large bitmap size appears to have been in SVGRootInlineBox::layoutInlineBoxes(), where a non-text inline flow box *without* children generates extremely large offset values based on INTMAX.

This code seems to have been completely re-architected recently, with the function I mention above having completely disappeared.

The new code *doesn't seem* to have the same problem, although I have not determined that with great certainty. There 8do* seem to be performance issues though ... on my up-to-date Chrome-Linux build it takes several *minutes* to render the image.
Comment 7 Nikolas Zimmermann 2010-06-24 02:15:07 PDT
Created attachment 59625 [details]
Reduced testcase

Attaching a reduced testcase. Very simple. Two problems:
- <use filter=".."> does not seem to have any effect in this testcase.
- <rect filter=".."> alone is already slow.
If you remove all filters the original testcase loads fast in under a second here, and with just _one_ filter present, it takes much longer.

Dirk, can you please have a look? This looks important to me.
Note, <text> is no problem at all, and I stripped it completely from the reduced testcase, so we can focus on the real problem. I suspect it has sth. to do with large intermediate buffers.
Comment 8 Dirk Schulze 2010-06-24 03:00:36 PDT
(In reply to comment #7)
> Created an attachment (id=59625) [details]
> Reduced testcase
> 
> Attaching a reduced testcase. Very simple. Two problems:
> - <use filter=".."> does not seem to have any effect in this testcase.
> - <rect filter=".."> alone is already slow.
> If you remove all filters the original testcase loads fast in under a second here, and with just _one_ filter present, it takes much longer.
> 
> Dirk, can you please have a look? This looks important to me.
> Note, <text> is no problem at all, and I stripped it completely from the reduced testcase, so we can focus on the real problem. I suspect it has sth. to do with large intermediate buffers.

Investigated a bit more on it. And found the reasons for the problem:
    <filter id="Shadow" filterUnits="userSpaceOnUse">

No x,y,width,or height are specified. So we take -10%,-10%,120%,120% of the viewport size. This makes the intermediate buffer sizes (2156.400146, 1371.600098). This is not realy a bug in WebKit, beside that the ImageBuffers seems not created fast enough. The author shouldn't use userSpaceOnUse here.
The feature plan for SVG Filter was to calculate the smallest neccessary area, that we will use. But this takes some time. I propose to add a return 0, if the ImageBuffer didn't get created for the moment. It's difficult to write a test case for this problem, because the test should be fast enough for DRT.

Niko, I didn't blame text for the issue and you're right, that <use> wasn't the problem here. But I sended you an exmaple, where <use> gave back the wrong strokeRect of the referenced object. We should talk about it in another bug report.
Comment 9 W. James MacLean 2010-06-24 07:23:03 PDT
I've tried a release build of Chrome-Linux on a modified version of Unix_history-simple.en.svg where I (1) change filterUnits to be "objectBoundingBox", (2) set x and y to be -0.3 and (3) set height & width to be 1.6.

This renders very quickly, so this does seem to explain the performance issue.
Comment 10 Justin Schuh 2010-06-24 07:45:42 PDT
Dropping the security flag because this is confirmed non-exploitable.
Comment 11 David Kilzer (:ddkilzer) 2011-07-27 16:18:24 PDT
This works now with WebKit nightly build (r91865).  Closing.