1. PHP / Говнокод #7608

    +169

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    23. 23
    24. 24
    25. 25
    26. 26
    27. 27
    28. 28
    29. 29
    30. 30
    case"register": // если do=register, выводим регистрацию
     if (isset($login) && isset($pass1) && isset($pass2)) {
       if (!empty($login) && !empty($pass1) && !empty($pass2)) {
           $users=get_serial('users');
           $reallogin=$login;
           $login=md5(strtolower($login));
           if (!$users[$login]) {
               if (strlen($pass1)>=4) {
                   $pass1=md5($pass1);
                   $pass2=md5($pass2);
                   if ($pass1==$pass2) {
                       $users[$login]=array();
                       $users[$login]['login']=htmlspecialchars($reallogin);
                       $users[$login]['pass']=$pass1;
                         set_serial($users,'users');
                         $error="Вы успешно зарегистрированны";
                         header("Refresh:3;url=".$_SERVER['PHP_SELF']);
                   }else {
                       $error="Ошибка: Пароли не совпадают";
                   }
               }else {
                   $error="Ошибка: Минимальная длина пароля 4 символа";
               }
           } else {
               $error="Ошибка: Такой пользователь уже существует";
           }
       }else {
         $error="Ошибка: Обязательные поля нужно заполнить";
       }
     }

    Запостил: invision70, 21 Августа 2011

    Комментарии (19) RSS

    • ava Lure Of Chaos 21.08.2011 16:00 # +6

      мило. все перлы начинающего в одном:
      1. один большой index.php на все случаи жизни, с главным "циклом жизни" switch-case
      2. условноисключающая валидация if-else
      3. сначала проверка на isset - и тут же на !empty
      4. непоследовательный API в get_serial('users') и set_serial($users,'users')
      5. куча ненужных md5 без засолки
      6. куча лишних "выделений", как в 5ой или 12ой строке
      7. двойные кавычки *WALL* для простых строк
      8. НАСТОЯЩИЙ логин! хотя, может, и имеет смысл кодировать в базу и логин?
      9. 17ая строка заслуживает особого внимания
      9.1. редирект не через Location, а через Refresh
      9.2. редирект со всеми GETными параметрами - в случае form method="GET" вероятно К.З.
      9.3. и КОНТРОЛЬНЫЙ: PHP_SELF уязвимость....

      я ничего не забыл? )))
      Ответить

      • ava 7ion 22.08.2011 01:30 # +1

        Я вот вижу регистер_глобалс, мой юный падаван.

        >сначала проверка на isset - и тут же на !empty
        Ого, а у нас теперь существование переменной - уже знак того, что она не пуста?

        >КОНТРОЛЬНЫЙ: PHP_SELF уязвимость....
        Что? Гугл про уязвимость в $_SERVER['PHP_SELF'] знает только одну страницу - эту.
        Ответить

        • ava Lure Of Chaos 22.08.2011 11:03 # 0

          это необязательно регистер_глобалс, там еще вначале может быть
          if(isset($_REQUEST['login'])) $login=$_REQUEST['login'];
          и куча подобного (может, $_GET или $_POST)

          конструкция empty также не кидает нотисов, если переменная не определена, поэтому можно приравнять "не существует" и "пуста"
          Ответить

          • ava 7ion 22.08.2011 12:24 # 0

            Но мне хотелось бы услышать про уязвимость в $_SERVER['PHP_SELF'].
            Ответить

            • ava Lure Of Chaos 22.08.2011 18:47 # 0

              если содержится \n то можно будет послать еще пару любых заголовков, в т.ч. перенаправление на любой сайт в контексте этого (с куками, идентификатором сессии и т.д.)
              Ответить

              • ava 7ion 22.08.2011 19:25 # 0

                И как же я могу изменить значение $_SERVER['PHP_SELF']?
                Ответить

      • ava Vasiliy 22.08.2011 08:22 # 0

        strlen($pass1) не правильно посчитает кириллицу в UTF-8 пароль аб пройдет, минимальная длина пароля за хардкожена. И это просто $reallogin=$login; эх .....
        Ответить

      • ava guest 26.08.2011 20:09 # 0

        Подскажите, а чем лучше заменить этот "цикл жизни" switch-case? у меня вот тоже самое во всех индексах сайтов.
        Ответить

    • ava Lure Of Chaos 21.08.2011 16:08 # 0

      ах да, и еще "зарегистрированны"
      Ответить

    • ava This is obvious 21.08.2011 17:17 # +1

      вспомнилось
      http://govnokod.ru/2545
      Ответить

      • ava 7ion 22.08.2011 01:31 # +5

        А мне вспомнилось if ($error == "Все хорошо")
        Ответить

    • ava eth0 22.08.2011 11:05 # +1

      Сравнение хешей двух паролей, конечно, сильно.

      Но главная беда тут в юзабилити. За такие интерфейсы, где одно опущенное поле показывает одну ошибку и сбрасывает форму - надо убивать.
      Проверять нужно все значения в форме, диагностические сообщения должны быть для каждого поля, форма сбрасываться не должна (возможно, за исключением поля "пароль", но это вопрос спорный).
      Ответить

      • ava Lure Of Chaos 22.08.2011 11:23 # +1 

        http://ragecomics.cloudapp.net/Troll/23954/password-captcha-troll
        Ответить

      • ava roman-kashitsyn 22.08.2011 11:27 # +1

        > Сравнение хешей двух паролей
        Сидимо, где-то глубоко в мозгу автора отпечаталось, что работать с паролями напрямую - зло, нужно всегда использовать хэши. На всякий случай.
        Ответить

    • ava rO_ot 23.08.2011 14:23 # 0

      $error="Вы успешно зарегистрированны";

      с намеком:)
      Ответить

      • ava RaZeR 24.08.2011 12:14 # 0

        "Регистрироваться у нас - уже большая ошибка..."
        Ответить

    Добавить комментарий