Достаточно ли безопасен этот скрипт загрузки изображений на сервер?

Question

[IvanIF]

На моём ресурсе пользователи имеют возможность загрузки некоторых изображений (например аватарок).

Чтобы избежать попадание на сервер вредоносного кода, я использую следующую функцию:

if ($_FILES['avatar']['error'] == UPLOAD_ERR_OK && ($_FILES['avatar']['type'] == 'image/png' || $_FILES['avatar']['type'] == 'image/jpeg' || $_FILES['avatar']['type'] == 'image/jpg')){

        //Проверка содержания файла
        $info = getimagesize($_FILES['avatar']['tmp_name']);
        if ($info['mime'] == 'image/png' || $info['mime'] == 'image/jpeg' || $info['mime'] == 'image/jpg'){

          //Проверка на недопустимые расширения
          $blacklist = array('.php','.phtml','.php3','.php4','.html','.htm');
          $coincedents = 0;
          foreach ($blacklist as $item){
            if (preg_match("/$item\$/i", $_FILES['avatar']['name'])){ $coincedents++; }
          }
          if ($coincedents == 0){

            //Проверка пройдена

          }
        }
}

Достаточно ли её для защиты веб-приложения?

Answer 1

[FanatPHP]

черный список всегда небезопасен.
проверять надо только по белому

Плюс желательно переменовывать файл целиком, потому что к примеру веб-сервер апач может принять за расширение файла то, что им не является.

Comments

[N]

+ Отключить выполнение скриптов в той директории куда всё это загружается...и отдавать как статику...
Тогда даже если загрузят PHP, например, то и скачают его как файл...

[IvanIF]

N, в статье на Habr, посвящённой этой теме есть вот такой абзац:

Можно подумать, что теперь мы можем пребывать в уверенности, что будут загружаться только файлы GIF или JPEG. К сожалению, это не так. Файл может быть действительно в формате GIF или JPEG, и в то же время PHP-скриптом. Большинство форматов изображения позволяет внести в изображение текстовые метаданные. Возможно создать совершенно корректное изображение, которое содержит некоторый код PHP в этих метаданных. Когда getimagesize() смотрит на файл, он воспримет это как корректный GIF или JPEG. Когда транслятор PHP смотрит на файл, он видит выполнимый код PHP в некотором двоичном «мусоре», который будет игнорирован. Типовой файл, названный crocus.gif содержится в примере (см. начало статьи). Подобное изображение может быть создано в любом графическом редакторе.

Я правильно понимаю, что отключение выполнения скриптов через .htaccess в директории с загруженными файлами вот таким образом предотвращает эту уязвимость?

php_flag engine 0
RemoveHandler .phtml .php .php2 .php3 .php4 .php5 .php7 .phps .cgi .pl .asp .aspx .shtml .shtm .fcgi .fpl .htm .html
AddType text/plain .phtml .php .php2 .php3 .php4 .php5 .php6 .php7 .phps .cgi .pl .asp .aspx .shtml .shtm .fcgi .fpl .htm .html

[N]

IvanIF, Ну да, смысл тот...
Достаточно именно выключить обработку файлов как исполняемых программ...
А как выключить - гугл поможет...просто для апача я точно не скажу, но engine off точно нужно насколько помню)) Я уже лет 10 на nginx...))

[FanatPHP]

IvanIF, обращения к файлам запускает веб-сервер.
веб-сервер обрабатывает файл в соответствии с его расширением.
если твой веб-сервер не настроен на то чтобы исполнять гифки как РНР файлы, то ничего специально отключать не надо
а вот бегать с .htaccess как с писаной торбой и молиться на него от всякой напасти как раз не стоит
потому что если веб-сервер у тебя nginx то этот файл будет нужен как телега на космической станции.

[N]

IvanIF, P.S.: Я, думаю, будет достаточно php_flag engine 0 + контролируемое расширение файла при сохранении. Т.е. сам назначай имя файла.
С этими RemoveHandler и AddType все расширения учесть вряд ли получится...поэтому смысл не глубокий...

[IvanIF]

N, когда в .htaccess было то, что я написал выше, скрипты не обрабатывались. Когда я изменил его на одну строку php_flag engine 0 появились ошибки (скрипт, на котором я тестирую работоспособность почему-то начал скачиваться + во время перезапуска сервера выходили ошибки: неправильно заданы права файлов и т.д.). Так что я пожалуй остановлюсь на своём варианте. НО вот вопрос, мой вариант блокирует исполнение всех скриптов или только тех, что я указал?

[N]

IvanIF,

скрипт, на котором я тестирую работоспособность почему-то начал скачиваться

Всё правильно...он отдался как обычный файл...

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

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

НО вот вопрос, мой вариант блокирует исполнение всех скриптов или только тех, что я указал

Твои последующие две строчки делают следующее...

Первая строка:

Директива удаляет обработчик ассоциации для файлов с данными расширениями. Это позволяет .htaccess файлам в подкаталогах отменять любые ассоциации, унаследованные от родительских каталогов или файлов конфигурации сервера.

Далее вторая строка просто назначает указанным расширениям отдаваться как text...

Поэтому я и сразу сказал, что смысл не глубокий! Лучше упрись в обработку загружаемых файлов...

[IvanIF]

Упёрся)

$blacklist = array('.phtml','.php','.php2','.php3','.php4','.php5','.php6','.php7','.phps','.cgi','.pl','.asp','.aspx','.shtml','.shtm','.fcgi','.fpl','.htm','.html','.jsp');
$whitelist = array('.png','.jpeg','.jpg');

[N]

IvanIF, blacklist в мусорку...проверяй на белый список...

А точнее можно сделать так:

1. Проверяешь на белый список и определяешь расширение, если попало в белый список;
2. Сам назначаешь сохраняемое имя файла с расширением, которое определил из белого списка;
3. Профит.

P.S.: Между 1 и 2 можно конечно учесть имя файла, которое пытаются загрузить, но тут тоже доп. проверки и обработка потребуется...

[IvanIF]

N, да, похоже, что это самый правильный вариант. Спасибо!

Answer 2

[Антон Шаманов]

самый простой способ это не загружать изображение как есть, а изменить его размер с помощью gd/imagick и сохранить если модификация не вызвала ошибок. ну и выдать файлам права 0644 чтобы их нельзя было выполнить.

Comments

[IvanIF]

А выдача файлам прав 0644 и вот такая запись в .htaccess чем-то отличаются?

php_flag engine 0
RemoveHandler .phtml .php .php2 .php3 .php4 .php5 .php7 .phps .cgi .pl .asp .aspx .shtml .shtm .fcgi .fpl .htm .html
AddType text/plain .phtml .php .php2 .php3 .php4 .php5 .php6 .php7 .phps .cgi .pl .asp .aspx .shtml .shtm .fcgi .fpl .htm .html

[Антон Шаманов]

IvanIF, эм... ты спрашиваешь чем отличается "горячо" от "желтый". твой код маскирует скрипты.

[FanatPHP]

IvanIF, это какие-то бессмысленные шаманские заклинания.
При чем здесь права доступа к файлам?
и при чем здесь отключение выполнения скриптов, если твой скрипт-загрузчик по идее не должен их пропускать? Ты как-то задом наперед решаешь проблему - дать юзерам заливать скрипты, но отключить их выполнение
может проще сразу не давать заливать скрипты в папку?
Ну и про то что .htaccess - не икона и молиться на него не надо, я уже писал

[Антон Шаманов]

FanatPHP,

это какие-то бессмысленные шаманские заклинания.

если ты способен обойти эти заклинания, то было бы интересно послушать как.

может проще сразу не давать заливать скрипты в папку

и где предлагаешь хранить в бд?

[N]

Да, первым делом, конечно, надо самому скрипту контролировать загрузку и имена файлов с расширениями...

Можно и nginx перед апач поставить и сделать отдачу из папки загрузки только как статику, а остальное проксировать на апач (ну кроме статики стандартной)...если уж прям хочется "вторую" дверь с замком...

Но первый пункт обязательный, второй как бонус для отдачи всей статики можно использовать, чтобы апач лишний раз не дёргать...