Add a page on why parametrized queries are better than escaping

[petdance]

No description provided.

[MrStonedOne]

But, they aren't.

[andy-twosticks]

+1. Comments like the one above demonstrate that this would be useful.

[MrStonedOne]

But, they aren't.

The argument is always that you can't forget to escape with parameterized queries. ie, you can't make mistakes.

That's not even true, all it takes is one slip up and using $name rather than :name and you are vulnerable. Only telling people that parameterized queries is perfect gives them a false blanket of security that will let mistakes like that slip by.

At least with escaping you can dynamically build the query text from conditions without having to either make massive repeating if else chains or maintaining what is basically a parallel array.

I know what I'm talking about when I say that they are not objectively better: https://github.com/tgstation/tgstation13.org/blob/master/tgdb/irvpolltally.php#L60

[andy-twosticks]

I absolutely did not say that parameterised queries are the perfect solution. They aren't. But escaping is a highly flawed solutuion.

(re your example: no, it certainly won't save you if you try and parameterise a table name! Bad idea.

Have a look through this slidedeck: http://www.slideshare.net/billkarwin/sql-injection-myths-and-fallacies

You might also read through the comments on this Bruce Schneier article, where a lot of very knowledable people break down the reasons why escaping is not the answer (at least, not on its own): https://www.schneier.com/blog/archives/2008/10/how_to_write_in.html

TL;DR: there is no magic bullet. We are all vulnerable to a greater or lesser extent.

[petdance]

I should start a page that is just links to other articles and slide decks about SQL injection.

[MrStonedOne]

Linking me to some long ass article or some hard to quickly digest powerpoint is going to do nothing.

Explain what is actually wrong with escaping so I can smack your arguments down myself.

I've been programming in php for 16 years since i was 13, I've used both prepared queries and escaped queries. And right now I code for a open source website (and game server) where the primary user base is 4chan users who love sniffing out vulnerabilities and exploiting them for fun. And its escaped queries all the way down, guess how many times we've been exploited since i took over 2 years ago?

[MrStonedOne]

(re your example: no, it certainly won't save you if you try and parameterise a table name! Bad idea.

My example is the chain of ifs that dynamically build the query text up, not the table name.

edit: here, i'll paste in the relevant part.

if ($user[1]) {

    if (isset($_GET['firstseen']) && $_GET['firstseen']) {
        $firstseen = '\''.esc($_GET['firstseen']).'\'';
        $sqlwherea[] = 'p.firstseen < '.$firstseen;
        $tpl->setvar('FIRSTSEEN', htmlspecialchars($_GET['firstseen']));
    }
    if (isset($_GET['rankfilter'])) {
        switch ($_GET['rankfilter']) {
            case 'player':
                $sqlwherea[] = 'p.lastadminrank IN (\'Player\', \'Coder\')';
                $tpl->setvar('RANK_PLAYERS', TRUE);
            break;
            case 'admin':
                $sqlwherea[] = 'p.lastadminrank NOT IN (\'Player\', \'Coder\')';
                $tpl->setvar('RANK_ADMINS', TRUE);
            break;
        }
    } 
    if (isset($_GET['connectionsstart']) && $_GET['connectionsstart']) {
        $connectionsstart = '\''.esc($_GET['connectionsstart']).'\'';
        $tpl->setvar('CONNECTIONSSTART', htmlspecialchars($_GET['connectionsstart']));
    }
    if (isset($_GET['connectionsend']) && $_GET['connectionsend']) {
        $connectionsend = '\''.esc($_GET['connectionsend']).'\'';
        $tpl->setvar('CONNECTIONSEND', htmlspecialchars($_GET['connectionsend']));
    }
    if (isset($_GET['connectioncount']) && $_GET['connectioncount']) {
        $connectioncount = (int)$_GET['connectioncount']+0;
        if ($connectioncount) {
            $tpl->setvar('CONNECTIONCOUNT', $_GET['connectioncount']);
            $sqlconnectionsubq = '(SELECT count(cc.ckey) FROM '.fmttable('connection_log').' AS cc WHERE cc.ckey = v.ckey';
            if ($connectionsstart && $connectionsend)
                $sqlconnectionsubq .= ' AND datetime BETWEEN '.$connectionsstart.' AND  '.$connectionsend;
            else if ($connectionsstart)
                $sqlconnectionsubq .= ' AND datetime > '.$connectionsstart;
            else if ($connectionsend)
                $sqlconnectionsubq .= ' AND datetime < '.$connectionsend;
            $sqlconnectionsubq .= ')';
            $sqlwherea[] = $sqlconnectionsubq.' >= '.$connectioncount;
        }
    }
    if (count($sqlwherea)) {
        $tpl->setvar('PANELOPEN', 'in');
        $sqlwhere = " AND ".join(" AND ", $sqlwherea);
    }
}
$res = $mysqli->query('SELECT v.id, v.optionid, v.ckey, o.text FROM '.fmttable('poll_vote').' AS v LEFT JOIN '.fmttable('poll_option').' AS o ON (v.optionid = o.id) LEFT JOIN '.fmttable('player').' AS p ON (v.ckey = p.ckey) WHERE v.pollid = '.$id.$sqlwhere.' ORDER BY v.id;');

[petdance]

Linking me to some long ass article or some hard to quickly digest powerpoint is going to do nothing.

No, not for you, for other users of the site. Turns out I already started a ticket for this: #29

[MrStonedOne]

That was directed at andy.

[petdance]

all it takes is one slip up and using $name rather than :name and you are vulnerable.

I hadn't thought of that. That's a good argument for not using interpolation for your SQL strings, too.

[andy-twosticks]

@MrStonedOne

Explain what is actually wrong with escaping so I can smack your arguments down myself.

What a wonderful invitation, let me consider it .... nope.

Son, I'm not here to educate you and I don't enjoy arguing with people. If you do, good luck with that. Do your reading and learn something to your advantage. Or don't. All the same to me.

[MrStonedOne]

The point is you're wrong.

Neither is objectively better

[petdance]

That was directed at andy.

I'm Andy, too.