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.
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).
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.
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 )
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.
)