Veilig uploaden
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
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;
}
}
}
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 -
Bump
Kunnen we testen :)
Haha, dat lijkt me niet de ideale manier om de veiligheid te reviewen...
Quote:
// Suffix name until it doesn't already exist
while(file_exists($this->directory.$this->subDirectory.$name.'.'.$extension))
$name .= '_1';
while(file_exists($this->directory.$this->subDirectory.$name.'.'.$extension))
$name .= '_1';
Niet echt een veiligheidsprobleem, maar wel een 'fout' .. je moet _1 dynamisch ophogen
Toevoeging op 27/08/2011 17:23:59:
Ja daar ben ik me van bewust. Ik vind het eigenlijk wel goed zo.
http://stackoverflow.com/questions/2242391/ensure-that-uploaded-file-is-image-in-php voor alternatieven.
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 Daarom check ik toch ook de bestandsgrootte. Dat wordt daar aangegeven.
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.
Sowieso kan alleen de admin uploaden ;-)