alnorris / SVG-Sanitizer

Whitelist-based PHP SVG sanitizer.
MIT License
22 stars 5 forks source link

Performance Concerns #1

Closed grok closed 10 years ago

grok commented 10 years ago

Hey there!

Decided to give the library a whirl.

Used: http://commons.wikimedia.org/wiki/File:Ghostscript_Tiger.svg

The script takes FOREVER to sanitize.

In particular -- the logic around "// does element exist in whitelist?" just causes a ton of overhead.

Is there a better way to use your library than the demo listed in the README?

grok commented 10 years ago

Also tried something simpler: http://upload.wikimedia.org/wikipedia/commons/8/85/Cyclohexane_simple.svg

grok commented 10 years ago

Actually what appears to be going on...is an infinite loop.

alnorris commented 10 years ago

Script has problems with namespaces remove xml:space="preserve" from line 2 of Cyclohexane_simple.svg and it should work.

grok commented 10 years ago

Not sure if that's a viable option.

I was hoping to run this against user uploaded files. I don't think I can expect each user to check their namespace.

Any ideas?

grok commented 10 years ago

Seems to be related to:

https://github.com/alister-/SVG-Sanitizer/blob/master/SvgSanitizer.php#L90

It's getting hung up on attributes like version and space. If I remove $x-- / it stops infinite looping.

grok commented 10 years ago

Even though it stops looping...it does in fact remove the attribute. What's the logic behind removing the node and then sending the counter backwards? Wouldn't the node count adjust automatically?

grok commented 10 years ago

My fix for this was to go into the whitelist array and add "space" to the svg list. Seems to be good now.

If there's a security reason for why that was excluded, please let me know! Thanks for the hard work!

grok commented 10 years ago

Also... the dom length doesn't seem to change.

So for example.

If you have 5 attributes -- you remove 1 -- the length is not 4. It's still 5. So what I am seeing , is while $x-- would seem correct...it's causing an infinite loop.

grok commented 10 years ago

Recommend changing from the for loop to a foreach loop.