[php] Zugriff auf Ordner beschränken

LeGaN

Hi,

wenn nur der ordner und die Unterordner von ./files ausgelesen werden dürfen, kannst du ja auch den Pfad testen, ob der mit ./files anfängt, vlt. so?: preg_match('/^\.\/files/', $pfad)

LeGaN schrieb:

wenn nur der ordner und die Unterordner von ./files ausgelesen werden dürfen, kannst du ja auch den Pfad testen, ob der mit ./files anfängt, vlt. so?: preg_match('/^\.\/files/', $pfad)

Oh ... Ganz böser Denkfehler

Es hat schon seinen Grund, dass ich ihm dazu geraten habe, auf sämtliche Vorkommen von 0x00, "./" und "../" im String zu prüfen!

Würdest du nur auf "./files" am Anfang prüfen (was ich indes eher mit substr als via Regex machen würde), könnte ein potentieller Cracker diesen Pseudosicherheitsmechanismus in Sekunden knacken, indem er z.B. "./files/../badFile" als Parameter übergibt. Abgesehen davon: Wenn ./files das Basisverzeichnis sein soll, dann wird das Skript vermutlich alle Pfadangaben dazu relativ zu diesem Verzeichnis berechnen wollen und mithin nicht "./files/subdir" als Parameter erzeugen sondern gleich "subdir". In sofern wäre die Abfrage nicht nur absolut unsicher, sondern auch noch umständlicher als nötig

Eine Prüfung am Ende des Strings ist ebenfalls nicht sinnvoll, denn mit einem 0x00 inmitten des Strings könnte ein Angreifer den String vorzeitig "beenden" (in C ist ist das Zeichen 0x00 das Endzeichen einer nullterminierten Zeichenkette)!

Ein str_replace, das böse Inhalte herausfiltert, ist auch nicht gut. Versuche niemals, kontaminierte Daten zu revidieren! In dem Moment, in dem du eines der Zeichen 0x00, ./ oder ../ in deinem String hast, kannst du sicher sein, dass da jemand (oder etwas) versucht, bösartige oder zumindest unvorhergesehene Dinge zu tun! Die Skriptausführung ist folglich unmittelbar abbzurechen!

Das ist auch der Grund, warum in "sicheren" Systemen möglichst nie mit Black- sondern mit Whitelists gearbeitet wird: Kontaminierte Daten zu reparieren ist fahrlässig und fehlerträchtig, und verdächtige - spricht: nicht zu 100% sichere - Angaben können meist nicht hingenommen werden.

Fazit: Prüfe, ob böse Zeichen in deinen Parametern enthalten sind. Falls ja, breche die Skriptausführung gnadenlos ab.

LeGaN

Hi,

ah, danke. Das war mir bis jetzt nicht klar. Ich wusste gar nicht, dass solche Pfade wie ./files/../blubb überhaupt gehen. jetzt bin ich wieder schlauer, danke

mantiz

Naja, man kann aber solche Pfade "./files/../badfile" auch auflösen und erhält dann "./badfile" und dort kann man dann darauf prüfen, ob der Pfad mit "./files" beginnt.

Oder gibt es da dann auch noch eine Schwachstelle?

"./files/./tmp" -> "./files/tmp" => OK
"./files/../tmp" -> "./tmp" => Böse

Alles andere dürfte kein Problen sein, wenn man vorher alles, was nicht Buchstabe, Ziffer, "." oder "/" ist rausfiltert. Oder sehe ich das falsch?

"./files/.../...//badfile" -> "./files/../badfile"

So sicher man sich auch ist, alle Fälle abgedeckt zu haben: Es brigt einfach nichts, zu versuchen, an korrupierten Werten herumzufuchteln. Warum auch!? Das Skript von sich aus wird solche "bösartigen" Pfade nicht erzeugen. Du kannst also relativ sicher sein, dass in dem Moment, in dem so etwas vorliegt, jemand vorsätzlich die Parameter manipuliert hat. Wieso solltest du jetzt versuchen, diese bewusst bösartig umgestalteten Daten zu korrigieren?

mantiz

"./files/../..//badfile" -> "./files/../../badfile" -> "../badfile" => Böse
und somit wieder als solches identifizierbar.

Wieso soll ich denn immer mit "absoluten" Pfaden hantieren, wenn es relative Pfade auch tun?

Hier gibt es z.B. eine Funktion, die das bereits erledigt und man findet in den Comments auch einige Ansätze, wie man sich die Funktion selbst basteln kann. So aufwändig ist das doch gar nicht.

Mag sein, dass ich etwas übersehe, aber wenn ich zuerst alle unerlaubten Zeichen rausfiltere und dann den Pfad z.B. per

$path = realpath($_SERVER['DOCUMENT_ROOT'] . '/files/' . $relative_path);

zusammenbaue, dann kann ich doch prüfen, ob der Pfad mit "$_SERVER['DOCUMENT_ROOT'] . '/files/'" beginnt und falls nicht, dann böse, ansonsten OK, oder gibt es da auch noch ein Sicherheitsleck?

Evtl. nicht realpath verwenden, da ich nicht weiß, ob die Bugfree ist, sondern eine aus den Comments, die man vorher natürlich genau unter die Lupe nimmt.

Nun, eine so primitive Angelegenheit wie diese hier lässt sich sicherlich weitgehend wasserdicht ausprogrammieren - Aber wozu die Mühe? Was bringt es dir, Angreifer, die mutwillig Parametermanipulation betreiben, zu korrigieren? Ein Pfad, der in diesem Fall ein "../" beinhaltet ist schlichtweg falsch! Das Programm soll mit solchen Pfaden nicht hantieren, also wird die Behandlung abgebrochen.

Wieso solltest du versuchen, Fälle zu korrigieren, die nur unter der Voraussetzung, dass dein Skript angegriffen wird, überhaupt entstehen können!?
Du investierst Zeit, Mühe und im Berufsleben somit Geld darein, ein Problem zu lösen, das keines ist.

Wenn ein Tabellenkalkulationsprogramm die Anzahl an Spalten vom Benutzer abfragt, die gedruckt werden sollen, und es bekommt "Hallo Welt" geliefert, versucht es auch nicht, die Werte zu berichtigen. Sie sind schlichtweg falsch und werden verweigert!

Es geht mir eher um das Prinzip, und das lautet glasklar: Whitelist ja, Blacklist nein! Versuche nicht, schwarze Schafe zu korrigieren, sondern sortiere sie gnadenlos aus. Sonst stehst du irgendwann vor einem Problem, bei dem du ebenfalls der Meinung bist, es wasserdicht gelöst zu haben. Dies zu denken ist aber naiv!

mantiz

Naja, mir geht es hierbei nicht direkt um das im ersten Post beschriebene Problem, aber ich denke da an z.B. Template-Engines.

Angenommen eine Template-Engine kennt den Pfad des Templates, welches gerade geparst wird und darin werden Sub-Templates eingebunden, dann kann das von mir aus so laufen:

<include file="/path/to/sub-template.tpl" />
<!-- oder auch so -->
<include file="|tplpath|/sub-template.tpl" />

Was aber, wenn die Sub-Templates ein Verzeichnis höher liegen?
Dann müsste eine 2. Pfad-Variable hinzukommen, was, wenn man kreuz und quer Templates einbinden möchte?
Für jedes Template eine eigene Pfad-Variable?

Da stelle ich mir sowas, wie

<include file="sub1/template1.tpl" />
<include file="../another-template.tpl" />

wesentlich schöner vor.

Vor allem, wenn die Templates noch dazu in verschiedenen Verzeichnissen gruppiert sind und man eine ganze Gruppe umkopieren möchte.
Mit relativen Pfadangaben hast Du überhaupt kein Problem, Du musst nur Pfade anpassen, die nach ausserhalb des Verzeichnisse zielen, wenn Du aber Pfade hast, die innerhalb des Verzeichnisses relativ gleich bleiben, dann kopierst Du um und bist fertig.

Natürlich kann man mit einem schönen Editor sehr schnell alle notwendigen Anpassungen vornehmen, aber wieso unnötig Arbeit machen?
Man überlegt einmal gründlich und ist fertig damit.

Früher habe ich auch nur absolute Pfade verwendet und hatte auch nie Probleme, auch in statischen HTML-Seiten hab' ich stets absolute Pfadangaben benutzt, dabei kann das Leben doch so einfach sein.

Dynamische Pfade an sich sind nicht böse, bzw. können statische Pfade genau so einfach kontaminiert werden. Ob du ein /test/../passwd oder ein ./test/../passwd hast macht da keinen großen Unterschied

Wie gesagt, mir geht es um ein grundlegendes Sicherheitskonzept. Wenn eine Template-Engine das Einbinden von anderen Templatedateien in den Template-Dateien der ersten Hierarchieebene erlaubt, ist das an sich nicht schlimm.
Gefährlich wird es dann, wenn die Einbindung dynamisch erfolgen kann, also z.B. der Pfad aus einem GET-Parameter kommt. Das ist in einer Template-Engine aber in der Regel nicht der Fall, sondern die Pfade stehen hartkodiert - aber relativ! - in der Templatedatei erster Hierarchieebene. Das ist auch sinnvoll so, denn wieso sollte ein Benutzer per GET-Parameter frei das verwendete Template bestimmen dürfen? Wenn, dann doch höchstens eine Auswahl aus im System fest deklarierten Templates, wobei die Auswahl in Form des Get-Parameters dann wiederum mit einer Whitelist abgeglichen werden kann.

Wer dann noch meint, die Pfade prüfen zu müssen, bei dem leckt die Sicherheit an ganz anderer Stelle - Denn wenn schon die Templatedateien selbst manipuliert wurden, der Angreifer somit bereits Zugriff auf das Dateisystem besitzt, der kann ohnehin nur noch den "Off"-Schalter seines Server drücken, um weiteren Schaden zu vermeiden

$templates = array('template1', 'template2' /* ... */);
if(!in_array(@$_GET['template'], $templates))   {
   exit;
}
$template = $_GET['template'];

// ...

$template->parse(ROOT_DIR.'templates/'.$template.'/eineDatei');

mantiz

Ich seh' schon, Du lässt Dich von Deiner Ansicht nicht abbringen.

Ich aber ebenso wenig, befürchte ich. Solange es keine sicherheitskritischen Gründe dafür gibt, welche ich nachvollziehen kann, werde ich das wohl auch weiterhin so machen.

Ich habe z.B. gerne ein "Haupt-Template", wo der Rahmen der Seite festgelegt wird und dann per Parameter der Inhalt da rein kommt. Per Script wird geprüft, ob das übergebene Template existiert und gültig ist und ggfs. ein Template mit Fehlermeldung da reingesetzt oder halt das gewünschte.

Wenn ich die Seite erweitere, dann bau ich das neue Template und setz' irgendwo einen Link auf "index.php?tpl=folder/neues-template" und bin fertig.
Aber man könnte das Template natürlich auch in eine Whitelist innerhalb des Scriptes eintragen, allerdings sehe ich nicht die Notwendigkeit dafür, wenn ich den Pfad vorher bereinige.
Wenn man natürlich einfach nur den übergebenen Pfad nimmt, wie er ist, dann ist man selbst Schuld.

Radiation2K5

deinem ersten posting nach zu beurteilen hast du open_basedir nicht gesetzt.
das ist schonmal die erste sicherheitslücke setz das auf dein homepage hauptverzeichnis, dann kann man damit schonmal einiges abfangen.

um dein eigentliches problem zu lösen wurden ja schon genug lösung gepostet.
trotzdem würd ich dazu gern meinen senf hinzugeben:
1. bei vorkommen von 0x00 -> script abbrechen, evtl. dann hauptverzeichnis auflisten oder fehler ausgeben
2. bei vorkommen von ./ -> script ausführen, nichts ändern
3. bei vorkommen von ../ -> script ausführen, in ./ umwandeln
4. file_exists prüfung durchführen
5. directory_exists prüfung durchführen

des Weiteren könntest du auch einfach eine .htaccess datei in dem /files ordner ablegen in der du '+Indexes' erlaubst. dann würdest du dir die ganze arbeit sparen es sei denn AllowOverride steht auf None.

Gruß
RadiatioN

mantiz

Bin mir jetzt nicht sicher, ob Deine Antwort mir galt, aber ich geh' einfach mal drauf ein.

Radiation2K5 schrieb:

deinem ersten posting nach zu beurteilen hast du open_basedir nicht gesetzt.
das ist schonmal die erste sicherheitslücke setz das auf dein homepage hauptverzeichnis, dann kann man damit schonmal einiges abfangen.

Wenn ich der einzige bin, der PHP-Scripte auf dem Server erstellt, ist mir das eigentlich Schnuppe, da ich in den Scripts sowieso prüfe.
Als Hoster muss man natürlich darauf achten, dass man den Kunden bestimmte Dinge per Default verbietet, da man sich da nicht drauf verlassen kann, dass diese keine Mist bauen.

Radiation2K5 schrieb:

um dein eigentliches problem zu lösen wurden ja schon genug lösung gepostet.
trotzdem würd ich dazu gern meinen senf hinzugeben:

Da ich ja eigentlich kein Problem hatte galt das wohl nicht mir.

Radiation2K5 schrieb:

1. bei vorkommen von 0x00 -> script abbrechen, evtl. dann hauptverzeichnis auflisten oder fehler ausgeben

Wenn man hierauf prüft, dann kann man es auch filtern.

Radiation2K5 schrieb:

2. bei vorkommen von ./ -> script ausführen, nichts ändern
3. bei vorkommen von ../ -> script ausführen, in ./ umwandeln

Wie man das auflösen kann, hab' ich ja bereits geschrieben.

Radiation2K5 schrieb:

4. file_exists prüfung durchführen
5. directory_exists prüfung durchführen

Sollte selbstverständlich sein, dass man prüft, ob das, was man anzeigen will überhaupt da ist. Besser als file_exists ist aber imho is_file, da file_exists bei mir bisher auch immer auf Verzeichnisse angewandt true zurückgeliefert hat, was aber so nicht stimmt.

Radiation2K5 schrieb:

des Weiteren könntest du auch einfach eine .htaccess datei in dem /files ordner ablegen in der du '+Indexes' erlaubst. dann würdest du dir die ganze arbeit sparen es sei denn AllowOverride steht auf None.

Galt wohl auch nicht mir.