check_csrf_token improvement

elcreator commented 9 years ago

В коде

function check_csrf_token() { if (empty($_SESSION['csrf_token'])) return false; if ($_POST['csrf_token'] != $_SESSION['csrf_token']) die("csrf protection"); unset($_SESSION['csrf_token']); }

перенести unset($_SESSION['csrf_token']); перед die. Иначе при вызове die() до этой строки не дойдет, и в сессии остается одно и то же значение, увеличивая с каждым запросом мизерную (с учетом капчи), но все же вероятность подбора csrf_token

imaginal commented 9 years ago

Подобрать таким образом CSRF при ограничении сессии как по времени так и на количество попыток очень маловероятно. С другой стороны, жизненная ситуация, когда при открытии нескольких окон и форм одна не пройдет из-за CSRF, но еще останется возможность отправить вторую. Так по крайней мере было задумано.

elcreator commented 9 years ago

Уже пишут, что при попытке проголосовать на продакшне выскочило csrf protection. Никаких дополнительных условий - одно окно, один браузер, все ок. Перекидывает на первый шаг, человек вводит капчу - и так раз семь. Потому что при каком-либо глюке с сессией (а они, как оказалось, на конкретно этом продакшне происходят) - неправильное значение оттуда не удаляется. И если однажды $_POST['csrf_token'] != $_SESSION['csrf_token'], так будет всегда - пока браузер не закроют и не грохнется сессионная кука. Перекидывание на первый степ усугубляет ситуацию - человек вводит капчу, у него та же ошибка, снова вводит, и так много раз, пока не забьёт на это гиблое дело или не додумается перезапустить браузер/почистить куки. Если бы дестрой сессии вызывался всегда при ошибке - такого бы не было. Максимум 1 раз на 1 глюк с сессией.

И этот глюк приоритетнее описанной жизненной ситуации с несколькими окнами, т.к. основной flow - это одно окно, и он при определенных условиях не работает.

imaginal commented 9 years ago

На шаге с капчей сессия очищается https://github.com/imaginal/nabuvote/blob/master/step1.php#L10

а при прохождении капчи инициализируется пустым массивом https://github.com/imaginal/nabuvote/blob/master/system/functions.php#L214

поэтому описываемый сценарий уже не подходит, нужно садиться и трейсить получаемые/отправляемые данные.

ed-org-ua / nabuvote

check_csrf_token improvement #13