[php] czy ten skrypt uploadu jest bezpieczny? Opcje

[AndyPSV]

tzn. czy nie pozwoli np. na wrzucenie plikow .php albo cokolwiek innego (o czym moge nie wiedziec)

prosilbym o analize, dziekuje

[PHP] pobierz, plaintext 
<?php
 
/* SECURITY HOLE: USER CAN UPLOAD FILES TO OTHER USER IF HE WANTS TO HACK SOMETHING; practically: low priority, nobody would engage in such action; BUT IT NEEDS TO BE FIXED */
/* DON'T UPLOADS PARTICULAR IMAGES WITH "NAMES" DIFFERENT */
 
include($_SERVER['DOCUMENT_ROOT'].'/libs/db/index.php'); include($_SERVER['DOCUMENT_ROOT'].'/libs/db/db.php'); if(!ctype_digit($_GET['id'])) die('!ctype_digit'); include($_SERVER['DOCUMENT_ROOT'].'/libs/url/index.php'); define('N','/');
$q = q('SELECT t,t2 FROM `'.PRFX.'cfg_en` WHERE id = 1'); if(n_r($q) == 0) die('no cfg'); $cfg = f($q);
$q = q('SELECT * FROM `'.PRFX.'ads` WHERE id = "'.$_GET['id'].'" LIMIT 1'); if(n_r($q) == 0) die(H.'no ad'); $r = f($q); /*include($_SERVER['DOCUMENT_ROOT'].'/libs/auth/index.php'); $u = isUlogd($cfg['t2']); if($u['id'] <> $r['u']) die(H.'u <> usr [you\'re trying to edit somebody\'s ad; no permission]');*/ $r['t_'] = _url($r['t']);
 
 
if(!empty($_FILES)) {
	/****
	 * GALERIE TWORZY mozliwa jest ona do edycji
	 * musi poczekac [3] KOMPRESUJE PLIK, AUTOMATYCZNIE I TWORZY MINIATURE (TZW FACEBOOK SIZE)
	 * 90%; stala wielkosc watermarka (ale to punkt [3] musi byc zrobiony)||||||dodaje watermark na srodku obrazka albo gdzies po boku z ID obrazka (id ogloszenia)
	 * 90% done, but it needs to change the thumbnail (image resized) value [2] sprawdza czy nie wystepuje juz plik o tej samej tzw. checksum; jesli wystepuje to go nie uploaduje
	 * 90% [zrobione] [1] tworzy folder osobny (do jednego folderu z ID wrzuca) dla kazdego dodanego zdjecia; na kazde 5 000 subfolderow jest jeden znowu podkatalog ||||| w nieskonczona petle wrzucic
	 * przy kasowaniu zdjec automatyczne usuwanie ich z bazy i z serwera; jak skasuje wszystkie to tez automatycznie usuwa folder
	 * [czeka na punkt [3]]mozliwosc tworzenia miniatury ze zdjecia (czyli inne)
	 * SIZE_LIMIT for all images and documents (1MB, 20MB) or maybe not do it for the photos (but up to 5mb);
	 */
	$range = 5000;/* UNCHANGEABLE VALUE! */ $sub_dir = ceil($r['id']/$range); $targetPath = $_SERVER['DOCUMENT_ROOT'].N.'i'.N.$sub_dir.N.$r['id'].N;
 
	/** HERE IS SECURITY HOLE: VERIFY DO I CAN UPLOAD ANY SCRIPTS WITH .php or any other extensions not allowed anywhere in script ||||| handled? **/
	$fileTypes  = str_replace('*.','',$_REQUEST['fileext']); $fileTypes  = str_replace(';','|',$fileTypes); $typesArray = split('\|',$fileTypes); $fileParts  = pathinfo(strtolower($_FILES['Filedata']['name']));
 
	#$q = q('SELECT id FROM `'.PRFX.'ads_files` WHERE checksum = "'.md5_file($_FILES['Filedata']['tmp_name']).'" AND id_ = "'.$r['id'].'" LIMIT 1'); if(n_r($q) == 0) {
		q('INSERT INTO `'.PRFX.'ads_files` (id_,descr,th) VALUES ("'.$r['id'].'","","")'); $filename = $r['t_'].',(www.'.$cfg['t'].',ID#'.$r['id'].'),'.l_id().'.'.$fileParts['extension'];
		q('UPDATE `'.PRFX.'ads_files` SET t = "'.$filename.'" WHERE id = "'.l_id().'"');
		/* '.md5_file($_FILES['Filedata']['tmp_name']).' DON'T DO CHECKSUMS, IF USER WOULD FIND A DUPLICATE [doesn't check] HE WOULD DELETE IT; INSTEAD A FILENAME WOULD BE PUT, BECAUSE IT WAS BE USED TO DON'T READ ALL THE TIME FOLDERS (lower server usage) */
		$targetFile = str_replace('//','/',$targetPath).basename($filename);
 
		if($fileParts['extension'] == 'gif' OR $fileParts['extension'] == 'jpeg' OR $fileParts['extension'] == 'png' OR $fileParts['extension'] == 'jpg' OR $fileParts['extension'] == 'jpg' OR $fileParts['extension'] == 'doc' OR $fileParts['extension'] == 'docx' OR $fileParts['extension'] == 'pdf') {
			if(in_array($fileParts['extension'],$typesArray)) {
				mkdir(str_replace('//','/',$targetPath),0755,true); move_uploaded_file($_FILES['Filedata']['tmp_name'],$targetFile);
 
				include($_SERVER['DOCUMENT_ROOT'].'/libs/watermark/index.php'); if($fileParts['extension'] == 'gif' OR $fileParts['extension'] == 'jpeg' OR $fileParts['extension'] == 'png' OR $fileParts['extension'] == 'jpg')
					imagewatermark($targetFile,$_SERVER['DOCUMENT_ROOT'].'/libs'.N.'watermark/watermark.png',65);
 
				switch($_FILES['Filedata']['error']) {
					case 1: die('The file is bigger than this PHP installation allows'); break;
					case 2: die('The file is bigger than this form allows'); break;
					case 3: die('Only part of the file was uploaded'); break;
					case 4: die('No file was uploaded'); break;
					case 6: die('Missing a temporary folder'); break;
					case 7: die('Failed to write file to disk'); break;
					case 8: die('File upload stopped by extension'); break;
					default: die('unknown error '.$_FILES['Filedata']['error']);
				} echo str_replace($_SERVER['DOCUMENT_ROOT'],'',$targetFile);
			} else die(H.'Invalid file type');
		}
	#}
}
 
?>
[PHP] pobierz, plaintext 

[Orzeszekk]

Moge się mylić ale na moje oko ten skrypt nie jest bezpieczny, co autor napisal nawet w komentarzach. Rozpoznaje rozszerzenie na podstawie rozszerzenia pliku, co mozna podrobic.

Jeśli dodasz warunek, że jeżeli nie uda się utworzyć miniatury (jeśli ktoś wyśle np. plik php z rozszerzeniem JPG) to usunie zuploadowany plik to będzie bezpieczniej.

Nie mozna polegac ani na rozszerzeniu pliku ani na typie mime, gdyz jedna i druga wartosc moze wprowadzic skrypt w błąd.
Ponadto, nic nie robisz z orginalnym plikiem (chyba - tak mi sie wydaje. Tworzysz miniature, a w galerii po kliknieciu miniatury normalnie wyswietlasz orginalny obraz?). Bezpieczniej jest wyciąć metadane z pliku, lub odczytać obraz, zmniejszyc go do rozsądnych rozmiarów biblioteka GD lub imagemagick i zapisac go na nowo - to tez wytnie metadane z pliku. Rozszerzenie nowemu plikowi nadajesz wedle rozpoznanego typu obrazka przez biblioteke GD - tego sie juz oszukac nie da bo odczytuje dane z tresci pliku.

Jeżeli ktos wrzuci ci do obrazka w komentarz kod PHP i uda mu sie podmienic .htaccess lub masz zle skonfigurowany serwer to w metadanych mozna przemycic kod PHP. Utworz obrazek gif, wpisz w komentarz kod <?php phpinfo(); ?>, tylko juz nie pamietam czy wystarczy go wrzucic w gif, czy trzeba zmienic mu rozszerzenie na php, w kazdym razie powinno ci wyskoczyc phpinfo.

po za tym pomyśl o jakichś limitach na upload (mozesz to trzymac w bazie danych i ujmowac gdy ktos zuploaduje plik od dziennego limitu dla usera) - zabezpieczy cie to przed zasypaniem serwera smieciami gdy wyjedziesz na tydzien na wakacje.

No i kolejna luka co juz pisze w komentarzach - pozwala na wrzucenie pliku dla innego uzytkownika.

Swoją drogą to chyba gotowiec - w takim razie wez solidny porządny skrypt a nie jakis szajs którego autorowi nawet nie chcialo sie pousuwac poważnych błędów.