Was haltet ihr von dieser Includemethode?

[INC.]

Hallo

Es geht - ganz klassisch - darum, über die index.php eine Seite zu includen, die über die Url entgegengenommen wird (also z.B. index.php?site=news).

Dazu habe ich folgendes geschrieben:

if (!preg_match("#^[A-Za-z0-9]$#", $_GET['page'])) {
die();
} else {
$site = 'includes/' . $_GET['page'] . '.php';
  if (file_exists($site))
  {
        include($site);
  }
} 

Was haltet ihr davon, vor allem Sicherheitstechnisch? Es wurde mir gesagt, dass es verdammt unsicher ist, mit einer Usereingabe DIREKT auf das Dateisystem zu zielen. Daraufhin hab ich noch den Regexteil hinzugefügt.

Also:

- Der Parameter aus der URL darf nur A-Za-z0-9 enthalten
- die Datei muss auf PHP enden
- die Datei muss im includes-Ordner stecken

Der Parameter wird ja regelrecht in den Pfad gepresst.
Ich hätte das eigentlich für verdammt sicher gehalten, aber werde gerne eines besseren belehrt.

[onemorenerd]

Passt schon. Unsicher wirds erst, wenn jemand einen Bug in preg_match() entdeckt.
Langsam ist es natürlich jetzt schon und auch nicht besonders sumafreundlich.

[Blackgreetz]

Warum nicht fest definieren über eine switch?

Ansich kann er somit alle Dateien aufrufen, die existieren. Egal, ob man sie sehen soll, oder nicht.

mfg

[INC.]

Hallo, danke schonmal

Passt schon. Unsicher wirds erst, wenn jemand einen Bug in preg_match() entdeckt.
Langsam ist es natürlich jetzt schon und auch nicht besonders sumafreundlich.

Ich nehme mal an, file_exists ist der Flaschenhals? bei so Sachen fehlt mir einfach dieses Wissen, aber "langsam" ist ja auch manchmal subjektiv.

Aber warum ist es nicht suchmaschinenfreundlich? (ich nehme mal an, dass "suma" Suchmaschine bedeutet ).
Da die Suma den PHP-Code nicht zu gesicht bekommt, ist es doch genauso (wenig) sumafreundlich wie alle anderen Varianten, die "index.php?site=news" verarbeiten können
Am Ende gibt diese URL doch nur den HTML-Code der News aus, genauso wie alle anderen Möglichkeiten.

Warum nicht fest definieren über eine switch?

Das ist meine bisher verwendete Methode, von der ich mich nun trennen wollte. Es ist einfach verdammt umständlich, jedes mal bei einer neuen Seite, die angelegt werden soll, den switch zu erweitern und wieder hochzuladen - statt einfach nur die neue Datei hochzuladen.

Ansich kann er somit alle Dateien aufrufen, die existieren. Egal, ob man sie sehen soll, oder nicht.

Es kann nur includet werden, was in einem speziell dafür angelegten Ordner ist (in dem Fall "includes").

[PHP-Desaster]

Ich nehme mal an, file_exists ist der Flaschenhals? bei so Sachen fehlt mir einfach dieses Wissen, aber "langsam" ist ja auch manchmal subjektiv.

Nein, das preg_match! Reguläre Ausdrücke sind (fast) immer langsamer als die einfachen String-Funktionen.

Aber warum ist es nicht suchmaschinenfreundlich?

Denke, onemorenerd meint damit, dass du immer über die index.php gehst. Ein mod_rewrite in der Form:

http://www.example.com/news

wäre sicher nicht verkehrt.

Das du bei nicht-übereinstimmen des Regexp einfach stirbst ist ganz und gar nicht Sinnvoll! Da wäre eine Anzeige der Startseite oder einer 404 mit Suchfeld sinnvoller.

[INC.]

Original geschrieben von PHP-Desaster
Nein, das preg_match! Reguläre Ausdrücke sind (fast) immer langsamer als die einfachen String-Funktionen.

Okay. Dann verwende ich am besten ctype_alnum? Dürfte schneller sein.
Habe regex verwendet, da ich damit bei Formularen immer whitelisting betreibe und es deshalb gewohnt war.

Original geschrieben von PHP-Desaster
Denke, onemorenerd meint damit, dass du immer über die index.php gehst. Ein mod_rewrite in der Form:

http://www.example.com/news

wäre sicher nicht verkehrt.

mod_rewrite wird kommen. Aber über index.php zu gehen ist ja praktisch das Fundament der ganzen Sache, mod_rewrite schreibts ja praktisch nur um?

Original geschrieben von PHP-Desaster
Das du bei nicht-übereinstimmen des Regexp einfach stirbst ist ganz und gar nicht Sinnvoll! Da wäre eine Anzeige der Startseite oder einer 404 mit Suchfeld sinnvoller.

Habe da nur ein die() geschrieben, um Codezeilen im Beitrag zu sparen. Eigentlich inkludiere ich ne 404-Seite, so dass keiner auf die Idee kommt, dass die Datei überhaupt existiert.

[PHP-Desaster]

Okay. Dann verwende ich am besten ctype_alnum? Dürfte schneller sein.

Zum Beispiel. Eigentlich sollte eine Kombination aus basename und file_exists schon genügen.

mod_rewrite wird kommen. Aber über index.php zu gehen ist ja praktisch das Fundament der ganzen Sache, mod_rewrite schreibts ja praktisch nur um?

Jep.

[jahlives]

if(isset($_GET['page'])){
  $file = basename($_GET['page']);
  if(file_exists("includes/{$file}.php")){
    include("includes/{$file}.php");
  }else{
   //hier kannst du alternativ einen 404-er werfen
    die('Seite nicht gefunden');
  }
}else{
  //Hauptseite anzeigen
} 

RegExp brauchst du imho ned. basename() sollte dafür sorgen dass keine ../ und ./ im Dateinamen diese Operation überleben

[UzumakiNaruto]

Original geschrieben von jahlives

if(isset($_GET['page'])){
  $file = basename($_GET['page']);
  if(file_exists("includes/{$file}.php")){
    include("includes/{$file}.php");
  }else{
   //hier kannst du alternativ einen 404-er werfen
    die('Seite nicht gefunden');
  }
}else{
  //Hauptseite anzeigen
} 

RegExp brauchst du imho ned. basename() sollte dafür sorgen dass keine ../ und ./ im Dateinamen diese Operation überleben

include("includes/{$file}.php"); was ist das den für eine schreibweise? meine natürlich die {}

[combie]

was ist das den für eine schreibweise?

Siehe Handbuch: http://www.php.net/manual/de/language.types.string.php

[jahlives]

was ist das den für eine schreibweise?

Vars sollten in " und " auch ohne {} gefunden werden. Aber z.B. in der IDE erhöht dieses Schreibweise die Lesbarkeit enorm, denn so werden die Vars auch als solche dargestellt und nicht einfach als Teil des Strings
Und hier finde ich {} auch nötig

//dat gibt nen Fehler
echo "A banana is $fruits['banana'] and tastes great";
//und dat ned
echo "A banana is {$fruits['banana']} and tastes great";
//und hier müssen sie sein
$fruits = 'banana';
echo "{$fruits}tastes great";
//ohne { würde PHP nach einer Var $fruitstastes suchen und nicht finden 

[h3ll]

Original geschrieben von jahlives
Vars sollten in " und " auch ohne {} gefunden werden. Aber z.B. in der IDE erhöht dieses Schreibweise die Lesbarkeit enorm, denn so werden die Vars auch als solche dargestellt und nicht einfach als Teil des Strings
Und hier finde ich {} auch nötig

Zend Studio stellt es auch ohne {} als Variable dar.

[INC.]

Okay, danke

Das mit dem basename versteh ich noch nicht so ganz.
Laut manual extrahiert die Funktion ja aus einem gegebenen Pfad einen Dateinamen.

Nun übertragt get ja sowas wie "news" (zumindest im besten Fall), aber vielleicht auch einen Pfad wenn der Angreifer will, oder vielleicht auch einen Code um einen eventuellen Bug in basename ausnutzen zu können (zu letztem: ka ob das möglich ist, aber mal angenommen).

Auf php.net kann man aber imho nirgends lesen, dass auch garantiert alle slashes entfernt werden. Da man ja nie weiß, was der User eingibt, sollte es ja eher heißen: basename _versucht_ den Dateinamen zu extrahieren. Und selbst wenn alle slashes entfernt werden, sind ja immernoch alle anderen Zeichen im String enthalten, die evt. was böses ausrichten könnten (was auch immer).

Bei ctype_alnum würde halt schonmal ordentlich im voraus gefiltert werden.

(Was macht basename eigentlich exakt? Sucht es immer das letzte Wort ohne slash am Ende eines Strings?)


edit, off topic:
@ jahlives, wäre statt echo "A banana is {$fruits['banana']} and tastes great"; nicht sowas wie print_f("A banana is %s and tastes great", $fruits['banana']); besser?

[jahlives]

Also wenn ich basename('news') mache kriege ich wie gewünscht news raus. Egal wieviele ./ und ../ ich davor anhänge.
Jegliche Pfadangabe wird entfernt.

Und selbst wenn alle slashes entfernt werden, sind ja immernoch alle anderen Zeichen im String enthalten, die evt. was böses ausrichten könnten (was auch immer).

Das wichtigste beim include eines GET Parameters ist es alle Pfadangaben rauszuhauen und das macht basename() sicher. Die anderen Zeichen sind erstmal nicht so tragisch, da du ja zusätzlich prüfst ob die Datei in deinem vorgegebenen Verzeichnis existiert. Oder welche "gefährlichen" Zeichen meinst du sonst?

@ jahlives, wäre statt echo "A banana is {$fruits['banana']} and tastes great"; nicht sowas wie print_f("A banana is %s and tastes great", $fruits['banana']); besser?

imho Geschmackssache

[INC.]

Original geschrieben von jahlives


Das wichtigste beim include eines GET Parameters ist es alle Pfadangaben rauszuhauen und das macht basename() sicher. Die anderen Zeichen sind erstmal nicht so tragisch, da du ja zusätzlich prüfst ob die Datei in deinem vorgegebenen Verzeichnis existiert. Oder welche "gefährlichen" Zeichen meinst du sonst?

Naja, bin da Laie, keine Ahnung, aber irgendetwas, was die Seite oder basename()/file_exists aus dem Takt bringen könnte (bei einem evt. Exploit oder Bug). Nur mit Buchstaben und Zahlen wäre das eher unmöglich.