Wenn man einmal kapiert hat wie es funktioniert versteht man, warum die letzten Bugfixes nur Flickwerk ohne grosse Bedeutung sein können. Nehmen wir einmal folgenden SQL Query, der etwa von der News/index.php ausgeführt wurde (dieser war Angriffsziel der letzten Angriffe):
"update ".$prefix."_stories set score=score+$score, ratings=ratings+1 where sid='$sid'"
Das Problem tritt nun auf, wenn die Variable $score einen etwas modifizierten Inhalt hat. So ging der Autor von PHPNuke aus, dass $score eine Zahl zwischen 1 und 5 ist. Man kann aber -wenn man böswillig ist- über den Browser der Variablen einen anderen Inhalt geben. Ersteinmal der Normalfall ($score=2), folgendes SQL Kommando wird ausgeführt:
"update nuke_stories set score=score+2, ratings=ratings+1 where sid=2"
Ganz normal. Wenn aber der USer über den Browser die berühmte Zeile aufruft:
http://localhost/modules.php?name=News&op=rate_article&score=4,counter=92&sid=1
Erhält die Variable $score einen ganz anderen Wert und zwar $score="4,counter=92". Sinn macht dies erst, wenn man sich die Folgen im Query String genau ansieht:
"update nuke_stories set score=score+2, counter=92, ratings=ratings+1 where sid=2"
Ergebnis: Durch eine einfache Änderung in der Browserzeile wird die Spalte Counter verändert - das war so nicht geplant.
Wie funktiniert der Bugfix
Kurz zum Bugfix: Im vorliegenden Fall ist das Problem klar, sobald die Variable $score einen anderen Wert als eine Zahl hat stimtm irgendwas nicht. Eine einfache Prüfung dessen mit "if (!is_numeric($score))" klärt die Frage und die Sache ist gegessen.
Wo ist das Problem?
Wo ist nun weiter das Problem? Im ersten Absatz habe ich hofentlich klar gemacht, wie gross das Gefährdungspotential ist - bei einem kleinen Fehler und nicht ausreichenden Prüfungen kann man bereits riskieren dass der vorgenommene SQL Query ungewollt erweitert wird. Das Code schreiben wird zr Sissifussarbeit. Eine Lösung muss her - eine allgemeine Lösung.
Eine Lösungsidee
Dsa Problem bei PHPNuke ist vor allem die Menge an (unüberschaubaren) Codezeilen. Wer jetzt hier alle SQl Injection Möglichkeiten ausklammern will wird nächstes Jahr noch daran sitzen. Mir kam folgende Idee: Sämtliche SQL Queries laufen über die sql_layer.php; Man hat also wenigstens schonmal eine zentrale Schnittstelle an der die vorgenommenen SQL Queries abgefangen werden können.
Ich habe viele Möglichkeiten durchgespielt (Checksummenbildung für die Queries, Abfragestufen für die Queries mit UPDATE und Insert) und bin am Ende bei einer äusserst einfachen Lösung gelandet.
So steht bei jedem SQL Query am Anfang fest wie viele Spalten verändert werden, bei obigen Beispiel sind es etwa 2. Erst wenn ein "böser" User im Browserstring rumspielt werden daraus mehr - etwa 3 oder 4 etc. Man muss der sql_layer.php "nur" mitteilen, wie viele Spalten verändert werden müssen und prüfen ob auch wirklich nur diese Zahl geändert wird. Falls nein liegt offensichtlich ein Problem vor und es wird nicht in die DB geschrieben. Das ganz ehabe ich als (primitives SKript) schon verwirklicht mit Erfolg.
Umsetzung
Bevor ich nun das Skript hier vorstelle ein Aufruf: Jeder der ein eigenes VKP herausbringt sollte sich die Idee durch den Kopf gehen lassen und ggfs. umsetzen. Es muss nicht unbedingt über meinen Weg geschehen - aber ich denke das System als solches bietet sehr viel Sicherheit. PHPNuke umzubauen macht wenig Sinn - wenn dann muss FB direkt die Änderungen aufnehmen. Ich habe ihm eine Mail geschrieben mit Code Beispielen, hoffentlich geht er darauf ein.
Noch eine Bitte: Ich habe in den letzten Studnen sehr viel Zeit in das ganze Thema und die Entwicklugn dieser Idee gesetzt. Es wäre schön, wenn diese Idee und das Codebeispiel genutzt werden, wenn man mich "in Memoriam" irgendwo im erstellten Code erwähnt.
Das Skript
Hier ist es nun. Das ganze geht in 3 Schritten vor sich:
1. Auslesen der Kommata-Anzahl
2. Von den Kommata auf die Spaltenzahl schließen
3. Die ausgelesenen mit der angegebenen Spaltenzahl vergleichen
Ich habe mich in diesem Beispiel nur auf das UPDATE konzentriert! Es empfiehlt sich, zuerst zu prüfen ob ein INSERT oder ein UPDATE vorgenommen wird und dann entsprechend die Spalten zu zählen. Ich werde das fertige Skript hierzu noch erstellen - im Moment ging es mir um eine rasche Verbreitung der Idee.
Ansatzpunkt ist zuerst die sql_layer.php, hier wird eine zusätzliche Variable angegeben zur Übergabe der Spaltenzahl. Die Variable erhält einen Standardwert damit auch alte Aufrufe dieser Funktion die diesen Wert nicht übergeben weiter funktionieren. Da ich hier nur von einem UPDATE ausgehe ist es recht leicht: Anhand der Kommate den SQl String in Teile zereilen, die Teile zählen - das sind die Spalten.
function sql_query($query, $id, $spaltenzahl='')
{
global $dbtype;
global $sql_debug;
$sql_debug = 0;
if($sql_debug) echo "SQL query: ".str_replace(",",", ",$query)."";
switch ($dbtype) {
case "MySQL":
{
if($spaltenzahl!='')
{
# 1. Kommata in Querystring zählen
$temp=explode(",",$sqlx);
$sqlzahl=count($temp);
# 2. Aus Kommata auf Spaltenzahl schließen
$sqlspalten=$sqlzahl;
# 3. Wenn Spaltenzahl != $Spaltenzahl sofort aussteigen
if($sqlspalten!=$spaltenzahl)
exit("nice try - guy");
}
$res=@mysql_query($query, $id);
return $res;
}
break;;
Das wars schon - ich hoffe die prinzipielle Idee kam rüber. Ich habe hier nur mySQL angepasst. Der Aufruf der Funktion sähe dann folgendermaßen aus (am Beispiel der News/index.php):
$result = sql_query("update ".$prefix."_stories set score=score+$score, ratings=ratings+1 where sid='$sid'", $dbi, 2);
Fertig. Ich muss nun zum einen die SQl Layer anpassen - je nachdem ob ein INSERT oder ein UPDATE vorgenommen wird - um auch wirklich die genaue Spaltenzahl zu erhalten. Ich hoffe die Idee findet Anklang und vermeht sich rasch. Wenn FB darauf eingeht wird es hofentlich in die nächste PHPNuke Version einfließen, angemailt mit Beispielen und dem Angebot zur Hilfe habe ich Ihn jedenfalls.
