If you call filter_list_format() with a format that does not exist, and the default format also does not exist, filter_list_format() will call itself recursively until PHP runs out of stack space and crashes. Obviously the database should not be in this state, but Drupal core should not have a code path leading to infinite recursion regardless of the state of the database.

CommentFileSizeAuthor
#1 filter-list-format-recursion-722190-1.patch1.52 KBbjaspan

Comments

bjaspan’s picture

bjaspan commented
Version: 7.x-dev » 6.x-dev
Status: Active » Needs review
StatusFileSize
new filter-list-format-recursion-722190-1.patch1.52 KB

The filter system changed a lot in D7. I found this problem on D6. I'm not sure if it still exists on D7 or not.

Patch attached for D6. Not sure if this is really the right solution.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Looking at the D7 code, it seems to return an empty array if the format requested did not exist, so there should be no infinite recursion from filter_list_format() itself. Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

David_Rothstein commented
Status: Closed (fixed) » Needs work

Hm, I'm pretty sure this replaced an infinite recursion with a security hole - is that really an improvement? :)

Granted, if they find themselves in the situation where the default format doesn't exist, they've managed to do something to seriously break their site anyway, and we don't have to support that. So it's not an actual hole, but I think it is still a definite security weakness to allow this - I'd rather see their site break in an obvious way, rather than in a silent way that can lead it to get hacked.

I think we should either roll this back or possibly replace it with throwing an exception or something, rather than silently returning an empty list of filters for the deleted format.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Ok, rolled back for now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (won't fix)

Since the whole auto-fallback mechanism in D6 is wrong in the first place, I don't see another way than throwing an exception or letting it recurse infinitely. Both equals boom, so any effort here is not really worth the effort, especially since this applies to D6 only.