Veilig uploaden

Pim -

21/08/2011 16:42:27

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)

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

08/03/2025 12:20:01

Pim -

26/08/2011 22:44:32

Bump

- Diov -

27/08/2011 14:14:48

Heb je een voorbeeldje online?
Kunnen we testen :)

Pim -

27/08/2011 15:10:10

Haha, dat lijkt me niet de ideale manier om de veiligheid te reviewen...

Jaron T

27/08/2011 15:52:21

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 -

27/08/2011 17:23:33

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

27/08/2011 17:48:35

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.

Forum berichten

Reacties

PHP scripts

PHP tutorials

Actief op PHPhulp

Veilig uploaden

Pim -

PHP hulp

Pim -

- Diov -

Pim -

Jaron T

Pim -

The Force

Pim -

The Force

Pim -

Over PHPhulp

Support

Gesponsord

Extra's

Privacy