[PHP] Secure file upload

Pagina: 1
Acties:

Onderwerpen


Acties:
  • 0 Henk 'm!

Verwijderd

Topicstarter
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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
 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 ]


Acties:
  • 0 Henk 'm!

  • Jantje2000
  • Registratie: Februari 2016
  • Laatst online: 18-09 17:16
Waarom zet je 3 if's na elkaar en doe je niet gewoon:

code:
1
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?

De wet van Murphy: Alles wat fout kan gaan zal fout gaan.


Acties:
  • 0 Henk 'm!

Verwijderd

Topicstarter
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?

Acties:
  • 0 Henk 'm!

  • GlowMouse
  • Registratie: November 2002
  • Niet online
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.

Acties:
  • +1 Henk 'm!

  • orf
  • Registratie: Augustus 2005
  • Laatst online: 00:44

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.

Acties:
  • 0 Henk 'm!

Verwijderd

Topicstarter
@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?

Acties:
  • 0 Henk 'm!

  • orf
  • Registratie: Augustus 2005
  • Laatst online: 00:44

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 ]


Acties:
  • +1 Henk 'm!

  • Demonitzu
  • Registratie: Augustus 2012
  • Niet online

Demonitzu

Incidentele gebruiker

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

TekkenZone - Dutch Tekken Community


Acties:
  • 0 Henk 'm!

Verwijderd

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

Acties:
  • 0 Henk 'm!

  • Foxl
  • Registratie: Juli 2002
  • Niet online
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)

I'm really easy to get along with, once you people learn to worship me...


Acties:
  • 0 Henk 'm!

  • Voutloos
  • Registratie: Januari 2002
  • Niet online
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..

{signature}


Acties:
  • 0 Henk 'm!

  • ikvanwinsum
  • Registratie: Februari 2011
  • Laatst online: 21:03

ikvanwinsum

/dev/null

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.

U zegt: ‘Alles is toegestaan.’ Zeker, maar niet alles is goed. Alles is toegestaan, maar niet alles is opbouwend.


Acties:
  • 0 Henk 'm!

Verwijderd

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

Acties:
  • 0 Henk 'm!

  • Demonitzu
  • Registratie: Augustus 2012
  • Niet online

Demonitzu

Incidentele gebruiker

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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
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 ]

TekkenZone - Dutch Tekken Community


Acties:
  • +2 Henk 'm!

  • ikvanwinsum
  • Registratie: Februari 2011
  • Laatst online: 21:03

ikvanwinsum

/dev/null

Als je het op die manier met een switch-statement doet kan het sowieso korter:
PHP:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
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;
}

U zegt: ‘Alles is toegestaan.’ Zeker, maar niet alles is goed. Alles is toegestaan, maar niet alles is opbouwend.


Acties:
  • +1 Henk 'm!

  • Douweegbertje
  • Registratie: Mei 2008
  • Laatst online: 20-09 20:54

Douweegbertje

Wat kinderachtig.. godverdomme

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:
1
2
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
  • Registratie: Februari 2008
  • Laatst online: 04-07 16:30
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
  • Registratie: Augustus 2011
  • Niet online
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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
$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.");
}

Fixing things to the breaking point...

Pagina: 1