Frage zur PHP-Sicherheit

Jop. Mit dem file_exists unterdrückst du eine Fehlermeldung, aber hast auch eine doppelte Abfrage. Einfacher ist da: @include('datei.php'); da ein @ Fehlermeldungen direkt unterdrückt.

Zu deiner Sicherheit nochmals: Wie bereits erwähnt ist eine Whitelist am sichersten und mehr als den Superlativ von "sicher" geht nicht ;)

Oder befürchtest du noch etwas bei deinem Skript?
 
So ich habs mal abgeändert hier ist das Ergebnis:

PHP:
if(!isset($_GET["page"])) { 

$page = "news"; 

} else { 

if(isset($_GET["page"])) {
$page = preg_replace( "/[^a-z0-9]/im", "", $_GET["page"]);
$page = stripslashes(strip_tags($page));
$page = trim($page);

}
  }

$whitelist = array("news", "archiv", "search", "guestbook", "inhalt"); 

if (!empty($page) && in_array($page, $whitelist)) {

$dir = "inhalt";

@include "./" .$dir. "/" .$page. ".php";

   }else{

@include "./inhalt/fehler.php";

}
 }

Nun ich denke damit wäre der Code optimiert und dürfte noch genauso sicher sein oder ? :confused: Bin da immer bissi unsicher :)
 
Hi, ich empfehle dir definitiv nicht die Variante mit dem @include zu nehmen, da sie

1. ebenfalls langsam ist (ja tatsächlich, es gibt zwar kein "if(file_exists(...))" mehr, dafür wird aber vom @ Operator das error_reporting level auf 0 gestzt und nach dem Befehl wieder hergestellt, es entsteht also im kompilierten Code auch wieder mehr Aufwand)

2. es unmöglich macht Fehler in der per include eingebundenen Datei zu erkennen, da alle Fehlermeldungen ja durch das @ unterdrückt werden

Meine Empfehlung: Definitiv die file_exists() Variante nehmen, denn hier zu versuchen auf Geschwindigkeit zu optimieren bringt keinen nennenswerten Gewinn und erschwert darüber hinaus die Fehlersuche, wenn mal was nicht klappt.

P.S. ich habe @include schon oft verflucht beim Fehlersuchen. ;)

P.P.S Zur Optimierung in deinem Fall: Da dein preg_replace() schon alle Zeichen ausser a-z, A-Z und 0-9 entfernt, sind die nachfolgenden Operationen (strip_tags, stripslashes und trim) alle unnötig, da sie immer ins Leere greifen. :)
 
Zuletzt bearbeitet:
Noch eine 2te Frage wie bekomm ich es hin das ich 3 verschiedene Verzeichnisse im include verwenden kann ?

PHP:
if(!isset($_GET["page"])) {  

$page = "news";  

} else {  

if(isset($_GET["page"])) { 
$page = preg_replace( "/[^a-z0-9]/im", "", $_GET["page"]); 

} 
  } 

$whitelist = array("news", "archiv", "search", "guestbook", "inhalt");  

if (!empty($page) && in_array($page, $whitelist)) { 

$dir = "inhalt"; 

if(file_exists("./" .$dir. "/" .$page. ".php")) { include "./" .$dir. "/" .$page. ".php"; }

   }else{ 

if(file_exists("./inhalt/fehler.php")) { include "./inhalt/fehler.php"; }
} 
 }
 
Zuletzt bearbeitet von einem Moderator:
Hi,

war eine interessante Disskusion, die ich bei euch mitlesen konnte. Man denkt einfach mal wieder über das eine oder andere nach, wenn manche über Optimierungen sprechen.
Einen hätte ich auch dazu, obwohl das hier wohl eher in den Bereich der Theorie geht, da der zu erzielende Gewinn nicht mehr wirklich spürbar ist. PHP hat einen grösseren Aufwand beim konkatenieren von Strings, wenn diese in doppelten Anführungsstrichen stehen als wenn sie in einfachen enthalten sind.

Also bringt
PHP:
if(file_exists('./' .$dir. '/' .$page. '.php')) { include './' .$dir. '/' .$page. '.php'; }

   }else{ 

if(file_exists('./inhalt/fehler.php')) { include './inhalt/fehler.php'; }
}
noch einen kleinen Vorteil.

Nun aber zur Frage, wie man mit den verschiedenen Verzeichnissen, in denen sich die Dateien befinden können, umgehen kann.
Hier gibt es sicher auch wieder verschiedenen Möglichkeiten. Eine davon wäre natürlich mit der Switch-Anweisung zu arbeiten und dort in jedem Zweig den passenden Pfad einzusetzen. Bei den vorangegangen Überlegungen zur Optimierung ist es aber wohl eine gute Idee gewesen, sie wegzulassen und die Array-Idee beizubehalten. Eine Switch-Anweisung ist ja für PHP auch eine Belastung, die mit der in_array() - Funktion nicht vergleichbar ist. Sie könnte sich allerdings wieder lohnen, wenn in einigen Fällen noch mehr Verarbeitung nötig ist, die z.B. in der inkludierten Datei nicht stattfinden kann, vielleicht um weitere Dateien zu inkludieren oder so.
Die Idee, die ich dir für den bisher fertigen Code empfehlen würde, wäre das Verzeichnis ebenfalls in deinem Array aufzunehmen und dann wieder daraus zu entnehmen. Dabei können auch Unterverzeichnisse beachtet werden. Du müsstest genau drei Änderungen vornehmen: (1) Das Array in ein mehrdimensionales Array ändern, (2) die Prüfung, ob ein Element im Array enthalten ist anpassen und (3) die Information nutzen.

PHP:
$whitelist = array("news"=>"dir1", "archiv"=>"dir2/subdir", "search"=>"dir1", "guestbook"=>"dir3", "inhalt"=>"dir2");  

if (!empty($page) && isset(array($whitelist[$page]))) { 

$dir = $whitelist[$page]; 

if(file_exists("./" .$dir. "/" .$page. ".php")) { include "./" .$dir. "/" .$page. ".php"; }

   }else{ 

if(file_exists("./inhalt/fehler.php")) { include "./inhalt/fehler.php"; }
} 
 }

Das optimiere ich nochmal ein wenig:
PHP:
$whitelist = array("news"=>"dir1", "archiv"=>"dir2/subdir", "search"=>"dir1", "guestbook"=>"dir3", "inhalt"=>"dir2");  

if (!empty($page) && isset(array($whitelist[$page]))) { 

$file = '/'.$whitelist[$page].'/'.$page.'.php';

if(file_exists($file)) { include $file; }

   }else{ 

if(file_exists("./inhalt/fehler.php")) { include "./inhalt/fehler.php"; }
} 
 }

Der Vorteil dieser Änderung ist nun noch, dass die Variable $dir entfällt. Somit sparen wir uns und vor allem dem Interpreter die Zuordnung des Verzeichnisses zur Variablen, da wir den Wert schon haben. Allerdings ist der Zugriff auf ein Array-Element wieder aufwendiger als auf einen einzelnen String-Wert. Das ist aber hier in Ordnung, da wir über die neue Variable $file das Problem sofort wieder beseitigen und gleich nochmal optimieren. Durch die Zuweisung an die Variable $file kann ab sofort $file verwendet werden. So findet der Zugriff auf das Array-Element in diesem Fall nur einmal statt und nicht, wie in meinem ersten Beispiel zweimal. Und da wir schon einmal dabei sind, stellen wir auch gleich fest, dass der Pfad zur zu inkludierenden Datei nur einmal zusammengestellt werden muss und dann aber zweimal verwendet werden kann. Auch das Zusammenstellen eines Strings bedeutet einen Aufwand, den wir so reduzieren.

Nun vielleicht noch meine Meinung zur Thematik Sicherheit der Parameter. Ich denke ebenfalls, dass über den inzwischen zusammengetragenen Code eine ziemliche Sicherheit entstanden ist. Hier kann sicher nicht mehr viel schiefgehen. Die Frage ist eher, ob die Parameter-Übergabe für dich noch ein Sicherheitsproblem darstellt. Ein Benutzer deiner Seite könnte nachdem er die URL in der Adresszeile gesehen hat, den Parameter manuell bearbeiten und damit eine Datei anfordern, z.B. impressum, die du eventuell an dieser Stelle gar nicht verlinkt hast und vielleicht zur Zeit nicht anbieten willst. Natürlich muss er entweder dafür wissen, welche Seiten du noch hast oder sie erraten. Das kann manchmal recht einfach sein, aber meistens ist es doch aufwendiger für den Benutzer, da er viel herumprobieren müsste. Das könntest du natürlich erschweren, in dem du nicht die Namen deiner Dateien als Parameter nimmst, sondern diese etwas verschlüsselst.
Da könntest du zum Beispiel page=hdnehsfhooz statt page=news verwenden. Diese Buchstabenkombination ist zufällig gewählt, aber erraten werden können diese nur sehr schwer. Ärgerlich ist dann allerdings, dass du selbst eigentlich nicht mehr weist, was in der Datei enthalten ist, wenn sie nicht mehr news.php sondern hdnehsfhooz.php heist.
Dann wäre die andere Idee, dass du die Parameter mit base64 kodierst. Ein häufiges Mittel übrigens. Das kannst du sogar auch mit den Namen der Parameter tun. Die PHP-Funktionen base64_encode() und base64_decode() helfen dabei.
Es gibt noch eine Variante, die ich sehr witzig finde und auf die man mittels raten auch nicht so leicht kommt:
PHP:
$mapping_keys = array("katze"=>"news", "haus"=>"archiv", "eierkuchen"=>"search", "motorblock"=>"guestbook", "news"=>"inhalt"); 

if(isset($_GET["page"])) { 
$page = $mapping_keys[$_GET["page"]]
...
Hier werden als Parameter Werte übertragen, die offenbar nicht sinnvoll sind und aus denen man keinen Rückschluss auf vorhandene Dateien ziehen kann. Diese Begriffe werden anschliessend in die passenden echten Werte mittels des Hilfsarrays mapping_keys gemappt.
Ideallerweise kann man auch mehrere Vorgehensweisen kombinieren.

Die Frage dabei ist natürlich, wieviel Sicherheit du tatsächlich benötigst und ob du dir damit tatsächlich weiterhilfst oder dich doch eher nur selbst beschäftigst, da die Manipulation der Parameter für dich nicht so entscheidend ist. Du bietest ja eh' schon nur Seiten an, die in deiner Whitelist enthalten sind. Bei vorübergehend nicht angebotenen Seiten müsstest du eben deine Whitelist entsprechend anpassen oder wenn diese Seiten "geheim" sein sollen, dann lohnt sich eine Berechtigungsprüfung für den Aufruf der Seite durch den Benutzer durch eine zuvor erfolgte Authentifizierung, die in der inkludierten Datei verwendet wird. Aber prinzipiell können solche Mechanismen natürlich für den Schutz deiner Parameter eingesetzt werden, auch wenn es sich um andere Zwecke als die Auswahl einer Datei handelt.
Besser noch ist es natürlich, wenn du die Parameter gar nicht als GET-Parameter über die URL überträgst, sondern lieber Formulare statt Links einsetzt und die Parameter dann als POST-Parameter empfängst ($_POST).

Jetzt bin ich gleich fertig. (Ja, ihr habt es gleich geschafft!)
Mir ist noch eine Kleinigkeit aufgefallen, die sich auf einen leicht überflüssigen Codeteil bezieht.
PHP:
if(!isset($_GET["page"])) {  

$page = "news";  

} else {  

if(isset($_GET["page"])) { 
$page = preg_replace( "/[^a-z0-9]/im", "", $_GET["page"]); 

} 
  }
Was passiert hier? Erst wird geprüft, ob der Parameter existiert. Ist das nicht der Fall gibt es eine Verarbeitung. Im anderen Fall, also wenn es den Parameter doch gibt, wird die Verarbeitung im else-Block durchgeführt. Aber worin besteht die? Nun zuerst wird geprüft, ob der Parameter existiert. Schon wieder? Wir wissen doch schon, dass er existiert. Deswegen sind wir doch überhaupt erst in den else-Zweig geraten. Also können wir diese Abfrage ohne Bedenken streichen. So geht's auch:
PHP:
if(!isset($_GET["page"])) {  

$page = "news";  

} else {  

$page = preg_replace( "/[^a-z0-9]/im", "", $_GET["page"]); 

}

Ich hoffe meine Anmerkungen waren interessant, verständlich und am Ende auch noch etwas hilfreich.
Mit Grüssen
Mitreder.
 
Der Teil hier funktioniert aber so nicht, da bekomm ich ne Fehlermeldung.

Parse error: syntax error, unexpected T_ARRAY, expecting T_STRING or T_VARIABLE or '$' index.php on line 640

PHP:
$whitelist = array("news"=>"dir1", "archiv"=>"dir2/subdir", "search"=>"dir1", "guestbook"=>"dir3", "inhalt"=>"dir2");   

if (!empty($page) && isset(array($whitelist[$page]))) {  

$file = '/'.$whitelist[$page].'/'.$page.'.php'; 

if(file_exists($file)) { include $file; } 

   }else{  

if(file_exists("./inhalt/fehler.php")) { include "./inhalt/fehler.php"; } 
}  
 }

Line 640 ist diese Zeile:

PHP:
if (!empty($page) && isset(array($whitelist[$page]))) {
 
PHP:
if(!isset($_GET["page"])) { 

$page = "news"; 

} else { 

$page = preg_replace( "/[^a-z0-9]/im", "", $_GET["page"]);

  }


$whitelist = array("news"=>"news", "archiv"=>"news", "search"=>"news", "inhalt"=>"inhalt");   

if (!empty($page) && isset($whitelist["$page"])) {  


$file = "./".$whitelist["$page"]."/".$page.".php"; 


if(file_exists($file)) { include $file; } 

   }else{

if(file_exists("./inhalt/fehler.php")) { include "./inhalt/fehler.php"; }

}

 }

So jetzt hab ichs soweit abgeändert, Dateien die nicht includiert sind habe ich eigentlich keine von daher denke ich nicht das ich unbedingt die Parameter mit Base64 bearbeiten muss.

Eure Meinung in Punkto Sicherheit ist mir noch wichtig was meint Ihr zu diesem Include Script.
Ich meine jetzt nicht die schwachstellen die PHP an sich eventuell mitbringt sondern ehr ob man hier noch mit Hacktools etc Schaden anrichten könnte (z.b. Scriptkiddies) die ihr Unwesen treiben :-)
 
Was mir momentan noch bissl Kopfzerbrechen bereitet ist das

PHP:
if (!empty($page) && in_array($whitelist, $page)) {

und das

PHP:
if (!empty($page) && isset($whitelist["$page"])) {

Meiner Meinung nach ist das in_array sicherer da ja genau überprüft wird was eingegeben wurde z.b. news.

Beim 2ten Codeschnipsel unten wird doch nur überprüft ob page nicht leer ist und ob die whitelist existiert aber nicht genau was drinsteht.
 
Aber da du ja eine Whitelist hast ist es relativ egal was in $page drinsteht, da du "böse" Sachen sowiso nicht erlaubst.

Übrigens kannst du den Code durch einen phpBeautifer jagen:

PHP:
if (!isset($_GET['page']))
	{
		$page = 'news';
	}
	else
	{
		$page = preg_replace('/[^a-z0-9]/im', '', $_GET['page']);
	}
	
	$whitelist = array('news'=>'news', 'archiv'=>'news', 'search'=>'news', 'inhalt'=>'inhalt');
	
	if (!empty($page) && isset($whitelist[$page]))
	{
		$file = './'.$whitelist["$page"].'/'.$page.'.php';
		
		if (file_exists($file))
		{
			include $file;
		}
	}
	else
	{
		if (file_exists('./inhalt/fehler.php'))
		{
			include './inhalt/fehler.php';
		}
	}
}
 
Zurück