[PHP] Zwiększenie bezpieczeństwa aplikacji podczas dołączania plików

Próbuję zbudować sobie modułową stronę w PHP. Zastanawiam się jednak, czy sposób wczytywania modułów, który zastosowałem jest bezpieczny. To najważnieszy fragment kodu w głównym pliku:

if (isset($_REQUEST['mode']) && !empty($_REQUEST['mode'])) {
	$path=SCRIPT_INCLUDE_PATH.$_REQUEST['mode'].".php";
	if (file_exists($path) && is_file($path)) {
		if (!@include($path)) {
			throw new Exception("Nie udało się dołączyć skryptu „".$path."”");
		}
	} else {
		throw new Exception("Żądany moduł nie istnieje", 400);
	}
} else {
	throw new Exception("Nieprawidłowe zapytanie", 400);
}

Wpisując w przeglądarce adres np. example.net/?mode=download-languages skrypt próbuje wczytać plik ./download-languages.php, który wygląda np. tak:

<?php
$db_result=$db->query("SELECT language, COUNT(*) AS quantity FROM functions GROUP BY language ORDER BY COUNT(*) DESC");

echo "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n";
echo "<languages>\n";
foreach ($db_result as $row) {
	echo "\t<language>\n";
	echo "\t\t<name>".htmlspecialchars($row['language'], ENT_XML1)."</name>\n";
	echo "\t\t<quantity>".htmlspecialchars($row['quantity'], ENT_XML1)."</quantity>\n";
	echo "\t</language>\n";
}
echo "</languages>";
?>

Czy taki sposób wczytywania modułów zapewnia wystarczający poziom bezpieczeństwa? Jeśli nie, to w jaki sposób zmodyfikowalibyście mój skrypt aby był bezpieczniejszy?

Dla większości w miarę doświadczonych programistów jest oczywiste, że takie rozwiązanie załączania plików wykonywalnych jest niebezpieczne i niezalecane.

 

  1. Nie powinno się używać tablicy $_REQUEST, a $_GET (w Twoim przypadku): $_REQUEST a bezpieczeństwo

  2. Wszystkie dane pochodzące od użytkownika (a więc i te przekazywane w $_GET) powinno się sprawdzać pod kątem poprawności (np. używając wyrażenia regularnego na dopuszczalność użytych znaków lub porównywać przekazaną wartość z listą/tablicą dopuszczalnych wartości).

 

Warto, żebyś poczytał więcej o tym, jak pisać bezpieczne skrypty PHP

Tablicy $_REQUEST użyłem, ponieważ skrypt będzie obsługiwał zapytania POST i GET. Jednak zmieniłem to tak, aby korzystać tylko z tablic $_POST i $_GET.

Wprowadziłem też walidację nazwy wczytywanego pliku w sposób podobny do tego: http://phpsec.org/projects/guide/1.html#1.4.2.

 

Aktualnie mój kod wygląda tak:

define("SCRIPT_INCLUDE_PATH", "./include/scripts/");

if ((isset($_GET['mode']) && !empty($_GET['mode'])) != (isset($_POST['mode']) && !empty($_POST['mode']))) { //nierówność będzie spełniona tylko gdy ustawiona jest wartość GET albo POST
   $file=isset($_GET['mode']) ? $_GET['mode'] : $_POST['mode'];
   $file.=".php";
   
   $directory=array_diff(scandir(SCRIPT_INCLUDE_PATH), array("..", "."));
   
   $path=SCRIPT_INCLUDE_PATH.$file;
   
   if (in_array($file, $directory) && is_file($path)) {
      if (!@include($path)) {
      throw new Exception("Nie udało się dołączyć skryptu „".$path."”");
     }
   } else {
  throw new Exception("Żądany moduł nie istnieje", 400);
   }
} else {
   throw new Exception("Nieprawidłowe zapytanie", 400);
}

@Pablo_Wawa: Dziękuję Ci za odpowiedź, chętnie posłucham dalszych uwag.

Widzę, żeśmy się nie zrozumieli. Możesz mi wyjaśnić, dlaczego wartość parametru ‘mode’ będzie przekazywany do Twojego skryptu raz metodą GET, a raz POST? Bo ja zupełnie nie rozumiem takiego podejścia. Rozumiem, że pewne parametry będzie się (zawsze) przekazywało do skryptu metodą POST, a inne GET (np. żeby móc operować na linku do danej strony/zawartości), ale nie rozumiem tego, że wartość danego parametru (tu: ‘mode’) będzie przekazywana raz tak, a raz inaczej!

Może nie wiem wszystkiego, ale ja bym się tu trzymał przekazywania go poprzez GET i odbierał jego wartość w skrypcie PHP z tablicy $_GET (i tylko jej).

To po pierwsze. A po drugie, żeby zabezpieczyć się przed różnymi błędami (w Twoich skryptach czy w konfiguracji danego serwera z PHP), zrobiłbym sobie tablicę z dozwolonymi nazwami ‘mode’, które można załączyć w tym skrypcie, i sprawdzał przekazaną wartość ‘mode’ czy znajduje się w takiej tablicy i jeśli tak, to dopiero ją załączał (ale poprzez include_once a nie samo include - chyba że ma to uzasadnienie). To zabezpieczy Cię przed próbą załączenia innego Twojego skryptu (niż odpowiadający jakiejś funkcjonalności ‘mode’), który może wysypać Twój program i wyświetlić atakującemu cenne informacje pomocne w dalszym ataku na serwis.

$allowedScripts=array('skrypt1', 'skrypt2', ... , 'skryptN');
if (isset($_GET['mode']) && !empty($_GET['mode'])) {
  $file=$_GET['mode'];
  $path=SCRIPT_INCLUDE_PATH.$file.".php";
  if (in_array($file, $allowedScripts) && file_exists($path) && is_file($path)) {
    if (!@include_once($path)) {
      throw new Exception("Nie udało się dołączyć skryptu „".$path."”");
    }
  } else {
    throw new Exception("Żądany moduł nie istnieje", 400);
  }
} else {
  throw new Exception("Nieprawidłowe zapytanie", 400);
}

Chciałem zrobić taki uniwersalny silnik, który będzie można rozbudowywać o różne moduły. W panelu administratora mogłaby być opcja “dodaj moduł”, wysyłałoby się plik z modułem, a ten byłby zapisywany w katalogu z modułami (w tym przypadku ./include/scripts/). (Ale to na kiedy indziej :wink:)

 

Dlatego też parametr mode może być przekazywany dwoma metodami. Mam na przykład skrypt download-languages i w tym przypadku parametr jest przekazywany motodą GET. Ale może być też inny skrypt, np. save-language i w tym przypadku mamy zapytanie POST, w którym jest parametr mode oraz inne parametry z danymi do zapisania. Stąd takie udziwnienie w moim programie.

 

Druga sprawa to sprawdzanie poprawności paramatru mode. Nie chciałem tego robić w sposób taki, jaki mi pokazałeś właśnie ze względu na umożliwienie rozbudowy strony bez ingerencji w jej kod. Ale zauważ, że robię bardzo podbną rzecz, różnica jest taka, że dynamicznie tworzę sobie tablicę z dozwolonymi wartościami parametru mode. Może w ostatnim moim skrypcie nie jest to najlepiej rozwiązane, jednak rozbudowałem trochę ten program i dodałem kilka zabezpieczeń.

define("SCRIPT_INCLUDE_PATH", "./php/scripts/");

if ((isset($_GET['mode']) && !empty($_GET['mode'])) != (isset($_POST['mode']) && !empty($_POST['mode']))) { //nierówność uniemożliwia przesłanie parametru "mode" metodą POST i GET jednocześnie
	if (file_exists(SCRIPT_INCLUDE_PATH) && is_dir(SCRIPT_INCLUDE_PATH)) {
		if (($directory=scandir(SCRIPT_INCLUDE_PATH)) == false) {
			throw new Exception("Wystąpił błąd podczas skanowania katalogu z modułami");
		} else {
			$file=isset($_GET['mode']) ? $_GET['mode'] : $_POST['mode'];
			$file.=".php";

			$directory=array_diff($directory, array("..", "."));

			$path=SCRIPT_INCLUDE_PATH.$file;

			if (in_array($file, $directory) && is_file($path)) {
				if (!@include_once($path)) {
					throw new Exception("Nie udało się dołączyć skryptu „".$path."”");
				}
			} else {
				throw new Exception("Żądany moduł nie istnieje", 400);
			}
		}
	} else {
		throw new Exception("Katalog z modułami nie istnieje lub skrypt nie ma praw dostępu");
	}
} else {
	throw new Exception("Nieprawidłowe zapytanie", 400);
}

No cóż, jeśli masz takie (uzasadnione) potrzeby, żeby tak przekazywać parametry, to pokazany kod jest w zasadzie w porządku.

Tylko “luka” może być w skrypcie dodawania modułu (wgrywania go na serwer) - musisz się porządnie tam zabezpieczyć, żeby nie dało się wgrać jakiegoś “złośliwego” kodu (skryptu), ponieważ po jego załączeniu zostanie on wykonany i… sam rozumiesz. :slight_smile: