Add an option to disable recursive imagecache preset path [#1934482]

The bug #1922812: Protection against DOS SA-CORE-2013-002 - Drupal core - Denial of service is a critical vulnerability for DoS attack because imagecache can be recursively generated. For example, with one image files/image.png you can create (it has been publicly disclosed).

files/imagecache/full/image.png
files/imagecache/full/imagecache/full/image.png
files/imagecache/full/imagecache/full/imagecache/full/image.png
...

etc. and 'full' could be replaced by any preset that make a very big combination. If an url can be up to 4000 bytes (Apache limit), a preset is in average 16 bytes, you can put up to 100 presets per url. Suppose that there are only 5 presets on your website, with only one image, an attacker can make up up to 5^100 different urls and ask your server to generate 5^100 (it is a number with 70 digits!) derivated images

To fix this problem prevent 99% from the original bug (the other 1% is "by design"). So let's introduce an option which is enabled by default because I don't think there is any reason for recursive imagecache generation.

Comment	File	Size	Author
#5	imagecache-1934482-no-recursive-5.patch	490 bytes	jcisio
#3	imagecache-1934482-no-recursive-3.patch	489 bytes	jcisio
#1	imagecache-1934482-no-recursive.patch	517 bytes	jcisio

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jcisio CreditAttribution: jcisio commented 5 March 2013 at 21:16

Category:	feature	» task
Priority:	Normal	» Critical
Status:	Active	» Needs review

File	Size
imagecache-1934482-no-recursive.patch	517 bytes

Comment #2

David_Rothstein CreditAttribution: David_Rothstein commented 6 March 2013 at 07:51

I haven't looked at this in detail, but be careful that no one is able to bypass this via a clever URL, e.g. one which uses something like ////imagecache/ or \\\imagecache/.

Comment #3

jcisio CreditAttribution: jcisio commented 6 March 2013 at 08:37

File	Size
imagecache-1934482-no-recursive-3.patch	489 bytes

New patch based on core's one.

Comment #4

David_Rothstein CreditAttribution: David_Rothstein commented 6 March 2013 at 14:26

Probably should use '\/' for the ltrim() (same as the core patch), rather than '/'? I'm pretty sure there is a way to get around this with backslashes.