Комментарии 24
С одной стороны не хочется брюзжать, поскольку дело, в целом, благое, да и пост не про код. С другой - на что ещё смотреть в хабе про РНР, кроме как на код? А он, к сожалению, довольно грустный. Выглядит так, будто писался глубоко в позапрошлом десятилетии.
Объект-бог, который содержит в себе весь код приложения целиком. Но при этом всё равно передаётся через global.
Защита от SQL инъекций фактически отсутствует, и заменяется забором из костылей. Где-то это проверка на числовое значение, где-то кромсание наивной регуляркой типа
"/[\'\;]/",(заведомо дырявой причём), где-то проверка через функциюvalidUrlParam(?!). Я больше чем уверен, что автор и сам путается, где что применять. Про несчастных пользователей я уж и не говорю. И хотя я не нашёл готовую инъекцию, по поговорке "У семи нянек дитя без глазу", она где-то да вылезет. Просто потому что защита не обеспечивается там где должна - в слое для работы с БД.Защиты от XSS не нашёл вообще, но допускаю что она есть, в каком-нибудь совершенно неожиданном месте.
Валидациия входящих данных в зачаточном состоянии
Зависимость от апача прибита гвоздями.
Вместо исправления динамических пропертей к каждому классу старательно подставлен костыль
Ну и так далее, тут можно долго удивляться.
Я понимаю, что проект разрабатывался давно, и ресурсов на переделку особо нет. Но в таком случае я бы на вашем месте ограничился хабами про вайти образование et al., а код скромно задвинул подальше за шкаф.
Да, реакция мэтров PHP на этот код предсказуема :) Если вкратце - я профессионально PHP не занимался никогда - изначально всё задумывалось на Java (в основном на ней тогда я работал) - но первые же попытки показали что процесс разработки будет (особенно по тогдашним джаванским технологиям) настолько трудоёмким и мучительным, что это дело я забросил. Ну и хостить её родимую и решать кучу инфраструктурных проблем. Так что пошёл "от обратного" и взялся за PHP потому что понял что с ним-то захоститься проблем не будет и написать MVP можно будет гораздо быстрее (2 недели заняло).
На вашем месте я бы постеснялась с таким лицом появляться на людях". А уж тем более учить других.
Ну PHP я никого учить не пытаюсь, не переживайте :) а в остальном - если сие творение кому-то помогло - то и хорошо. А ругательства меня наверное мало уж заденут. Действительно в ближайшее время глубочайший рефакторинг вряд ли случится - дело не столько в ресурсах сколько в целесообразности.
Дырявую регулярку постараемся заменить, спасибо. насчет XSS я наверное не очень понимаю как вы заэксплойтить хотите, если не лень - поясните. Ну и в любом случае спасибо что не поленились хотя бы перечислить пункты вызывающие возмущение - и из этого какую-нибудь пользу да извлечем :)
Я больше чем уверен, что автор и сам путается, где что применять
Ну, больше скажу, я просто уже значительную часть этого добра не помню - когда приходится что-то добавлять или править - вспоминаю - но не всегда то что надо :)
P.S. впрочем насчет XSS сейчас проглядывая примеры как минимум одно место сообразил. наверное дальше сдюжу сам поискать ещё.
В том-то и дело, что вы не понимаете главного. Для вас защита принципиально сводится к латанию дыр. "Ну я сейчас поищу", "ну я заменю регулярку". Для того чтобы отбояриться от зануды на Хабре это может и сойдёт (нет). Но к реальной защите это не имеет никакого отношения.
Защита работает только тогда, когда она применяется системно. Переменные попадают в SQL запрос всегда с защитой. А не в надежде, что где-то другом месте кода кто-то не забудет провалидировать. Данные выводятся в HTML всегда с защитой, а не только там где "я поискал".
Причем такой подход помимо прочего ещё и упрощает код. Из него пропадают растущие там и сям бородавки типа "/[\'\;]/". Валидация - это хорошо и правильно. Но она не должна заменять защиту. Это две совершенно разные области, которые пересекаются только в головах у новичков.
Уф, искал по коду упомянутую регулярку, по-моему нашёл только в Auth.php - это явно фрагмент один из самых ранних, с первых недель сайта - и по-моему там она не особо и нужна т.к. формат юзернейма проверяется раньше. Наслоение в общем, на свежую голову внимательнее посмотрю. Если ещё где-то заметили и не лень будет сказать - скажите пожалуйста :)
Этот validUrlParam появился из-за того что тема sql-injection-а была изначально упущена и хотя спохватился довольно быстро, уже менять сами обращения к базе показалось затруднительно, да и проверку хотелось устроить на раннем этапе. В общем безусловно это костыль от которого в идеале стоит уйти, но действительно не факт что я наберусь решимости методически это зарефакторить :)
Тут проблема на самом деле глубже, чем "менять сами обращения к базе". А в кривой архитектуре. У вас SQL торчит в контроллерах. Только вместо нормальных запросов вы туда высунули огрызки, типа "taskid = $id". Это такой очень наивный детский подход, "я в домике!".
Причём даже его можно было сделать нормально, в виде $ctx->taskDataDao->makeLookup('type', 'taskid', $id);. И хотя это всё равно нарушение границ - в контроллере надо знать название таблицы БД, такой код по крайней мере легко сделать безопасным. Но по-хорошему в контроллере длолжен вызываться метод модели/сервиса, который сам знает детали реализации - в какой таблице и по какому полю искать.
Как пример, один из пользователей сайта, который ещё год назад ничего кроме VBA не знал - не поленился немного изучить PHP и с небольшими подсказками развернул вот такой собственный сайт https://the-athenaion.com/
у этого клиента доступна ссылка https://the-athenaion.com/sqlexec.php
даже хорошо что не работает, но не сильно безопасно такой бэкдор оставлять
Хе-хе :) действительно надо сделать чтобы в админском аккаунте появлялась плашка "Да ты же не удалил sqlexec", спасибо!
даже хорошо что не работает
Возможно, это "уже не работает". Я попробовал сделать `drop table users`, но, возможно, это уже кто-то пробовал до меня.
Фу, зря меня путаете, этот 'бэкдор' только в докере работает при локальном запуске. в инструкции где-то было.
это таки бекдор если пользователь веб-сервера сможет выполнить
exec("echo -n '$input' | base64 --decode | mariadb -B 2>&1", $output, $code);
если криво настроен сервер то можно много чего сделать интересного. так же отдельно про sql-инъекции надо посмотреть.
так же все из репозитория болтается доступным
https://the-athenaion.com/dbinit.sql
https://the-athenaion.com/taskinit.sql
это просто файлы, но такого быть не должно. как правильно - делаете папку public, в нем index и все, остальные файлы будут в директории выше и не будут так болтаться в общей доступности, можно сделать даже на шаред-хостингах
это таки бекдор если пользователь веб-сервера сможет выполнить
ну такое. хакер не был бы хакером если бы ему просто дали консоль с рутовыми правами
это просто файлы, но такого быть не должно.
да, потом может передвинем, но "быть не должно" звучит странновато. как вы правильно заметили "это просто файлы", копии из публичной репы.
Вы правда, всерьёз думаете, что вот эта дырища позволяет выполнять только SQL команды? И предыдущая дискуссия вас ничему не научила?
предыдущая дискуссия вас ничему не научила?
простите за прямоту, ваши экивоки тоже не то чтоб напрягают, но выглядят как позиция человека которому охота немного потрепаться, немного показать свой ум и так далее
я вообще не спорю что вы очень-очень умный, но если вы хотите помогать - можете говорить более прямо и не пытаться ёрничать. не стесняйтесь объяснить если считаете что что-то нужно объяснить. люди делятся на две примерно категории - одни стараются что-нибудь полезное сделать. другие не стараются. если в вас прям возбушует злоба и вы не придумаете ничего лучше как пойти для самоутверждения ломать сайты - ну тут ничего не поделаешь.
Во-первых, с чего вы взяли, что я "хочу помогать"? Вы не в Армию Спасения пришли, чтобы вам тут все кидались помогать. Я комментирую статью на Хабре. И если изначально отношение у меня было нейтрально-благожелательное, то вам удалось его сильно испортить. И ладно если бы это было просто невежество (прикрытое фиговым листком "вообще-то я маститый программист на Яве, а это так - левой пяткой нацарапал для нубов"). Но оно усугубляется вашим пофигизмом в отношении замечаний, которые вам делают. Вы даже не даёте себе труда подумать, перед тем как строчить комментарии. И выставляете себя дураком, выдавая элементарные вещи за какие-то сокровенные знания, которые можно не знать.
И начинаете рассказывать, будто "без выбора дб" SQL дамп не выполнится. Или показываете код, который до боли напоминает SQL инъекцию, и заявляете что с ним всё нормально. Вы реально мне сейчас хотите сказать, что mysql_query("SELECT * FROM table WHERE col='$input'"); - это плохо, а exec("command '$input'"); - это ОК? Или про первое вы тоже так и не поняли? И это я виноват что не объяснил с ложечки, почему это позор и профнепригодность?
да ладно-ладно, Вы во всём правы
просто сверху написали "не хотел брюзжать"... и понеслось: ах позор, ах профнепригодность, ах мерзавец, как смел вообще что-то написать и бла-бла-бла
ну, развлекайтесь на здоровье
P.S. где про маститого программиста вообще нашли? вот реально вы воспринимаете текст не так как написано а через какую-то свою призму. понимаете, мне код с учетом времени и недостатка скилла и т.п. может быть сложновато исправить - но потихоньку я этим занимаюсь. а вы себя не исправите потому что вы уверены что всё делаете правильно :)
Здесь изначально вы себя поставили в позицию мол не основной язык но посмотрите что сделал.
Но только суть в том что в любой системе на любом языке правила безопасности одни и те же, везде надо писать через плейсхолдеры, везде надо валидировать любой входящий запрос
Поэтому и получаете такой фидбек, не за сам код, а за то что пытаетесь убедить что с ним все ок, а если там и дыра, то как нибудь потом поправите
Ну и есть ненулевой шанс что какой нибудь новичок увидит и заведомо научится плохим практикам и где нибудь потом вы сам же наткнетесь на этот код
не-не, не пытаюсь убедить что всё ок :) откуда это взялось? код там безусловно нужно приводить в порядок. речь лишь о том что прямо сейчас я не брошу всё чтобы заниматься этим. потихоньку начиная с наиболее кричащих проблем поправим.
просто есть ещё какое-то количество запросов на функционал от текущих пользователей, есть ещё какие-то идеи. есть другие проекты. с опенсорсным барахлом это всё довольно типично...
у коллеги FanatPHP просто стиль изложения мыслей такой... нужно потратить время чтобы понять что он хотел сказать, не ошибся ли он сам и так далее. немного более сознательные коллеги мне например в личку написали чОтенько - исправить здесь, улучшить там - не проблема, сделано
Просто идеальный комментарий, но тоже придерусь по мелочи :)
У меня пунктик на формулировках. Меня столько раз понимали неправильно, что выработался рефлекс на обнаружение таких мест.
везде надо валидировать любой входящий запрос
Эту фразу можно так понять, что валидация помогает защититься от взлома. Но в большинстве случаев это не так. Валидация - это хорошо и нужно, но к безопасности она почти никогда не имеет отношения (за исключением граничных случаев, таких как пурификация HTML). Я, собственно, это автору и пытался объяснить выше - что он пытается обезопасить SQL с помощью валидации входящих данных. А делать это не стоит, поскольку распыляет защиту и делает её в принципе ненадёжной. Плюс такой подход прибивает разные слои приложения друг к другу гвоздями: слой для работы с БД надеется, что слой обработки входящих данных сделает их безопасными для SQL. Ну это же полная бессмыслица.
Кроме того, обеспечивающая безопасность валидация зачастую в принципе невозможна (или неоправданно ресурсоёмка). Вот как в данном конкретном случае например. Валидировать SQL дамп с точки зрения безопасной передачи в командную строку - я не уверен, что эта задача в принципе выполнима. И в любом случае обойдётся довольно большой кровью. При этом чтобы экранировать тот же дамп для командной строки - вообще думать не надо, escapeshellarg() - и передавай что хочешь, хоть бинарную строку.
Поэтому валидацию я бы тут делал обычную - тип данных, длина. Но при этом независимо от её наличия всегда корректно экранировал данные при передаче в командную строку.
Ваш собственный сайт задач за 30 минут — и совсем бесплатно :)