Veilig uploaden

Overzicht Reageren

Sponsored by: Vacatures door Monsterboard

Pim -

Pim -

21/08/2011 16:42:27
Quote Anchor link
Dag allemaal!
Omdat ik er niet zo heel vertrouwd mee ben, wil ik jullie vragen dit uploadscript te reviewen. Het is een methode van mijn ImageRepository en het neemt Symfony\Component\HttpFoundation\File\UploadedFile als argument, dat Symfony\Component\HttpFoundation\File\File uitbreidt, dat weer SplFileInfo uitbreidt.
UploadedFile#getClientMimeType() geeft, verrassend genoeg het aangeleverde mime type.
UploadedFile#move($directory, $name) verplaatst de upload naar de definitieve locatie en geeft een Symfony\Component\HttpFoundation\File\File terug.

ImageUploadException is een reguliere exception.

$this->directory.$this->subDirectory is de uiteindelijke locatie (subdirs zijn niet toegestaan)
$this->extensions zijn de toegestane extensies

Is dit (voldoende) veilig?

Code (php)
PHP script in nieuw venster Selecteer het PHP script
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
58
59
60
<?php
class ImageRepository
{
    /**
     *
     * @param UploadedFile $image
     * @return Symfony\Component\HttpFoundation\File\File
     */

    public function add(UploadedFile $image)
    {

        // Guess extension based on mime
        $mime = $image->getClientMimeType();
        $extension = substr($mime, strpos($mime, '/')+1);
        
        if(!in_array($extension, $this->extensions))
                throw new ImageUploadException(
                        'Bestandstype niet toegestaan. Upload een afbeelding'.
                        'met met een van de volgende types: '.
                        implode(', ', $this->extensions)
                );

        
        // Make the name safe to use
        $name = $image->getClientOriginalName();
        $name = basename($name);
        // Delete extension
        $name = substr($name, 0, strrpos($name, '.'));
        // Replace weird characters with an underscore
        $name = preg_replace('/[^a-z0-9]+/i', '_', $name);
        $name = trim($name, '_');

        // Suffix name until it doesn't already exist
        while(file_exists($this->directory.$this->subDirectory.$name.'.'.$extension))
                $name .= '_1';

        $path = $this->directory.$this->subDirectory.$name.'.'.$extension;        

        // Delete image in case of an exception
        try {
            $image = $image->move($this->directory.$this->subDirectory, $name.'.'.$extension);

            $size = getimagesize($path);

            if(!isset($size[0]) || !isset($size[1]))
                throw new ImageUploadException(
                        'De geüploade afbeelding is beschadigd'
                );

            if($size[0] != $this->allowedImageSize[0] || $size[1] != $this->allowedImageSize[1])
                throw new ImageUploadException(sprintf(
                        'Upload een afbeelding met de volgende afmetingen: %d pixels breed en %d pixels hoog',
                        $this->allowedImageSize[0], $this->allowedImageSize[1]
                ));


            return $image;
        }
catch(\Exception $e) {
            unlink($path);
            throw $e;
        }
    }
}
Gewijzigd op 21/08/2011 17:10:41 door Pim -
 
PHP hulp

PHP hulp

06/08/2020 00:54:17
 
Pim -

Pim -

26/08/2011 22:44:32
Quote Anchor link
Bump
 
 - Diov  -

- Diov -

27/08/2011 14:14:48
Quote Anchor link
Heb je een voorbeeldje online?
Kunnen we testen :)
 
Pim -

Pim -

27/08/2011 15:10:10
Quote Anchor link
Haha, dat lijkt me niet de ideale manier om de veiligheid te reviewen...
 
Jaron T

Jaron T

27/08/2011 15:52:21
Quote Anchor link
Quote:
// Suffix name until it doesn't already exist
while(file_exists($this->directory.$this->subDirectory.$name.'.'.$extension))
$name .= '_1';


Niet echt een veiligheidsprobleem, maar wel een 'fout' .. je moet _1 dynamisch ophogen
 
Pim -

Pim -

27/08/2011 17:23:33
Quote Anchor link
Ja daar ben ik me van bewust. Ik vind het eigenlijk werk goed zo.

Toevoeging op 27/08/2011 17:23:59:

Ja daar ben ik me van bewust. Ik vind het eigenlijk wel goed zo.
 
The Force

The Force

27/08/2011 17:48:35
Quote Anchor link
Als iemand een PHP bestand upload met een "image/jpeg" mimetype dan komt hij hier doorheen. Mimetype kan volgens mij gewoon door gebruikers verandert worden en is dus niet te vertrouwen. Zie http://stackoverflow.com/questions/2242391/ensure-that-uploaded-file-is-image-in-php voor alternatieven.
 
Pim -

Pim -

27/08/2011 18:01:11
Quote Anchor link
Daarom check ik toch ook de bestandsgrootte. Dat wordt daar aangegeven.
 
The Force

The Force

27/08/2011 18:32:40
Quote Anchor link
Hé ja, in dat geval niks gezegd. Ziet er veilig uit. Het enige wat ik nog kan bedenken is dat mensen heel vaak gaan uploaden, maar dat zal in de meeste situaties geen gevaar vormen.
 
Pim -

Pim -

27/08/2011 20:36:38
Quote Anchor link
Sowieso kan alleen de admin uploaden ;-)
 



Overzicht Reageren

 
 

Om de gebruiksvriendelijkheid van onze website en diensten te optimaliseren maken wij gebruik van cookies. Deze cookies gebruiken wij voor functionaliteiten, analytische gegevens en marketing doeleinden. U vindt meer informatie in onze privacy statement.