antirez weblog

Nel precedente post ho scritto del refactoring effettuato sul codice di oknotizie la scorsa settimana. Ancora il nuovo codice non e' online, ma una delle modifiche piu' interessanti che abbiamo operato (oltre alla separazione della implementazione del DB dal resto del programma, ora mediato da una API) e' la quella del potenziamento di mysql.php, libreria scritta da noi qualche tempo fa e utilizzata sia su Segnalo che Oknotizie.

Di norma PHP forza il programmatore a fare l'escape manuale dei parametri delle query SQL. Cio' e' chiaramente disumano. Ad esempio bisogna scrivere:

$name = mysql_escape_string($name);
$sql = "SELECT * FROM rubrica WHERE name='$name'";
$res = mysql_query($sql) ...

Nel nostro codice mysql_query non viene mai utilizzata direttamente perche' e' mediata da altre funzioni come mysqlQuery o mysqlQueryResult e altre simili che si occupano di gestire la connessione al DB se ancora questa non fosse attiva, il caching, ritornare direttamente la prima row, ed altri dettagli troppo tediosi da gestire a mano.

Sin da quando abbiamo iniziato a scrivere le due applicazioni, una cosa mi era abbastanza chiara, prima o poi un escape ce lo saremmo dimenticati. Tutti i linguaggi che utilizzo quando posso scegliere (evito il PHP come la peste) utilizzano una API abbastanza furba da fare l'escape da soli, la sicurezza non puo' essere affidata alla capacita' di non dimenticare mai un escape.

Bisognava porre rimedio, e a tale scopo ho scritto la seguente funzione, utilizzandola poi nel resto della libreria mysql.php:

function mysqlCreateQuery() {
    return mysqlCreateQueryVector(func_get_args());
}

function mysqlCreateQueryVector($argv)
{
    # Build the query escaping where needed
    $argc = count($argv);
    $q = $query;
    for ($i = 0; $i < $argc; $i++) {
        if ($i&1) {
            if (is_array($argv[$i])) {
                $aux = "";
                foreach($argv[$i] as $x)
                    $aux .= "'".mysql_escape_string($x)."',";
                $q .= trim($aux,",");
            } else {
                $q .= "'".mysql_escape_string($argv[$i])."' ";
            }
        } else {
            $q .= $argv[$i];
        }
    }
    return $q;
}

Le funzioni mysqlCreateQuery e mysqlCreateQueryVector sono praticamente identiche, la differenza e' che l'input viene passato ad una come una serie di argomenti e all'altra come un array contenente piu' elementi.

Cosa fa questa funzione? Costruisce query utilizzando l'escape automatico.

Ad esempio scrivere:

$sql = mysqlCreateQuery("SELECT * FROM rubrica WHERE name=",$name);

e' esattamente equivalente a scrivere:

$name = mysql_escape_string($name);
$sql = "SELECT * FROM rubrica WHERE name='$name'";

La funzione si occupa di fare l'escape del parametro e aggiungere i due apici all'inizio e alla fine.

Ovviamente le query non sono sempre cosi' semplici, cosi' la funzione e' abbastanza furba da trattare il primo argomento come una stringa normale, il secondo come una stringa con cui fare l'escape, il terzo ancora come normale, il quarto escape e cosi' via. Per cui e' possibile scrivere qualcosa come:

$sql = mysqlCreateQuery("SELECT id FROM news WHERE time>",$t,"AND user_id=",$uid);

Tuttavia ci sono casi in cui questa sintassi e' scomoda. Ad esempio durante una insert bisognerebbe scrivere:

$sql = mysqlCreateQuery("INSERT INTO foobar (a,b,c) VALUES (",$a,",",$b,",",$c,")");

E' il tripudio delle virgole e degli apici. Cosi' la funzione e' stata modificata in modo che se l'argomento da espandere e' un array invece di una stringa, viene espanso a escape($a[0]),escape($a[1]),escape($a[2]) e cosi' via. Dunque la precedente query puo' essere scritta semplicemente nella seguente forma:

$aux = Array($a,$b,$c);
$sql = mysqlCreateQuery("INSERT INTO foobar (a,b,c) VALUES (",$aux,")");

In mysql.php non usiamo queste funzioni in maniera diretta. Ce ne sono alcune di piu' alto livello come gia' detto che ci semplificano la vita. Ad esempio mysqlQueryResult crea la query con mysqlCreateQuery, collega il DB se e' sconnesso, esegue la query e ritorna il primo risultato (e lascia il DB connesso e selezionato per le prossime query).

Allo stesso modo mysqlQueryList si occupa di fare il foreach all'interno per ritornare un array di rows, e cosi' via.

Nel layer di livello superiore, dbapi.php, ci sono le funzioni che a loro volta utilizzano queste per dare maggiore comodita' al programmatore. Ad esempio la dbCount permette di scrivere una query come la seguente:

$row = mysqlQueryResult("SELECT COUNT(*) FROM news WHERE user_id=",$uid);
$count = $row[0];

in maniera piu' compatta come:

$count = dbCount("news","user_id=",$uid);

Anche la dbCount utilizza internamente mysqlCreateQuery.

E infatti...

Tanto e' vero che dovevamo preoccuparci che qualche giorno fa un utente ci ha segnalato che oknotizie era vulnerabile a SQL injection. Ci eravamo dimenticati un escape in un file PHP che si occupava di controllare via Ajax se la news era probabilmente duplicata. Era ovvio che sarebbe successo, e infatti e' accaduto :)

Non c'e' stato nessun problema reale e il baco e' stato corretto, ma mi dispiace che non siamo riusciti a mettere online la versione piu' sicura di oknotizie in tempo. Con la nuova interfaccia siamo piu' sicuri, e per tale motivo le semplici funzioni sopra esposte sono a disposizione di tutti sotto la licenza GPL: non ci vuole niente a riscriverle, ma e' un gesto di full disclosure come quello operato dal gentile utente che ci ha informati della vulnerabilita'.