[PHP] Secure file upload

[Verwijderd]

Heren, nadat mijn server gehackt is omdat een grapjas een .jpg uploaden met daarin verborgen een PHP code, ben ik mijn functie aan het herschrijven geweest. Ik denk dat ik al een heel eindje ben, maar wie heeft nog aanvullingen:

PHP:

 public static function upload_document_id($user_id, $type)
    {

        define("UPLOAD_DIR", "resources/identification/");

        $allowed_types      = ['image/jpg', 'image/png', 'image/jpeg', 'image/gif', 'application/pdf', 'application/octet-stream'];
        $allowed_extensions = ['jpg','png','jpeg','gif','pdf'];

        if (isset($_FILES['identity_document']) && $_FILES['identity_document']['size'] < 2000000) {

            $finfo = finfo_open(FILEINFO_MIME_TYPE);
            $imageFileType = finfo_file($finfo, $_FILES['identity_document']['tmp_name']);

            $ext = pathinfo($_FILES['identity_document']['name'], PATHINFO_EXTENSION);

            if ($imageFileType == 'image/jpg' || $imageFileType == 'image/png' || $imageFileType == 'image/jpeg' || $imageFileType == 'image/gif') {
                $check = @getimagesize($_FILES['identity_document']['tmp_name']);
            }
            if ($imageFileType == 'application/pdf' || $imageFileType == 'application/octet-stream') {
                $check = true;
            }

            if ($check !== false && in_array($imageFileType, $allowed_types) && in_array($ext, $allowed_extensions)) {

                $target_name = $user_id.'-'.date("YmdHis").'-'.md5($_FILES['identity_document']['name']).'.'.$ext;
                $target_filename = preg_replace("/[^A-Z0-9._-]/i", "_", $target_name);
                $target_file = UPLOAD_DIR.basename($target_filename);

                //move_uploaded_file($_FILES['identity_document']['tmp_name'], $target_file);

                if (file_exists($target_file)) {
                    Users::update_identification($_POST, $target_name, $user_id);
                }
            }
        }
    }

[ Voor 48% gewijzigd door Verwijderd op 28-12-2016 10:14 ]

[Jantje2000]

Waarom zet je 3 if's na elkaar en doe je niet gewoon:

code:

if ($check !== false && $_FILES['identity_document']['size'] < 2000000 && in_array($imageFileType, $allowed_types))

Dan staan die 3 ifs gelijk in één functie. Verder vind ik het er goed uit zien, maar zeker in pdf's kunnen volgens mij snel vuln codes worden uitgevoerd. Wat zorgt er in deze code nu voor dat dit niet gaat gebeuren? Want zo'n jpg met ingesloten php code kan nu nog steeds worden geupload toch?

[Verwijderd]

Zojuist even aangepast.

Voor de PDF zit ik te inderdaad te denken aan een clamav scanner die de PDF doorspit, of een soort van PDF controleer class?

[GlowMouse]

Je voorkomt hiermee nog steeds niet dat iemand php-code op je server kan uitvoeren omdat je het toestaat dat users .php-bestanden op je server plaatsen. Daarnaast werkt je code niet met open_basedir.

[orf]

Hoe komt het dat je server is gehacked terwijl de PHP code in een afbeelding zat?
Als het goed is gaat een afbeelding (met afbeelding extension) niet door de PHP-interpreter, maar wordt die gewoon geserveerd als afbeelding. De afbeelding is dan wel kapot vanwege de code, maar niet uitvoerbaar. Die code is eventueel wel uitvoerbaar als de afbeelding als include wordt gebruikt. Zo wordt nog wel eens gehackt, maar dat is als het goed is niet mogelijk.

[Verwijderd]

@GlowMouse: vertel mij eens hoe je via dit script een .php code kunt uploaden? Het is juist de bedoeling om .php uit te sluiten.

@orf: Dat is dus de grote vraag, misschien dat NGINX een php code zag in een plaatje en dacht, hee dat is toch een PHP bestand?

[orf]

Als het goed is bepaalt NGINX op basis van de file-extension of de PHP-interpreter er overheen moet.
Het is behoorlijk link om de bezoeker zelf te laten bepalen wat de extensie wordt (omdat je het bepaalt op basis van de bestandsnaam)

Edit:

Superglobals hoef je in je functie niet global te maken. Dat is automatisch zo. (ikzelf vind het wat mooier om de specifieke key van de $_FILES en $_POST mee te geven als functieargument, maar soit.

Er zit een typefout in je application/octet-streamn (op meerdere plekken)

[ Voor 38% gewijzigd door orf op 27-12-2016 20:44 ]

[Demonitzu]

Je filtert niet op de file-extension, dus dan komt een .php bestand met bv. een aangepaste image/jpeg mime type erdoorheen

[Verwijderd]

@toshin: klopt helemaal, zo is het ook gebeurd vrees ik. Ik pas het even aan.

[Foxl]

Als je de bestanden nou eens allemaal met dezelfde extensie (b.v. ".dat") wegschrijft, dan ben je overal vanaf. Echte naam en extensie sla je ergens in de database op. (of je plakt overal een extra extensie achter, als je niet aan databases doet)

[Voutloos]

Doe de eenvoudigere checks eerst en return dan direct (of gooi een Exception). Momenteel roep je getimagesize() etc aan terwijl de extensie eenvoudigweg al ongeldig is of de file misschien gigantisch is..

[ikvanwinsum]

Sowieso wel een goed idee om de bestanden die in de upload dir staan niet uitvoerbaar zijn, op die manier kan er ook geen code uitgevoerd worden. En inderdaad onder een andere bestandsnaam / extensie opslaan, zo voorkom je ook problemen als iema d een bestand probeert te uploaden wat al bestaat.

[Verwijderd]

Bedankt allemaal, heb het script aangepast en hierboven neergezet. Mochten er toch nog verbeterpunten zijn dan hoor ik het graag.

[Demonitzu]

Volgens mij kan je je code nog wel aanscherpen. (disclaimer: ik ben een hobbyist coder)

- Het eerste wat je doet is o.a. de filesize controleren. Wat als iemand een bestand van een paar 100MB weet te selecteren? Honderden MB's worden uitgelezen, terwijl het bestand misschien later alsnog afketst doordat het een niet-toegestane mime type heeft.

- Je handelt eventuele errors niet af

PHP:

public static function upload_document_id($user_id, $type) {

    define("UPLOAD_DIR", "...");
    $allowed types = [];
    $allowed_extensions = [];
    $max_filesize = 2000000;
    
    if (isset($_FILES['identity_document') {

        $ext = pathinfo($_FILES['identity_document']['name'], PATHINFO_EXTENSION);
        
        if (in_array($ext, $allowed_extensions)) {

            if ($_FILES['identity_document']['size'] < $max_filesize) {
            
                $finfo = finfo_open(FILEINFO_MIME_TYPE);
                $imageFileType = finfo_file($finfo, $_FILES['identity_document']['tmp_name']);
            
                switch($imageFileType) {
                    case 'image/jpg':
                    case 'image/png':
                    case 'image/jpeg':
                    case 'image/gif':
                        $check = @getimagesize($_FILES['identity_document']['tmp_name']);
                    break;
                    case 'application/pdf':
                    case 'application/octet-stream':
                        $check = true;
                    break;
                    default;
                        $check = false;
                    break;
                }
                
                if ($check = true) {
                    /* code voor uploaden */
                }
                else {
                    /* error: niet-toegestane mime type */
                }
            
            }
            else {
                /* error: bestand te groot */
            }

        }
        else {
            /* error: niet-toegestane file-extension */
        }

    }
    else {
        /* error: geen bestand geselecteerd */
    }

}

[ Voor 19% gewijzigd door Demonitzu op 28-12-2016 23:20 . Reden: tip van ikvanwinsum ]

[ikvanwinsum]

Als je het op die manier met een switch-statement doet kan het sowieso korter:

PHP:

switch($imageFileType) {
    case 'image/jpg':
    case 'image/png':
    case 'image/jpeg':
    case 'image/gif':
        $check = @getimagesize($_FILES['identity_document']['tmp_name']);
        break;
    case 'application/pdf':
    case 'application/octet-stream':
        $check = true;
        break;
    default;
        $check = false;
        break;
}

[Douweegbertje]

Dat stuk ziet er wel goed uit alleen je moet nog wat dingen uitsluiten. Een belangrijke regel is gewoon om bij elke 'case' te denken "wat als.." en bij elke regel er van het slechte geval vanuit gaan.

Dus je wilt er sowieso voor zorgen dat je in de uploads folder geen script executie kan uitvoeren.

Bijv:

code:

AddHandler cgi-script .php .pl .jsp .asp .sh .cgi
Options -ExecCGI

Je wilt er ook zeker van zijn dat je de file 'moved'. Zet er dus een try/catch bij. Verwijder de originele file bij een fout en geef een foutmelding. Anders kan je alsnog 'malafide' bestanden in je 'tmp upload folder' krijgen.

Je mist nog een check op de lengte van de bestandsnaam. Dat is 255 meestal (afhankelijk van je filesystem).


Als laatste baar ik mij een beetje zorgen dat de volledige $_POST meegeeft aan je update functie. Escape je dat uiteindelijk wel goed?

[johnkeates]

Naast het niet-uitvoerbaar maken van bestanden in een upload of content map zou je ook kunnen zorgen dat uploads nooit rechtstreeks aan de web kant gepresenteerd worden. Dus dat je bijv. /var/www/index.php hebt, maar dat de rest van je scripts in /srv/php/ staan en alleen door index.php ingevoegd worden. En dat je uploads in /srv/web-uploads zet, en dan een streaming optie gebruikt om vanaf het FS een bestand direct aan de gebruiker door te geven in plaats van de map benaderbaar maken met je webserver. Code na het deployen read-only maken en de webserver user geen schrijfrechten voor die bestanden geven is ook geen overbodige luxe.

[RGAT]

Anders gewoon ipv direct naar bestanden te linken in downloads bijv. een download.php?dl=downloadID pagina waar je met headers het bestand met iets als:

PHP:

$attachment_location = "filePath";
if (file_exists($attachment_location)) {

    header($_SERVER["SERVER_PROTOCOL"] . " 200 OK");
    header("Cache-Control: public"); // needed for internet explorer
    header("Content-Type: application/zip");
    header("Content-Transfer-Encoding: Binary");
    header("Content-Length:".filesize($attachment_location));
    header("Content-Disposition: attachment; filename=filePath");
    readfile($attachment_location);
    die();
} else {
    die("Error: File not found.");
}