Directory Traversal Attack erkennen / abwenden...

jens · Dienstag 3. Juli 2012, 08:48

Ich hab angefangen einen django filemanager zu implementieren. Soll nichts großartiges werden. Sinn ist eigentlich nur, das der admin sehen kann, welche statischen dateien existieren und wie die links dazu sind. Siehe auch: http://www.pylucid.org/en/blog/371/pylu ... 4-changes/ (Dort ist auch ein Bild, wie der momentane Stand aussieht)

Als Basis für den filemanager soll eine kleine Klasse bilden, die später allgemein zugänglich in django-tools einfließen soll. Momentan sind allerdings die sourcen, der einfachkeitshalber noch hier: https://github.com/jedie/PyLucid/tree/m ... ilemanager

Diese Basis Lib soll nur die Grundfunktionen liefern in einem Basis Pfad navigieren zu können.

Die Frage ist nun, wie kann man sicher verhindern das der User aus dem Basis Pfad ausbrechen kann? Also wie kann man ein sog. "Directory Traversal Attack" erkennen bzw. verhindern?

z.Z. mache ich es quasi so (Pseudocode):

Code: Alles auswählen

# urls.py
urlpatterns = patterns('',
    url(r'^filemanager/(?P<rest_url>.*?)$', views.filemanager),
)

# im view:
UNAUTHORIZED_SIGNS = ("..", "//", "\\")
for sign in UNAUTHORIZED_SIGNS:
    if sign in rest_url:
        raise 500

BASE_PATH = settings.STATIC_ROOT
abs_path = os.path.normpath(os.path.join(BASE_PATH, rel_path))
if not abs_path.startswith(BASE_PATH):
    raise 500

Der echte code ist momentan noch hier:
https://github.com/jedie/PyLucid/blob/m ... lemanager/
https://github.com/jedie/PyLucid/blob/m ... lemanager/

Die Frage ist, ob ich damit alles abdecke?

Hab mir auch überlegt, das die "user eingabe" also die rein kommende rel_path mehr auf Sonderzeichen absuchen soll. Doch nur Buchstaben/Zahlen erlauben würde auch "normale" Umlaute usw. ausschließen.

Meinungen dazu?

BlackJack · Dienstag 3. Juli 2012, 08:57

@jens: Ich würde überhaupt nicht auf irgendwelche Zeichen testen sondern einfach den Pfad zusammenbauen, absolut machen, und dann schauen ob das Ergebnis mit dem absoluten Basispfad beginnt. Falls nicht → Fehler. Das dürfte sowohl das einfachste als auch das sicherste Vorgehen sein.

jens · Dienstag 3. Juli 2012, 09:05

@BlackJack: Ja, das mache ich ja, mit if not abs_path.startswith(BASE_PATH):
Vielleicht sollte ich aber vorher noch os.path.abspath() machen?

Hab gerade mal getestet mit ein paar Beispielen von https://en.wikipedia.org/wiki/Directory ... sal_attack

"URI encoded directory traversal"

%2e%2e%2f which translates to ../
%2e%2e/ which translates to ../
..%2f which translates to ../
%2e%2e%5c which translates to ..\

Wird von der Prüfung mit UNAUTHORIZED_SIGNS abgefangen, weil Django die Zeichen vorher decodiert.

lunar · Dienstag 3. Juli 2012, 09:29

@jens: Ich schließe mich BlackJack an, und halte die vorherige Prüfung auf ungültige Zeichen für überflüssig und unsinnig. Noch dazu ist sie fehlerhaft, denn was genau beabsichtigst Du mit dem Verbot von "//"?!

Lass diese Prüfung einfach weg, und prüfe nur die von Django dekodierte URL in "rest_url"-Feld:

Code: Alles auswählen

relpath = rest_url.replace('/', os.sep)
abspath = os.path.join(BASE_PATH, relpath)
# Nur falls Du symbolische Verknüpfungen verhindern möchtest:
abspath = os.path.realpath(abspath)
normpath = os.path.normpath(abspath)
if os.path.isdir(normpath):
    normpath += os.sep
normbasepath = os.path.normpath(BASE_PATH) + os.sep
if not normpath.startswith(normbasepath):
    raise InvalidPathError()

Ich hänge "os.sep" an Verzeichnisse an, damit Verzeichnisse eindeutig terminiert sind. Da "normpath()" abschließende Trenner entfernt, könnte der Client andernfalls Verzeichnisse abfragen, die mit derselben Zeichenkette beginnen wie BASE_PATH, beispielsweise "/home/foobar/spam", wenn der BASE_PATH auf "/home/foo" steht. Durch das Anhängen des Trennzeichens findet der Vergleich aber zwischen "/home/foo/" und "/home/foobar/spam" statt, und verhindert dementsprechend den Wechsel in "/home/foobar".

Wozu Du "os.path.abspath()" benutzen möchtest, erschließt sich mir nicht. "os.path.abspath()" arbeitet mit dem aktuellen Arbeitsverzeichnis, das in diesem Kontext keine Rolle spielt, denn die Pfad sind ja schon anderweitig absolut.

jens · Dienstag 3. Juli 2012, 09:40

lunar hat geschrieben: Ich schließe mich BlackJack an, und halte die vorherige Prüfung auf ungültige Zeichen für überflüssig und unsinnig. Noch dazu ist sie fehlerhaft, denn was genau beabsichtigst Du mit dem Verbot von "//"?!

Die Idee ist einfach: Warum sich mit dem Pfad beschäftigen, wenn offensichtlich Zeichen eingefügt wurden, um einen Directory Traversal Attack zu machen?

Wobei man das selbe IMHO damit erreicht:

Code: Alles auswählen

clean_path = posixpath.normpath(rest_url)
if clean_path != rest_url:
    raise 500

lunar hat geschrieben:Ich hänge "os.sep" an Verzeichnisse an, damit Verzeichnisse eindeutig terminiert sind. Da "normpath()" abschließende Trenner entfernt, könnte der Client andernfalls Verzeichnisse abfragen, die mit derselben Zeichenkette beginnen wie BASE_PATH, beispielsweise "/home/foobar/spam", wenn der BASE_PATH auf "/home/foo" steht. Durch das Anhängen des Trennzeichens findet der Vergleich aber zwischen "/home/foo/" und "/home/foobar/spam" statt, und verhindert dementsprechend den Wechsel in "/home/foobar".

Ah! Das ist doch nochmal ein sehr guter Tip! Das muß ich beachten.
EDIT: Gemacht: https://github.com/jedie/PyLucid/commit ... 0302ece73c

Hab noch was gefunden: http://www.djangobook.com/en/2.0/chapter20/#cn104

Dort wird diese Lösung vorgeschlagen:

Code: Alles auswählen

import os
import posixpath

# ...

path = posixpath.normpath(urllib.unquote(path))
newpath = ''
for part in path.split('/'):
    if not part:
        # strip empty path components
        continue

    drive, part = os.path.splitdrive(part)
    head, part = os.path.split(part)
    if part in (os.curdir, os.pardir):
        # strip '.' and '..' in path
        continue

    newpath = os.path.join(newpath, part).replace('\\', '/')

der Code stammt wohl aus: https://github.com/django/django/blob/m ... /static.py

Dort wird der Pfad quasi gesäubert und dann ein redirect zu diesem pfad gemacht.

Ich weiß nicht. Ich finde es besser direkt einen Fehler zu werfen, wenn ".." oder "//" vorkommt. Warum säubern -> redirect und dann 404 werfen?

Wobei auch der code auch explizit nicht für den produktiven Einsatz ist.

lunar · Dienstag 3. Juli 2012, 10:37

@jens: Diese Zeichen sind nicht zwingend Ausdruck bösartiger Absicht. Ich weiß nicht, in wie weit HTTP Clientprogramme zur Normalisierung der URL zwingt, aber möglicherweise kann ein doppelter Schrägstrich auch einfach durch einen Tippfehler des Nutzers entstehen. ".." wird ein standardkonformer Client zwar nie senden, aber wer weiß schon, ob jeder noch so exotische HTTP-Client wirklich standardkonform ist.

Ich halte es für sinnvoller, möglichst defensiv und fehlertolerant zu programmieren, vor allem, wenn diese Vorgehensweise die Komplexität Deines Codes reduziert! AUch ist es vernünftig, eine bestimmte Aufgabe – wie die Prüfung auf gültige Pfade – an genau einer Stelle im Quelltext zu erledigen. Über kurz oder lang erleichtert Dir das die Wartung des Quelltexts.

jens · Dienstag 3. Juli 2012, 11:43

lunar hat geschrieben: AUch ist es vernünftig, eine bestimmte Aufgabe – wie die Prüfung auf gültige Pfade – an genau einer Stelle im Quelltext zu erledigen.

Genau das habe ich ja vor. Ich möchte eine Basis schaffen mit der man relativ einfach Apps wie Datei Browser, Datei Manager oder ähnlich machen kann.

Das verarbeiten der URL, als des relativen Pfades und zusammen setzten des Absoluten Dateipfades soll damit Zentralisiert werden.

Ich hab z.B. in PyLucid die öffentliche Gallery: http://www.pylucid.org/en/about/screenshorts/ und nun den internen Filemanager.

Beide haben gemeinsam, das ein User in einem Basis Pfad umher wandern kann. Wobei die Links ja vorgegeben sind. Also kann es eigentlich zu "falsch Angaben" kommen.
Von daher lege ich lieber weniger Wert auf Fehlertolerant und mehr Wert auf Sicherheit.

Ich hab mit nochmal den Django code von https://github.com/django/django/blob/m ... /static.py angesehen und daraus das gebaut: http://www.python-forum.de/pastebin.php?mode=view&s=290

Der Unterschied zur original Version von Django:
* Ein Slash am Anfang/Ende wird beibehalten: Django schneidet die immer weg.
* "/foo/bar/../../etc/passwd" wird bei mir zu '/foo/bar/etc/passwd' bei django zu 'etc/passwd'
* r"\foo\bar\..\etc\passwd" wird bei mir zu '/foo/bar/etc/passwd' und bei django zu '/foo/bar/../etc/passwd'
* r"c:\boot.ini" bei mir 'boot.ini' - django: 'c:/boot.ini'
* r"foo/bar/c:\boot.ini" bei mir: 'foo/bar/boot.ini' - django: 'foo/bar/c:/boot.ini'

Vorschläge für weitere Tests???

Damaskus · Dienstag 3. Juli 2012, 14:27

Ein Lösungsansatz von mir war mal alle Dateien in eine DB schreiben und von der DB aus wieder verteilen. Das waren damals aber auch nur PDFs und Bilder bei einer nur sehr schwach ausgelasteten Anwedung. Bei sowas großem wie einer Django App wird die DB vermutlich ziemlich schnell ziemlich voll.

Bei dem Thema fällt mir gerade ein... gibt es eigentlich eine Sammlung solcher "Maßnahmen" gegen Sicherheitslücken?

Wobei mir dann gleich wieder der Gedanke kommt, ob es sinnvoll wäre solche Lösungen zu verwenden? Denn wenn die Masse immer die gleichen Lösungen verwendet, dann ist eine Sicherheitslücke in der Lösung noch schneller ausgenutzt...

Gruß
Damaskus

jens · Dienstag 3. Juli 2012, 14:59

Damaskus hat geschrieben:Ein Lösungsansatz von mir war mal alle Dateien in eine DB schreiben und von der DB aus wieder verteilen. Das waren damals aber auch nur PDFs und Bilder bei einer nur sehr schwach ausgelasteten Anwedung. Bei sowas großem wie einer Django App wird die DB vermutlich ziemlich schnell ziemlich voll.

Ja, wäre sicherlich eine Lösung. Wobei das Problem dann nur ein wenig verschoben ist. Außerdem wäre es ein wenig umständlich, denn in meinem Fall geht es u.a. darum zu sehen, welche Dateien alles in settings.STATIC_ROOT und settings.MEDIA_ROOT sich befinden.

Damaskus hat geschrieben:Bei dem Thema fällt mir gerade ein... gibt es eigentlich eine Sammlung solcher "Maßnahmen" gegen Sicherheitslücken?

Wäre schon nett. Das Wiki würde sich anbieten, aber da würde es nur wieder verstauben...

Ich hab nun versucht alles umzusetzten: Die UNAUTHORIZED_SIGNS Geschichte weg und die Prüfung über filemanager.utils.clean_posixpath(): Wenn sich der Pfad ändert, dann stimmt was nicht.

Außerdem habe ich unittests angelegt. Kann jemand drüber sehen?

https://github.com/jedie/PyLucid/commit ... a7565ebd20

lunar · Dienstag 3. Juli 2012, 19:57

@jens: Verlinkte bitte auf die Dateien, oder noch besser auf die Funktionen, die Du kritisiert haben möchtest. Das Diff ist alles andere als übersichtlich, und es ist mir ehrlich gesagt auch ein bisschen viel Quelltext und API-Bloat, um da jetzt die Funktion herauszusuchen, die tatsächlich die Überprüfung implementiert.

Lies bei Gelegenheit vielleicht auch mal A Note About Git Commit Messages…

jens · Mittwoch 4. Juli 2012, 07:41

Mach ich:

Der wichtigste Teil ist clean_posixpath() hier:
https://github.com/jedie/PyLucid/blob/m ... ils.py#L17

Genutzt wird der dann in BaseFilesystemBrowser() hier:
https://github.com/jedie/PyLucid/blob/m ... ser.py#L77