Yoda Condition

WordPress 3.5 bringt viele Neuerungen mit sich. Unter anderem auch eine Fehlermeldung: Warning. Missing argument 2 for wpdb::prepare(). Nun ist das ganze keine “richtige Fehlermeldung”, sondern eine Information an den Benutzer. Dennoch verwirrt es viele Nutzer da sie es doch für eine Fehlermeldung halten und befürchten das ihre Plugins oder Themes nun nicht mehr funktionieren. Obwohl es nur ein Hinweis ist, kann es dennoch dazu führen das einige Plugins nicht mehr funktionieren. Zum Beispiel in dem Fall, wenn eine Ajax-Routine ihre Ergebnisse mittels echo oder print anstatt als wohlgeformtes JSON zurückgeben.

Der Grund für diese Änderung lag darin, dass man verhindern wollte das prepare() durch falsche Anwendung anfällig für SQL-Injections ist.

$wpdb->prepare( "SELECT * FROM table WHERE id = $id" );

See the problem? That query isn’t secure! You may think you are “preparing” this query, but you’re not — you’re passing $id directly into the query, unprepared.

Weiter heißt es:

Because you can’t prepare a query without more than one argument.

Ich halte die Änderung vom Prinzip her für sinnvoll. Die Umsetzung jedoch für hektischen Aktionismus. Schauen wir uns das ganze doch einmal näher an und fangen mit der zweiten zitierten Aussage an.

Keine Hühner ohne Eier oder keine Eier ohne Hühner?

Man kann eine SQL-Abfrage nicht vorbereiten wenn man prepare() mit nur einen Argument aufruft. Im Umkehrschluß würde dies bedeuten, jede SQL-Abfrage wird vorbereitet wenn man zwei Argumente übergibt. Stimmt das?

$wpdb->prepare( "SELECT * FROM table WHERE id = $id", $id );

Das soll jetzt also besser sein? Die Abfrage wird ebenfalls nicht vorbereitet und ist genauso anfällig für SQL-Injections. Das Blöde an der Geschichte ist, dass der eine oder andere Plugin-Autor sich mit der PHP-Warnung an sein lieblings PHP-Forum wendet anstatt in den Codex zu schauen. Dort wird man ihm sagen – was ja im Grunde genommen auch richtig ist – dass er einen zweiten Parameter übergeben muss, nichts anderes sagt schließlich die PHP-Warnung aus. Das Ergebnis sieht dann so aus wie das Beispiel oben.
Hier wäre doch der Umkehrschluss – keine Vorbereitung der SQL-Abfrage wenn sie keinen Platzhalter enthält – besser gewesen. Diese Prüfung lässt sich relativ einfach mit einem Regulären Ausdruck realisieren und es kann anschließend ein WordPress-Error zurück gegeben oder eine PHP-Warnung ausgegeben werden.
Um das Rad rund zu machen, kann man weiterhin prüfen ob im Query-String ein $-Zeichen gefolgt von einer Reihe alphanumerischen Zeichen, also ein Variablenname, vorkommt. Dies würde jedoch eine weitere Änderung benötigen zu der ich gleich komme.
Das Ergebnis ist nahezu das gleiche. Falsch programmierte Plugins funktionieren nicht mehr (richtig) und die Plugin-Autoren werden dazu gezwungen die richtige Syntax zu verwenden.

Ist die Lücke wirklich geschlossen?

Bisher ist es meiner Ansicht nach eher Geschmackssache. Mir schmeckt es halt nicht wirklich. Ziel der Anforderung ist jedoch erreicht und damit gut.
Was ich weniger toll finde, ist der Umstand das SQL-Injections weiterhin möglich sind und die derzeitige Lösung dies eben nicht verhindert. Das Problem ist der Tabellenname.

In allen Beispielen, auch im Blogpost von Andrew Nacin, sieht man in der SQL-Abfrage ‘table‘ als Angabe für den Tabellennamen. Leider heißt keine einzige Tabelle in der WP-Datenbank ‘table‘. Und obendrein kann der Präfix je nach Installation anders lauten. Wie bildet man den Tabellennamen also richtig? Zum Beispiel mit

$post_table = $wpdb->posts

(siehe Codex)
Und wie bekommt man den Tabellennamen in seine SQL-Abfrage? Wahrscheinlich so:

$prepared_query = $wpdb->prepare( "SELECT * FROM $post_table; WHERE id = %d;", $post_id );

Leider gibt es auch im professionellen Umfeld noch Autoren die solche Sachen machen:

/// OUCH!!!!
$post_table = $_GET['table'];

// build a query with possible sql-injection
$query = "SELECT * FROM $post_table WHERE id = %d;";

Wer garantiert denn das ein gültiger Tabellenname übergeben wird? Man braucht ja nur etwas Code an das Ende des Tabellennamens anhängen und schon sieht die Abfrage z.B. so aus:

SELECT * FROM wp_posts; DROP TABLE wp_posts;-- WHERE id = %d"

Wäre doch schade wenn es jemanden gelingen würde ein DROP TABLE (oder schlimmeres) an den Tabellennamen anzuhängen.

Jetzt könnte man ja geschwind der Meinung sein, dass man den Tabellennamen auch mit dem Platzhalter %s einfügen könnte.

$post_table = $_GET['table'];
$post_id = $_GET['id'];

$prepared_query = $wpdb->prepare( "SELECT * FROM %s WHERE id = %d;", $post_table, $post_id );

Tja, leider nicht. Denn das ergibt dann eine Abfrage wie z.B.:

SELECT * FROM 'wp_posts' WHERE id = 5;

Und diese Abfrage führt zu einen MySQL-Fehler denn der Tabellenname darf natürlich nicht in Anführungszeichen stehen. Der Plugin-Autor muss sich an dieser Stelle selber darum kümmern das der Tabellenname vorhanden ist und keinen Schadcode enthält. Wie viele Plugin-Autoren haben das bisher gemacht? Wohl die wenigsten.

Was fehlt

Es fehlt ein Platzhalter für den Tabellennamen (z.B. %t). prepare() könnte die vorhandenen Tabellennamen abfragen und somit gleichzeitig prüfen ob eine solche Tabelle vorhanden ist. Dies würde dann die Gefahr für SQL-Injections tatsächlich gegen Null tendieren lassen.
Zudem wäre die Prüfung auf einen vorhandenen Platzhalter deutlich sinnvoller. Es ist zwar nicht die bessere, meines Erachtens nach aber die sinnvollere, Lösung.
Als Fazit muss man also festhalten das sich im Grunde genommen nichts gebessert hat. Lediglich etwas Verwirrung ist unter den Benutzern entstanden und der eine oder andere Plugin-Autor wird wohl ein paar E-Mails bekommen.
Ich zweifele ja daran das entsprechende Tickets im Trac sinnvoll sind (Erfahrungswert), werde jedoch mal ein oder zwei schreiben. Falls jemand schneller ist (oder sein möchte ), hinterlasst die Ticket-Nummer bitte in den Kommentaren. Ich werde mich dann den Tickets aktiv anschließen.