PDO/prep statement/whitelisting/set charset, това достатъчно безопасно ли е, за да предотврати инжектирането

Преобразувам от разширение mysql към PDO и след като прочетох всичко, което можах от вас гурута в SO и другаде, имам някои остатъчни съмнения. Измислих следното, за да адресирам sql инжекция за типична заявка. Просто се чудя дали това е достатъчно или може би малко прекалявам с белия списък, преди да репликирам това към цялото си приложение.

Не ми е ясно дали правилно съм направил белия списък, т.е. дали и аз трябва да избягам по някакъв начин.

Освен това не съм сигурен дали трябва да задам емулация на атрибута на false за всяка заявка или само веднъж за скрипта.

$link = new PDO("mysql:host=$hostname;dbname=$database;charset=utf8", $username, $password);

$link->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

            $arr_i=$arr_k='';
            $m_act=$v_act='Y';
            $table = array('prices', 'versions', 'models');
            $allowedTables = array('prices', 'versions', 'models');             
            $field = array('model_id', 'version_id', 'price', 'models.active', 'versions.active');
            $allowedFields = array('model_id', 'version_id', 'price', 'models.active', 'versions.active');
            if(count( array_diff($field, $allowedFields))==0 AND  count( array_diff($table, $allowedTables))==0){
            $sql = "SELECT COUNT(DISTINCT `" . $field[0] . "`) as ctmod FROM `" . $table[0] . "`
            INNER JOIN `" . $table[1] . "` USING (`" . $field[1] . "`)
            INNER JOIN `" . $table[2] . "` USING (`" . $field[0] . "`)
            WHERE `" . $field[2] . "` BETWEEN :arr_i AND :arr_k
            AND " . $field[3] . " = :m_act
            AND " . $field[4] . " = :v_act";
            $stmt = $link->prepare($sql);
            $stmt->bindParam(':arr_i', $arr_i, PDO::PARAM_INT);
            $stmt->bindParam(':arr_k', $arr_k, PDO::PARAM_INT);
            $stmt->bindParam(':m_act', $m_act, PDO::PARAM_STR);
            $stmt->bindParam(':v_act', $v_act, PDO::PARAM_STR);
            for ($i=0; $i < $ctpri; $i++){
            $k=$i+1;
            $arr_i=$arr_pri[$i]+1;
            $arr_k=$arr_pri[$k];
            $stmt->execute();
            while ($r = $stmt->fetch(PDO::FETCH_ASSOC)) {
            $ctmod[] = $r['ctmod'];
            }
            }
            }
            else{die();}

person BernardA    schedule 21.12.2013    source източник
comment
codereview.stackexchange.com   -  person Mihai    schedule 22.12.2013


Отговори (2)


Да, вашият код е напълно защитен от SQL инжектиране. Добра работа.

Въпреки че, както посочва @YourCommonSense, няма причина в примера, който показвате, изобщо да правите имена на таблици и колони в променливи. Би било по-лесно просто да ги напишете буквално в заявката.

Следователно, предполагам, че задавате този въпрос, защото правете понякога избирате имена на таблици и колони чрез логика или променливи на приложението, въпреки че не сте го показали в този конкретен пример.


Единствените съвети, които бих предложил са:

  • Цялото конкатениране на низове, с крайни двойни кавички, използване на . и повторно започване на двойни кавички прави кода да изглежда неподреден и може да бъде объркващо да следите кои двойни кавички сте започнали и спрели. Алтернативен стил на интерполация на PHP низове за променливи е ограждането им във фигурни скоби, като следното:

    $sql = "SELECT COUNT(DISTINCT `{$field[0]}`) as ctmod FROM `{$table[0]}`
        INNER JOIN `{$table[1]}` USING (`{$field[1]}`)
        INNER JOIN `{$table[2]}` USING (`{$field[0]}`)
        WHERE `{$field[2]}` BETWEEN :arr_i AND :arr_k
        AND `{$field[3]}` = :m_act
        AND `{$field[4]}` = :v_act";
    
  • И за още една алтернатива, можете да използвате тук документи, така че изобщо не е нужно да се притеснявате за разделянето на низа. Хубаво е, ако имате буквални двойни кавички във вашия низ, защото не е нужно да ги избягвате:

    $sql = <<<GO
        SELECT COUNT(DISTINCT `{$field[0]}`) as ctmod FROM `{$table[0]}`
        INNER JOIN `{$table[1]}` USING (`{$field[1]}`)
        INNER JOIN `{$table[2]}` USING (`{$field[0]}`)
        WHERE `{$field[2]}` BETWEEN :arr_i AND :arr_k
        AND `{$field[3]}` = :m_act
        AND `{$field[4]}` = :v_act
    GO;
    
  • И накрая, няма нищо общо с SQL инжектирането, но добра практика е да проверите върнатата стойност от prepare() и execute(), защото те връщат false, ако възникне грешка при анализиране или изпълнение.

    if (($stmt = $link->prepare($sql)) === false) {
        trigger_error(PDO::errorInfo()[2], E_USER_ERROR);
    }
    

    (Този пример използва синтаксис на PHP 5.4 за дерефериране на масив, върнат от функция.)

    Или пък можете да конфигурирате PDO да хвърля изключения, така че да не не трябва да проверявам.

    $link->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    
person Bill Karwin    schedule 22.12.2013
comment
Благодаря, сега се чувствам по-добре. Няколко коментара: ще трябва да премахнете задните кавички на клаузите И. По някаква причина, вероятно защото низът има точка (models.active), мисля, че няма да работи с тях. Оценявам, че ми показахте начина за обработка на грешки, имам много да науча по този въпрос. Един от тях е къде ще намеря изключенията/грешките, хвърлени от PDO/PHP. - person BernardA; 22.12.2013
comment
@BernardA, трябва да разграничите имената на таблиците и колоните отделно: `models`.`active`. Въпреки че разделянето им така или иначе не е необходимо, защото те не са MySQL запазени думи, нито съдържат специални знаци или интервал. - person Bill Karwin; 22.12.2013
comment
Благодаря. Това е добре. Когато мисля за това, е доста объркващо да чета заявката по този начин. Вероятно е по-добре да използвате ключове в масивите, с името на таблицата/полето. - person BernardA; 22.12.2013
comment
@YourCommonSense, отговорих на въпроса на OP в първия ред на моя отговор. И като се има предвид колко пъти сте давали тангенциални съвети за добри практики за кодиране, съм изненадан от вашия коментар. - person Bill Karwin; 22.12.2013
comment
@YourCommonSense, друга мисъл: правенето на по-удобно форматиране на безопасен SQL е от полза за сигурността, защото разработчиците искат да останат продуктивни. Ако им покажем как могат да бъдат безопасни и продуктивни, е по-вероятно те да приемат най-добрите практики за кодиране. - person Bill Karwin; 22.12.2013

Подозирам, че наистина прекалявате с белия списък. И не само с бели списъци, но дори и с подготвени отчети. И за да задоволите погрешните си възгледи, вие преустроихте заявката си до точката на напълно неразбираема бъркотия.

Това, което трябва да разберете е, че всяка постоянна стойност е безопасна по проект. Така че няма абсолютно никакъв смисъл да използвате нито бели списъци, нито подготвени изявления за него.

И така, вместо

AND " . $field[3] . " = :m_act

трябва да пишеш просто

AND versions.active = 'Y'

без обвързване или бели списъци.

Всичко, което трябва да защитите, са само динамични стойности. Така че трябва да използвате подготвени изрази само за $arr_i и $arr_k. Всички други части на заявката трябва да бъдат записани директно в заявката, точно както го правехте преди.

person Your Common Sense    schedule 22.12.2013
comment
@YCS, Ако знаех, че всичко е правилно, нямаше да задам въпроса като начало. Оценявам приноса ви, но със сигурност не тона му. Ако възнамерявате да запазите същия тон, ще ви бъда благодарен, ако се въздържите да отговаряте на който и да е от въпросите ми в бъдеще. Фактът, че уча, не означава, че ще приема да бъда тормозен. - person BernardA; 23.12.2013
comment
Нямам представа за какъв тон говориш. Никъде не споменах вашите умения или познания или каквито и да било други лични характеристики. Всичко, което направих, беше да отговоря на въпроса ви, давайки единствения надежден отговор. Така че, моля, обуздайте въображението си. - person Your Common Sense; 23.12.2013
comment
@YCS, точно това е проблемът. Виждал съм вашите намеси. Правите го толкова често, без да ви отхвърлят, че смятате, че е добре. Е, не е. Ако желаете да обсъдим това допълнително, предлагам да ми пишете на моя имейл в профила. Мразя да притеснявам други момчета от SO с това. - person BernardA; 24.12.2013