[PHP] Vulnerabilities in preg_replace_callback?

[Koen]

De volgende PHP code schijnt niet veilig te zijn (hoogstwaarschijnlijk een XSS probleem), maar ik heb zelf geen flauw idee waarom.

Ik gebruikte in een UBB achtige parser altijd:

PHP-code:

$msg = preg_replace_callback("/\[img\](.+?)\[\/img\]/is",SmallIMG,$msg); 


SmallIMG verwijst naar een functie in een ander bestand:

PHP-code:

function SmallIMG($matches) {
    $strlen = strlen($url);
    $url = substr($matches[0], 5, $strlen-6); 

  $setting['maxwidth'] = 450;
    $imagehw = @getimagesize("$url");
    $imagewidth = $imagehw[0];
    $imageheight = $imagehw[1];
  if ($imagewidth > $setting['maxwidth']) {
    $imageprop=($setting[maxwidth]*100)/$imagewidth;
    $imagevsize= ($imageheight*$imageprop)/100 ;
    $imagewidth=$setting['maxwidth']; 
    $imageheight=ceil($imagevsize);
  }
    
    return "<img src=\"$url\" width=\"$imagewidth\" border=\"1\" height=\"$imageheight\">";
} 


Voor URL herkenning gebruik ik een zelfde soort systeem. Ook daarin gebruik ik een preg_replace_callback om de URL in te korten zodat die beter in de layout past.

Iemand heeft 't lang geleden uitgeschakeld in mijn code omdat 't niet veilig zou zijn, maar hij weet ook niet meer waarom. Iemand die me vertellen kan wat hier niet veilig aan is, en of het inderdaad aan de callback ligt? Moet ik '(.+?)\' misschien uitgebreider maken?

Ik strip wel verdachte tekens ed alvorens het voor de img parser gaat:

PHP-code:

    $msg=str_replace("<","<",$msg);
    $msg=str_replace(">",">",$msg);
    $msg=str_replace("<","&lt;",$msg);
    $msg=str_replace(">","&gt;",$msg); 


[Dr HenDre]

ik snap je script niet op sommige plaatsen, maar je filtert volgens mij dus alleen op < en > (en de entities daarvan).
Maar wat dacht je van
<img src="javascript:alert(document.cookie);"> (vbb zet spatie tussen java en script, maar t moet dus aan elkaar )
Als je zoiets invoer zal de functie imagesize ten eerste niet klagen omdat dat onderdrukt wordt, en zal het netjes in de url komen. En voilla, een javascript wat uitgevoerd wordt bij het laden van de pagina

edit:
Oplossing voor dit probleem is om alleen url's toe te staan die beginnen met http:// (of evt ftp:// maar lijkt me overbodig). Dus gewoon

PHP-code:

if (substr($url, 0, 7) == 'http://') 
{ 
//blala
}
else
{
$url = 'http://'.$url;
} 


Beetje nasty code maar het werkt
En daarnaast ga ik ervan uit dat je eerst htmlentities($msg, ENT_QUOTES); doet voordat je uberhaupt gaat reg expen

Iemand heeft 't lang geleden uitgeschakeld in mijn code omdat 't niet veilig zou zijn, maar hij weet ook niet meer waarom.

was dat toevallig jon, die de img praser in t fotoboek had uitgeschakeld

Zo even snel op het oog is je PCRE al niet in orde.

Je pattern is "/[img](.+?)[/img]/is", dit is natuurlijk niet correct, aangezien zowel [, ] als / regexp chars zijn, respectievelijk voor "character class definition" en delimiter. Escapen die hap:

/\[img\](.+?)\[\/img\]/is


Bovendien zoals al eerder gezegd sta je met de bovenstaande regexp toe dat er rare image urls gemaakt worden, die niet eens URL zelfs zijn... file:// ftp:// etc... Nu kan ftp:// volgens mij nog wel werken dus je kan dan een OR opnemen in je restrictie:

/\[img\](http|ftp):\/\/(.+?)[\/img\]/is
Uiteraard is het aan te raden om nog eventueel extensions in acht te nemen en in z'n algemeenheid een wellformed URI.

Dergelijke ubb parsers kun je trouwens beter doen met een stackparser (zelf maken dus). Die is efficienter dan een regexp en biedt meer mogelijkheden in dat je ook eventueel de code kan repareren zodat er propernesting ontstaat.

[Dr HenDre]

Hmm, dat is idd een stuk netter
Maar is een if else zoals in mijn vorige post niet (iets) sneller?

Dr HenDre schreef op 09-11-2005 @ 18:55 :
Hmm, dat is idd een stuk netter
Maar is een if else zoals in mijn vorige post niet (iets) sneller?

Ik denk niet dat er veel verschil in zal zitten, sterker nog ik vermoed dat mijn manier net iets sneller is aangezien er al een payload zit voor de regexp, waarna er bij jou nog via een substr een stukje van de string wordt vergeleken. Substring is zo geimplementeerd dat daar een lus bij aanwezig is, die in dit geval de karakters van 0 t/m 7 een voor een buffered, en deze dan terugstuurt; in jouw geval dus na de regexp nog minstens O(7) dus voor "http://". Als jij ftp:// ook nog zou willen ondersteunen zou je een extra rule erbij toemoeten voegen, en weer een substring doen van dit keer substr(0, 6), dat weer goed is voor minstens O(6). O(7+6) = O(13), om nog maar te zwijgen van meerdere protocollen

[Dr HenDre]

Ninh schreef op 09-11-2005 @ 19:02 :
Ik denk niet dat er veel verschil in zal zitten, sterker nog ik vermoed dat mijn manier net iets sneller is aangezien er al een payload zit voor de regexp, waarna er bij jou nog via een substr een stukje van de string wordt vergeleken. Substring is zo geimplementeerd dat daar een lus bij aanwezig is, die in dit geval de karakters van 0 t/m 7 een voor een buffered, en deze dan terugstuurt; in jouw geval dus na de regexp nog minstens O(7).

hmm, mja true
had er effe niet aan gedacht dat er sowieso al een regexp was