Уязвим ли этот участок кода PHP для загрузки произвольного файла?

Question

[sr36]

Добрый день. Сейчас дорабатываю небольшой сайт, изначально код писал не я. Наткнулся на следующий участок кода

session_start();
if (0 < $_FILES['file']['error']) {
    echo 'Error into upload.php file.';
} else {
    $id = $_POST['user_id'];
    $type =  $_POST['type'];
    if (!file_exists('uploads/' . $id)) {
        mkdir('uploads/' . $id, 0777, true);
    }
    $infos = pathinfo($_FILES['file']['name']);
    if ($type == "attachments") {
        $allowed_extensions = array("psd","ai","eps","pptx","rtf","wma","odp","ods","sxw","sxi","sxc","dwg","xps","jpg","jpeg","png","gif","svg","pdf","doc","docx","key","ppt","odt","xls","xlsx","zip","rar","mp3","m4a","ogg","wav","mp4","mov","wmv","avi","mpg","ogv","3gp","3g2","mkv","txt","ico","exe","csv","java","js","xml","unx","ttf","font","css");
        if (in_array($infos['extension'], $allowed_extensions)) {
            $file_name = $_FILES['file']['name'];
            move_uploaded_file($_FILES['file']['tmp_name'], "uploads/" . $id . "/" . $file_name);
        }
    } else {
        if ($infos['extension'] == "jpg" || $infos['extension'] == "png") {
            $url = "uploads/" . $id . "/" . $id;
            move_uploaded_file($_FILES['file']['tmp_name'], $url . "." . $infos['extension']);
            if ($infos['extension'] == "png") {
                imagejpeg(imagecreatefrompng($url . "." . $infos['extension']), $url . ".jpg", 90);
                unlink($url . "." . $infos['extension']);
            }
        }
    }
}
?>

Стало интересно, уязвим ли этот участок кода для загрузки файла, с расширением php. С одной стороны, происходит фильтрация по расширениям только из "белого списка" С другой стороны, проверка in_array($infos['extension'], $allowed_extensions) не внушает доверия. Не хватает опыта разобраться, буду благодарен за помощь

Comments

[FanatPHP]

а что не так с проверкой in_array($infos['extension'], $allowed_extensions) ?

[OxCom]

с виду выглядит нормально, но если каким-то образом в $infos['extension'] будет ноль, то может отработать положительно:

<?php
$allowed = ['jpg', 'png'];
$ext = 0;
var_dump(in_array($ext, $allowed)); // true

Я бы немного добавил логики в код:

$infos['extension'] = \mb_strtolower($infos['extension']);

и потом

in_array($infos['extension'], $allowed_extensions, true)

Так же:
- move_uploaded_file() по хорошему не должен использовать имя файла, которое был задано пользователем, чтобы отмести возможные хаки с именами как "qwe/../../../public/hack.psd", и $id = $_POST['user_id']; провести валидацию
- mkdir('uploads/' . $id, 0777, true); - проставляйте права только тем, кому надо.

[Дмитрий]

А пробовали загрузить файл php?

[Дмитрий]

Ну и как бы вы обрабатываете не все картинки

Answer 1

[Евгений]

Как правило уязвимость больше определяется настройкой веб сервера. Какие файлы и как он может выполнять.

Answer 2

[nokimaro]

Вижу в коде возможность залить файл вне папки uploads/ подменив $_POST['user_id']
В зависимости от настроек сервера, потенциально в связке с остальным кодом можно перезаписать на сервере или добавить свои

• robots.txt
  
• sitemap.xml
  
• любые JS используемые на сайте
  
• любые CSS используемые на сайте
  
• любые изображения используемые на сайте
  

Answer 3

[xmoonlight]

$id = $_POST['user_id']; //тута опа!
$type =  $_POST['type'];
if (!file_exists('uploads/' . $id)) {
        mkdir('uploads/' . $id, 0777, true);
}

Обрабатывайте все входные параметры фильтром через регулярные выражения!

Comments

[xmoonlight]

kruslan, Того!!! :)

[xmoonlight]

kruslan, проблемы будут сразу, если их не применить)

Answer 4

[qdevelopment]

Во-первых, такая информация как user_id, обычно хранится в сессии. В вашем случае получается, что пользователь может ввести user_id другого пользователя (да еще и не фильтруется значение) и загрузить файл в его папку.

Во-вторых, нельзя файл сохранять с тем названием, что указано в запросе от пользователя. Генерируйте уникальную строку и храните с этим названием и с расширением из белого списка.