Sicherheit von GET

[fantomas]

Salut Leute.

Die Vorgeschichte ist zu lang... also kurz:

hab ein Skript das eine URL mit GET-Parametern aufruft. Es wird lediglich ein Status gesendet.

Am Anfang lösche ich alle Globals außer GET.

Ich prüfe die GET-Parameter(3) ob die einen Gültigen Wert enthalten. Sprich alle Werte die ankommen sind von vorherein festgelegt.

"mysql_real_escape_string" setze ich auch ein.

Alle drei IFs verlassen das Skripts mit exit im ELSE-Zweig.

Hab einiges gelesen über die Sicherheits-Einstellungen von PHP. Jedoch bin ich mir nicht sicher, wie ich den Einsatz von GET absichern könnte. Oder ob mein PHP-Code Schwachstellen hat.

[Multi]

Oder ob mein PHP-Code Schwachstellen hat.

Das ist ohne Code nicht zu sagen.
Deine Theorie ist nicht schlecht, wie du es praktisch umgesetzt hast, weisst nur du.

Grundsätzlich gilt: Wenn das Script ausschlieslich nur das machen kann, was es darf, egal welcher Parameter übergeben wird, bist du ziemlich sicher.

[fantomas]

  
error_reporting(0);  
//  
    if ( count($_GET) == 3 ) {  
		if ( isset($_GET["v"],$_GET["s"],$_GET["h"]) ) {  
			//  
			function unr_gl() {  
				foreach (func_get_args() as $name) {  
					foreach ($GLOBALS[$name] as $key=>$value){  
						if (isset($GLOBALS[$key]))  
							unset($GLOBALS[$key]);  
					}  
				}  
			}  
			unr_gl('_POST', '_COOKIE', '_REQUEST', '_SERVER', '_ENV', '_FILES');  
			Function remote_status ($get_v, $get_s, $get_h) {  
				$get_h = strtolower($get_h);  
					if ( $get_v == 'A') {  
						$insert = "update table set ".$get_v." = ".$get_v." + '".$get_s."' where col = '".$get_h."';";  
					} else {  
						$insert = "update table set ".$get_v." = '".$get_s."' where col = '".$get_h."';";  
					}  
				mysql_query($insert);  
			}  
				$link = mysql_connect ("localhost", "user", "passwd");  
				mysql_select_db("database");  
				//  
				$get_v = mysql_real_escape_string(trim($_GET["v"]));  
				$get_s = mysql_real_escape_string(trim($_GET["s"]));  
				$get_h = mysql_real_escape_string(trim($_GET["h"]));  
				$get_v_chk_array = array('A','B','C','D','E','F','G','H');  
				//  
					if ( in_array($get_v, $get_v_chk_array) ) {  
						if ( strlen($get_s) == 1 && preg_match('[w|x|y|z]', $get_s) == 1 ) {  
							remote_status($get_v, $get_s, $get_h);  
							unset($insert,$_GET["v"]);  
						} else exit;  
					} else exit;  
				mysql_close($link);  
		} else exit;  
	} else exit;  

[dedlfix]

Tach!

Es ist im Allgemeinen nicht notwendig, überflüssige Parameter/Variablen zu entfernen. Solange diese nicht genutzt werden kann da stehen, was will. Das Sicherheitsproblem gab es früher mit register_globals in Zusammenarbeit mit einer schlampigen Programmierweise. Wenn Variablen vor dem Erstgebrauch nicht gezielt mit einem definierten Wert belegt werden, konnten sie über register_globals mit einem Wert vorbelegt werden.

[code lang=php]
error_reporting(0);
//
    if ( count($_GET) == 3 ) {

Auch diese Abfrage halte ich aus Sicherheitsgesichtspunkten für überflüssig. Lediglich Bots, die eine andere Parameterzahl verwenden werden damit ausgeschlossen. Aber diese müssten durch eine Inhaltsanalyse der gewünschten Werte bereits wirksam ausgeblendet werden können.

  if ( isset($\_GET["v"],$\_GET["s"],$\_GET["h"]) ) {  
  	function unr\_gl() {  

Funktionsdefinitionen sollten extra stehen und nicht mitten im Programmfluss auftauchen. So ist das unsauberer Stil.

  	Function remote\_status ($get\_v, $get\_s, $get\_h) {  
  		$get\_h = strtolower($get\_h);  
  			if ( $get\_v == 'A') {  
  				$insert = "update table set ".$get\_v." = ".$get\_v." + '".$get\_s."' where col = '".$get\_h."';";  
  			} else {  
  				$insert = "update table set ".$get\_v." = '".$get\_s."' where col = '".$get\_h."';";  
  			}  
  		mysql\_query($insert);  
  	}  

Die Maskierung solte beim Erzeugen des SQL-Statements erfolgen und nicht irgendwo ganz anders im Code. Wenn man nur diese Funktion ansieht, hat die Funktion SQL-Injection-Lücken, besonders im else-Fall. Die Funktion selbst prüft $get_v nicht und maskiert keine Werte, sie ist darauf angewiesen, dass das vor ihrem Aufruf passiert. Wenn man das vergisst, ...

dedlfix.

[fantomas]

@ Blubb

Bist du sicher, dass du diese Funktion so haben möchtest?

Ich dachte, ich kann damit unerwünschtes "Ausspähen" oder eine andere Angriffsart ausschließen.

@ dedlfix

Wenn man das vergisst, ...

Ist hier aber nicht der Fall. Nicht?!

So habe ich mir dies vorgestellt. Dass, alle Unstimmigkeiten erst ausgeschlossen werden und dann sauber an die Funktion übergeben werden.

Ist wahrscheinlich unüblich für ein Programmierstil :) naja. mal sehen ...

[Multi]

Ist wahrscheinlich unüblich für ein Programmierstil :) naja. mal sehen ...

Vorallem problematisch, wenn jemand anderer den Code weiterentwickeln soll. Der muss erstmal suchen, wo die Maskierungen erfolgen.

Und wenn du die Funktion mehrfach verwenden willst, musst duvor jedem Aufruf maskieren, anstatt dass du einfach die Daten in die Funktion wirfst und diese einmal die Maskierungen vornimmt.

Das macht dein Vorgehen nicht nur unüblich sondern vorallem kompliziert, vermehrt die Arbeit, verlängert den Code und steigert die Fehleranfälligkeit.

[fantomas]

ok. Durch das viele Verändern des Skript habe ich den Sinn einer Funktion ausgeblendet.

Da das Skript sehr kurz ist, denke ich dass, die Funktion überflüssig ist. ok.

Wo wäre das Skript aber sicherheitskritisch?

[Multi]

Wo wäre das Skript aber sicherheitskritisch?

Da, wo du die Funktion irgendwann mal von einer anderen Stelle aufrufst und dabei vergisst, dass das Maskieren bereits vor dem Aufruf passieren muss. Oder jemand schleust bösen Code ein udn nutzt diese Funktion für eine Injection, da sie nicht  Maskiert.

Grundsätzlich sollte eine Funktion selbst dafür sorgen dass die übergeben Daten keinen Schaden anrichten können. Sich auf andere zu verlassen (hier die Funktion auf den Aufruf) ist immer ein Glücksspiel, da die Funktion nie wissen kann, ob die Daten selbst nicht böse sind.

[dedlfix]

Tach!

@ dedlfix

Wenn man das vergisst, ...
Ist hier aber nicht der Fall. Nicht?!

Noch nicht.

So habe ich mir dies vorgestellt. Dass, alle Unstimmigkeiten erst ausgeschlossen werden und dann sauber an die Funktion übergeben werden.

Das ist ungünstig. Eine Funktion ruft man auf, um sich wiederkehrende Arbeit zu ersparen. Wenn du nun aber für die Funktion mitdenken musst und ihr erst noch alles mundgerecht machen musst, bevor du sie aufrufen kannst, ist das nicht gerade besonders sinnvoll und effizient.

dedlfix.

[Blubb]

Hallo,

error_reporting(0);
//
    if ( count($_GET) == 3 ) {
if ( isset($_GET["v"],$_GET["s"],$_GET["h"]) ) {
//
function unr_gl() {
foreach (func_get_args() as $name) {
foreach ($GLOBALS[$name] as $key=>$value){
if (isset($GLOBALS[$key]))
unset($GLOBALS[$key]);

Bist du sicher, dass du diese Funktion so haben möchtest?  
  
Für jedes Element aus $GLOBALS[\_POST, \_COOKIE, ...] prüfst du, ob ein dort enthaltenes Element ebenfalls als $GLOBALS[...] existiert und wenn, dann setzt du es zurück/löscht es.  
  
Grüße  
  

[Sven Rautenberg]

Moin!

Wenn du deinen Code hier so freundlich reinstellst, will ich die Gelegenheit mal nutzen, um ein paar Kommentare abzugeben.

error_reporting(0);

Es ist nie eine gute Idee, das Error-Reporting abzuschalten! Es ist hingegen immer eine gute Idee, das eingeschaltete Error-Reporting in ein passendes Logfile zu leiten, und auf produktiven Systemen dem Besucher keinerlei PHP-Fehlermeldung zu zeigen (display_errors = off).

if ( count($_GET) == 3 ) {
if ( isset($_GET["v"],$_GET["s"],$_GET["h"]) ) {

Das ist eine eher unkonventionelle Art, eine Sammlung von Eingabeparametern zu validieren, aber sie funktioniert. Verbesserungswürdig ist die überflüssige zweite IF-Ebene an dieser Stelle. Tatsächlich weitergearbeitet werden soll ja nur, wenn beiden Bedingungen gleichzeitig zutreffen, die bessere Vorgehensweise wäre an dieser Stelle eine logische Verknüpfung mit "&&" und eben nur ein IF.

  	function unr_gl() {  
  		foreach (func_get_args() as $name) {  
  			foreach ($GLOBALS[$name] as $key=>$value){  
  				if (isset($GLOBALS[$key]))  
  					unset($GLOBALS[$key]);  
  			}  
  		}  
  	}  

Erstens: Funktionsdefinitionen gehören irgendwo an zentraler Stelle gesammelt und dann einmalig definiert, nicht mitten in den Code eingestreut. Ansonsten könnte es passieren, dass der Code noch ein zweites Mal durchlaufen wird und er dabei die Funktion ein zweites Mal versucht zu definieren - das wäre dann ein PHP-Fehler, den man auf diese Weise leicht vermeiden könnte.

Mir gefällt an der Funktion nicht, dass sie unendlich viele Parameter erlaubt, ohne einen einzigen davon aufzulisten. Ich hätte die Funktion mit genau einem Parameter definiert, und diesen dann als Array formuliert. Dann wird beim Betrachten der Funktion sofort klar, dass man da ein Array übergeben soll.

Abgesehen davon ist die Funktion aber komplett überflüssig.

  	unr_gl('_POST', '_COOKIE', '_REQUEST', '_SERVER', '_ENV', '_FILES');  

Keine der hier angeführten Variablen kann auch nur den geringsten schädlichen Einfluss ausüben, wenn man nicht auf sie zugreift.

  	Function remote_status ($get_v, $get_s, $get_h) {  
  		$get_h = strtolower($get_h);  
  			if ( $get_v == 'A') {  
  				$insert = "update table set ".$get_v." = ".$get_v." + '".$get_s."' where col = '".$get_h."';";  
  			} else {  
  				$insert = "update table set ".$get_v." = '".$get_s."' where col = '".$get_h."';";  
  			}  
  		mysql_query($insert);  
  	}  

Über die Definition von Funktionen mitten im Code siehe oben.

Dass diese Funktion das Escaping nicht durchführt, ist ein eklatanter Mangel. Dass die Funktion im Zweifel das mysql_query() auf der Mysql-Standarddatenbank ausführt, weil die zu benutzende Mysql-Connection nicht als Parameter übergeben wird, ist ebenfalls eine Unschönheit, die die Fehlersuche im Zweifel extrem verkompliziert.

  		$link = mysql_connect ("localhost", "user", "passwd");  
  		mysql_select_db("database");  
  		//  
  		$get_v = mysql_real_escape_string(trim($_GET["v"]));  
  		$get_s = mysql_real_escape_string(trim($_GET["s"]));  
  		$get_h = mysql_real_escape_string(trim($_GET["h"]));  

Das Escaping gehört in die Funktion!

Zu beachten ist auch, dass mysql_real_escape_string() eine existierende Mysql-Connection benötigt und diese im Bedarfsfall selbst herstellt - mit Standardparametern, wenn vorher kein mysql_connect() ausgeführt wurde.

  		$get_v_chk_array = array('A','B','C','D','E','F','G','H');  
  		//  
  			if ( in_array($get_v, $get_v_chk_array) ) {  
  				if ( strlen($get_s) == 1 && preg_match('[w|x|y|z]', $get_s) == 1 ) {  

Validierung ist ja nicht verkehrt, aber warum für dasselbe zwei unterschiedliche Methoden verwenden?

$get_v darf einen der in $get_v_chk_array enthaltenen Werte annehmen (Großbuchstaben von A bis H).
$get_s darf einen der im preg_match angegebenen Buchstaben enthalten, also w, x, y oder z.

Das ist zweimal dasselbe Schema: Eine Variable darf genau einen Wert aus einer Liste von Werten annehmen. Warum wird das unterschiedlich gecoded?

Warum wird für die zweite Variable nicht komplett ein regulärer Ausdruck verwendet, der die strlen-Prüfung überflüssig macht, indem mit Regex-Ankern (^ und $) gearbeitet wird?

Außerdem gilt für das verschachteln von IFs das weiter oben gesagte: Und-Verknüpfen der beiden Bedingungen reduziert den Code um eine Einrückungsebene!

  					remote_status($get_v, $get_s, $get_h);  
  					unset($insert,$_GET["v"]);  

Das unset() ist überflüssig. Erstens: Es gibt an dieser Stelle keine Variable $insert - die ist schon gelöscht worden, als die Funktion beendet wurde.

Und das Löschen von $_GET['v'] ist ebenso überflüssig wie das Löschen der weiteren superglobalen Variablen weiter oben.

  				} else exit;  
  			} else exit;  
  		mysql_close($link);  

Die Datenbank-Connection wird nicht geschlossen, wenn eines der oben angegebenen IFs fehlschlägt. ;)

Nein, doch, natürlich wird die Connection geschlossen - dann, wenn alle Connections automatisch geschlossen werden, nämlich am Skriptende. Das explizite Schließen der Connection ist überflüssig. Ich würde darauf verzichten.

  } else exit;  

} else exit;

  
Es ist außerdem überlegenswert, sich hinsichtlich der diversen validierenden IF-Abfragen mal zu überlegen, ob "fail fast" nicht die bessere Taktik wäre. Anstatt "if (alles ist gut) { mach; } else {exit}" würde das bedeuten, die Abfrage logisch umzudrehen und im if-Code das Skript zu beenden. Wenn es nicht beendet wurde, ist alles in Ordnung, und man kann weitermachen: "if (!alles ist gut) {exit;} mach;"  
  
Das spart in der Regel lange IF-Konstrukte, an deren Ende noch dieses kleine Exit-Anhängsel dransteht. Der relevante Code, der die Arbeit leistet, ist nicht so tief eingerückt - durchaus ein Vorteil.  
  
Außerdem sollte man "teure" Dinge wie das Herstellen einer DB-Verbindung möglichst spät und nur dann erledigen, wenn dieser Code tatsächlich benötigt wird, weil tatsächlich etwas in die Datenbank hineingeschrieben werden muss - und das nicht kurz vorher aufgrund einer scheiternden Prüfung noch abgeblasen wird.  
  
Also zusammengefasst wäre das:  
  
~~~php
function remote_status ($connection, $get_v, $get_s, $get_h) {  
    $get_h = strtolower($get_h);  
    if ( $get_v == 'A') {  
        $insert = "update table set ".mysql_real_escape_string($get_v, $connection)." = ".mysql_real_escape_string($get_v, $connection)." + '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";  
    } else {  
        $insert = "update table set ".mysql_real_escape_string($get_v, $connection)." = '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";  
    }  
    mysql_query($insert, $connection);  
}  
  
if (! (count($_GET) == 3 && isset($_GET["v"], $_GET["s"], $_GET["h"]))) {  
    exit();  
}  
  
$get_v = trim($_GET["v"]);  
$get_s = trim($_GET["s"]);  
$get_h = trim($_GET["h"]);  
  
if (! in_array($get_v, array('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'))) {  
    exit();  
}  
  
if (! in_array($get_s, array('w', 'x', 'y', 'z'))) {  
    exit();  
}  
  
$link = mysql_connect ("localhost", "user", "passwd");  
mysql_select_db("database", $link);  
  
remote_status($link, $get_v, $get_s, $get_h);  

Das ist aber nur ein Zwischenschritt. Denn wie zu sehen ist, wird der SQL-String nicht mit reinen Daten befüllt. mysql_real_escape_string() ist aber nur dafür da, Daten zu escapen, nicht Tabellenfeldnamen!

Andererseits darf man nicht beliebige Feldnamen verwenden, diese Prüfung gehört aber mit zur Validierung der Eingabeparameter in der Funktion remote_status().

Also:

function remote_status ($connection, $get_v, $get_s, $get_h) {  
    if (! in_array($get_v, array('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'))) {  
        return false;  
    }  
  
    if (! in_array($get_s, array('w', 'x', 'y', 'z'))) {  
        return false;  
    }  
  
    $get_h = strtolower($get_h);  
  
    if ($get_v == 'A') {  
        $insert = "update table set ".$get_v." = ".$get_v." + '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";  
    } else {  
        $insert = "update table set ".$get_v." = '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";  
    }  
  
    return mysql_query($insert, $connection);  
}  
  
if (! (count($_GET) == 3 && isset($_GET["v"], $_GET["s"], $_GET["h"]))) {  
    exit();  
}  
  
$get_v = trim($_GET["v"]);  
$get_s = trim($_GET["s"]);  
$get_h = trim($_GET["h"]);  
  
  
$link = mysql_connect ("localhost", "user", "passwd");  
mysql_select_db("database", $link);  
  
remote_status($link, $get_v, $get_s, $get_h);  

Ich habe das exit() in den beiden IFs durch "return false" ersetzt - das beendet die Funktion an dieser Stelle. Außerdem wird das Ergebnis des Aufrufs von mysql_query() jetzt als Funktionsergebnis zurückgegeben - aufgrund der UPDATE-Statements kann das nur true oder false sein. Am Ende gibt die Funktion also false zurück, wenn irgendwas nicht geklappt hat, oder true, wenn das UPDATE funktioniert hat.

- Sven Rautenberg

[MudGuard]

Hi,

if ( strlen($get_s) == 1 && preg_match('[w|x|y|z]', $get_s) == 1 ) {
$get_s darf einen der im preg_match angegebenen Buchstaben enthalten, also w, x, y oder z.

und vermutlich eher unabsichtlich wird hier auch das Pipe-Zeichen erlaubt - überflüssigerweise gleich dreimal ...
Pipe hat innerhalb der Zeichenklasse keine Sonderbedeutung, nur außerhalb bedeutet es "oder".

cu,
Andreas

[fantomas]

ok. Durch das viele Verändern des Skript habe ich den Sinn einer Funktion ausgeblendet.

Da das Skript sehr kurz ist, denke ich dass, die Funktion überflüssig ist. ok.

Wo wäre das Skript aber sicherheitskritisch?