[bottle] Sicherheitslücken

[DasIch]

Die Anwendung die im Bottle Tutorial gebaut wird ist offensichtlich mit SQL Injections angreifbar. Mitsuhiko hat in #pocoo darauf aufmerksam gemacht wodurch ich dass überhaupt erstmal gelesen hab. Hat jemand sich mal bottle speziell unter dem Gesichtspunkt der Sicherheit unter die Lupe genommen? Ich denke nicht dass defnull daran nicht gedacht hat aber dass Tutorial verunsichert.

Im hinblick auf den Denied Scherz, der zusätzliche Aufmerksamkeit auf bottle werfen sollte dürfte, sollte dass schnell geklärt werden.

EDIT: Formulierung entschärft.

[Defnull]

Es gab bisher nur einen Bug, den ich als sicherheitsrelevant einstufen würde: Es war mit (sehr) frühen Bottle-Versionen möglich, die Root-Pfad Angabe von send_file() zu umgehen. Ansonsten gibt es nicht besonders viele Punkte, die bei einem Framework wie Bottle angreifbar wären. Dafür tut Bottle einfach zu wenig, das wirklich sicherheitsrelevant wäre (Dateioperationen, Sockets, Systembefehle kommen einfach nicht vor).

Schauen wir uns mal die Angriffsvektoren an: HTTP wird bereits vom Server-Backend geparst und verarbeitet. Überlange oder defekte Header werden also schon vorher abgefangen. Nur wsgi.input könnte fehlerhaft sein. Zum verarbeiten von wsgi.input nutzt Bottle ab einer gewissen Größe eine tempfile (um das Vollaufen des Speichers zu verhindern) und überlässt das eigentliche parsen der cgi.FieldStorage Implementierung. Wenn man davon aus geht, das sowohl "tempfile" als auch "cgi" aus der stdlib sicher sind, kann hier nicht viel mehr als eine Exception bei raus kommen. Exceptions sind aber kein sicherheitsrelevantes Problem, da sie abgefangen werden und nicht zum Ausfall des Servers führen.
Ansonsten greift Bottle noch lesend auf PATH_INFO und ein paar andere WSGI Variablen zu, die ja per definition saubere Strings sein sollten. Selbst wenn da Mist drin steht, Bottle tut damit nichts gefährliches.

Bleiben also noch Fehler in der Applikation, die zu Sicherheitsproblemen führen könnten. send_file() ist definitiv einer der wichtigsten Angriffsvektoren. Fehler in dieser Implementierung könnten dem Angreifer lesenden Zugriff auf Daten auf dem Server geben, eventuell sogar als root (wenn Bottle auf Port 80 laufen soll). Hier würde ich mit der Suche anfangen, wenn ich ein Angreifer wäre.
Ansonsten gibt es nur triviale Helfer-Funktionen, die nicht viel tun. Interessant wird es wieder bei zwei Features, die (aus gutem Grund) noch nicht dokumentiert und als experimentell gekennzeichnet sind: secure_cookies und Request.auth(). Erstere picklen und unpicklen Daten, die vom Client gesendet wurden (cookies) und damit als unsicher gelten. Allerdings wird hmac verwendet, um sicher zu stellen, das nur Daten verarbeitet werden, die vom Server selbst generiert wurden. Theoretisch ist das also sicher, solange der salt geheim bleibt, aber man weiß ja nie. Letzteres verarbeitet http-auth Daten. Der Algorithmus ist einfach genug, das ich mal behaupte, er tut das auch korrekt. Trotzdem könnte man diese Funktion angreifen, um unberechtigten Zugriff zu bekommen.

Ein weiterer großer Punkt sind Templates. Sie sind unsicher. Man sollte nie nie nie niemals Templates rendern, deren Quelle man nicht absolut vertraut. Punkt, Ende. Das ist aber gut genug dokumentiert, denke ich.

Du siehst also, ich mache mir durchaus Gedanken. Trotzdem ist ein zweiter oder dritter Blick definitiv sinnvoll.

Das Tutorial ist etwas naiv in manchen Punkten und sollte noch einmal überarbeitet werden, das stimmt auch. Mal sehen, was sich da machen lässt. Vielleicht schaut noisefloor (der autor) ja auch noch einmal drüber.

Denied hatte übrigens wenig Einfluss auf die Zugriffsstatistiken von paws.bottle.de. Der direkte Reddit-Link kurz vorher war deutlich stärker zu spüren. Die meisten Besucher kommen eh über Blogs oder google auf die Seite.

[mitsuhiko]

... überlässt das eigentliche parsen der cgi.FieldStorage Implementierung.

Die allerdings nicht fuer WSGI geeignet ist. Da wird readline() fuer multipart am input stream direkt aufgerufen ohne size hint (waere auch nicht gueltig). Auf der anderen Seite wird readline auch mit size hint an andere Stelle aufgerufen was wiederrum nicht erlaubt ist.

Resultat? Auf einem gueltigen WSGI Server laesst sich einfach ein Request machen, der den Socket offen haelt bis Request timeout weil der Content-Length falsch gesetzt ist und readline() drueberhinaus versucht was auszulesen. Evtl. funktioniert der ganze Code aber auch gar nicht und es bricht mit einer Exception ab, dass readline() keinen Size-Hint akzeptiert.

Sollte ersteres der Fall sein kann man damit einen Server auch schoen DoSen, je anachdem wie seine socket limits eingestellt sind.

Denied hatte übrigens wenig Einfluss auf die Zugriffsstatistiken von paws.bottle.de. Der direkte Reddit-Link kurz vorher war deutlich stärker zu spüren. Die meisten Besucher kommen eh über Blogs oder google auf die Seite.

Wundert mich nicht wirklich, weder denied noch der Blogpost danach hat bottle in irgendeiner Form erwaehnt.

[Defnull]

... überlässt das eigentliche parsen der cgi.FieldStorage Implementierung.

Die allerdings nicht fuer WSGI geeignet ist. Da wird readline() fuer multipart am input stream direkt aufgerufen ohne size hint (waere auch nicht gueltig). Auf der anderen Seite wird readline auch mit size hint an andere Stelle aufgerufen was wiederrum nicht erlaubt ist.

Genau aus diesem Grund packt bottle den Inhalt von wsgi.input vorher in einen Puffer oder eine tempfile (größenabhängig), die die entsprechenden size-parameter für readline unterstützen und den Speicher-Vollauf verhindern (Content-Length wird natürlich auch berücksichtigt, wenn vorhanden). Ohne size-limit wird read oder readline nur in cgi.parse_multipart() und damit auch in cgi.parse() aufgerufen. Diese Funktionen gehören allerdings zu einer veralteten API und werden von FieldStorage nicht verwendet.

Ein weiteres Problem von cgi.FieldStorage für WSGI ist, das es davon aus geht in CGI-Scripten verwendet zu werden und Daten in sys.argv oder sys.stdin sucht. Das kann man aber verhindern, wenn man sicher stellt, das gewisse environ-parameter immer vorhanden sind. Bottle baut daher ein sicheres environ-dict, statt blind das original weiter zu reichen.

Ansonsten ist cgi.FieldStorage zwar nicht schön, aber es funktioniert und ist ausgiebig getestet. Wenn man die Unterschiede zwischen CGI und WSGI und ihre Feinheiten kennt und sich die Zeit nimmt, cgi.FieldStorage aus den Quellen nach zu vollziehen, ist es durchaus brauchbar.

Resultat? Auf einem gueltigen WSGI Server laesst sich einfach ein Request machen, der den Socket offen haelt bis Request timeout weil der Content-Length falsch gesetzt ist und readline() drueberhinaus versucht was auszulesen. Evtl. funktioniert der ganze Code aber auch gar nicht und es bricht mit einer Exception ab, dass readline() keinen Size-Hint akzeptiert.

Noch lustiger wird es mit Python3, wo Teile des parsers plötzlich unicode erwarten, während andere nen Stream haben wollen und wieder andere still und heimlich von Bytes aus gehen. Dieses Problem auch noch 2to3 kompatibel zu lösen war echt ne Nuss Jaja, ich weiß, gleich kommt wieder 'WSGI existiert nicht für Python3' und das stimmt ja auch. Ich versuche aber, das Bottle mit der Referenz-Implementierung (WSGIRefServer) läuft. Bisher klappt das auch ganz gut.

[Hyperion]

Im hinblick auf den Denied Scherz, der zusätzliche Aufmerksamkeit auf bottle werfen sollte dürfte, sollte dass schnell geklärt werden.

Super genial Habe das jetzt erst durch mitsuhikos "Aufklärung" mitbekommen... vor allem der Screencast ist echt Spitze!

[noisefloor]

Hallo,

habe das Tutorial korrigiert, also mit

Code: Alles auswählen

c.execute("SELECT foo FROM bar WHERE spam LIKE ?", (egg))

Das mit der Ersetzung mittels %s vs. SQL-Injection war mir bis dahin so nicht bewusst - schließlich findet man ganz viele Beispiele mit regulärer %-Ersetzung im Internet. Da ist das mit der ?-Ersetzung aber auch die Tage in einem Buch gelesen habe glaube ich das jetzt mal.

Merke: Auch beim Tutorial-Schreiben kann man was lernen.

Gruß, noisefloor

[cofi]

Das mit der Ersetzung mittels %s vs. SQL-Injection war mir bis dahin so nicht bewusst - schließlich findet man ganz viele Beispiele mit regulärer %-Ersetzung im Internet. Da ist das mit der ?-Ersetzung aber auch die Tage in einem Buch gelesen habe glaube ich das jetzt mal.

Ganz so einfach ist das nicht. Je nachdem was die DB-Bindings unterstuetzt ist das gleichwertig, sofern die Funktionen der DB-Bindings benutzt werden und nicht die normale Formatierung.

Siehe http://www.python.org/dev/peps/pep-0249/ oder http://wiki.python-forum.de/Parametrisi ... QL-Queries

[lunar]

@cofi: Wie gleichwertig?! Du meinst, es gibt DBAPI-Bindings, bei denen die folgenden Zeilen den selben Effekt haben?

Code: Alles auswählen

c.execute("SELECT foo FROM bar WHERE spam LIKE ?", egg)
c.execute("SELECT foo FROM bar WHERE spam LIKE %s" % egg)

Dann wäre das DBAPI-Binding kaputt.

@noisefloor: Die Klammern um "egg" sind da vollkommen überflüssig.

[apollo13]

@cofi: Wie gleichwertig?! Du meinst, es gibt DBAPI-Bindings, bei denen die folgenden Zeilen den selben Effekt haben?

Code: Alles auswählen

c.execute("SELECT foo FROM bar WHERE spam LIKE ?", egg)
c.execute("SELECT foo FROM bar WHERE spam LIKE %s" % egg)

Dann wäre das DBAPI-Binding kaputt.

@noisefloor: Die Klammern um "egg" sind da vollkommen überflüssig.

er meinte glaube ich:

Code: Alles auswählen

c.execute("SELECT foo FROM bar WHERE spam LIKE ?", egg)
c.execute("SELECT foo FROM bar WHERE spam LIKE %s", egg)

Dann wäre es wieder völlig in Ordnung…

[jens]

Wir haben dazu die Seite: http://wiki.python-forum.de/Parametrisi ... QL-Queries
und dort steht u.a.:

... Zum Beispiel gibt es hier gleich fünf Arten des Quotens. Welche benutzt wird, kann man über dbapi.paramstyle herausfinden.

[snafu]

@apollo13: Beachte, dass `%s` hier durch `str(egg)` ersetzt wird. Und das wiederum ist abhängig davon, wie `egg` seine `__str__()`-Methode implementiert hat. Wird in SQL genau so vorgegangen? Ich kenne mich da nicht aus, würde aber vermuten, dass Lunar darauf hinaus wollte.

[noisefloor]

Hallo,

die Ersetzung mit ? funktioniert bei SQLite - und darum geht es im Tutorial. Dass das bei anderen DB-APIs evtl. nicht geht ist mir auch klar - aber es ist ein Bottle-Tutorial und kein Python SQL-Tutorial.

Gruß, noisefloor

[nemomuk]

2 Sachen, die mir aufgefallen sind: make_table.tpl ist unten in der Übersicht nicht gelistet und ein escapen der User-Eingaben erfolgt, soweit ich das auf die Schnelle sehen konnte, auch nicht. Ist vllt. auch nicht so wichtig bei einer todo-app, aber das Tutorial sollte ja mit gutem Beispiel voran gehen.

[snafu]

Es mag kleinkariert klingen, aber ich schließe mich der Meinung an. Tutorial-Code sollte mindestens so streng behandelt werden wie sonstiger öffentlich zugänglich gemachter Code. Denn gerade dies wird von Anfängern oft 1:1 übernommen. Und Bottle ist nunmal anfängertauglich...

Wohlgemerkt: "Sollte" != "Muss" ;P

[sma]

Ich habe gerade mal das Tutorial überflogen.

Es ist korrekt, dass die Benutzung der DBAPIs falsch und anfällig für SQL-Injection ist. Doch das wäre in wenigen Minuten korrigiert. Was mich viel mehr stört ist, dass die Code-Fragemente alle rechts aus dem Layout ausbrechen. Eine etwas kleinere Schrift und kürzere Zeilen würden hier die Lesbarkeit erhöhen.

Als URLs würde ich `/todos´ für die Liste und das Neuanlegen und `/todos/:no` für ein Element sowie `/todos/:no/edit` für ein Formular vorschlagen, weil das IMHO näher an dem RESTful Quasi-Standard liegt.

Statt darauf hinzuweisen, dass die Text maximal 100 Zeichen haben können, würde ich lieber ausnutzen, dass SQLite da keine solche Beschränkung für `char` oder `varchar` braucht und einfach beliebig lange Texte erlauben. Auch würde ich sagen, dass man Einträge einer TODO-Liste eher erledigt als schließt.

Bei `@validate` unter "Goals" fehlt die Code-Annotation.

Unter "Before We Start" wird etwas von SQLite und dessen Installation gesagt. AFAIK ist das auch schon bei Python 2.5 enthalten und da muss man gar nichts extra installieren. Das Kommandozeilen-Werkzeug `sqlite3` ist absolut entbehrlich für das Tutorial und die paar SQL-Befehle zum Anlegen der Datenbank würde ich doch einfach mit einem Python-Programm abschicken. Macht das ganze viel einfacher.

So in der Art:

Let's create a database to work with by executing the following Python program:

Code: Alles auswählen

    import sqlite3

    conn = sqlite3.connect('/tmp/todo.db')
    c = conn.cursor()
    c.execute("create table if not exists todo "+
        "(id integer primary key, task string, status bool)")
    c.executemany("insert into todo (task, status) values (?, ?)", (
        ("Read A-byte-of-python to get a good introduction into Python", 0),
        ("Visit the Python website", 1),
        ("Test various editors for and check the syntax highlighting", 1),
        ("Choose your favorite WSGI-Framework", 0)))
    conn.commit()

Ich finde übrigens, es ist relativ schlechter Stil, in `todo_list` (und anderen Funktionen) einfach so die DB-Verbindung hängen zu lassen, ohne sie explizit wieder zu schließen.

Später finde ich es ist schlechter Stil, eine Variable zuzuweisen und dann in der nächsten Zeile zurückzugeben

Code: Alles auswählen

    output = template(...)
    return output

Der Text selbst bemerkt es und erwähnt die bessere Alternative. Also gleich machen.

Mich wundert in dem späteren Beispiel, dass es möglich ist, dass Cursor-Objekt zu benutzen, nachdem man ein "commit" ausgelöst hat. Selbst wenn das innerhalb der DBAPI-Spezifikation ist, finde ich das fragwürdig und würde doch immer vorschlagen, dass `select last_insert_rowid()` unmittelbar nach dem INSERT zu machen, bevor das "commit" kommt. Auch böte sich hier vielleicht an, das "Rumgemache" mit der DB in eine eigene Funktion auszulagern, um nicht zu sehr Bottle (der Controller) und die DB (das Modell) zu vermischen.

Die Überschrift heißt "Using GET And POST values" aber ich sehe nicht, wo POST benutzt wird (was deutlich korrekter wäre als die Formulardaten mit GET zu verschicken). Spätestens wo man die Daten per Formular erfasst sollte man auf POST umschwenken.

Ach, und 403 hat nichts mit falschen Parameterwerten zu tun. Das ist mehr eine Antwort auf eine fehlgeschlagene Authentifizierung auf ein 401 hin. Besser wäre IMHO 400 (Bad Request). Auch ist natürlich 418 (I'm a teapot) möglich. Der passt eigentlich immer :)

Stefan

[jens]

Später finde ich es ist schlechter Stil, eine Variable zuzuweisen und dann in der nächsten Zeile zurückzugeben

Code: Alles auswählen

    output = template(...)
    return output

Das ist Geschmackssache. Ich bevorzuge das in Django Applikationen. Der Vorteil ist, das man die Werte damit im Traceback sieht. Ob das in bottle auch so ist, weiß ich nicht.

[noisefloor]

Hallo,

@sma: Einfach von Github forken, verbessern und an Defnull einen Pull-Request schicken.

Gruß, noisefloor

[Defnull]

Die sql-injection Probleme sind korrigiert und die Änderungen von noisefloor (neue Kapitel) habe ich eingepflegt. Außerdem gibt es auf der Homepage nun einen Link zur neuen Dokuemntation (testing).

Wenn ihr Fehler in der Dokumentation findet oder Verbesserungsvorschläge habt, dann immer her mit den Patches. Meine Zeit wird leider gerade etwas knapp (Semesterbeginn) aber ich versuche, alles so schnell wie möglich um zu setzen. Am liebsten sind mir forks auf github oder diff-patches per e-mail/pastebin. Zur Not geht aber auch die gesamte Datei mit den eingepflegten Änderungen als e-mail oder pastebin, dann merge ich das selbst.

[apollo13]

@apollo13: Beachte, dass `%s` hier durch `str(egg)` ersetzt wird. Und das wiederum ist abhängig davon, wie `egg` seine `__str__()`-Methode implementiert hat. Wird in SQL genau so vorgegangen? Ich kenne mich da nicht aus, würde aber vermuten, dass Lunar darauf hinaus wollte.

Ka, aber %s ist hier kein reines string format sondern nur ein place holder wie „?“. ich kann mir durchaus vorstellen, dass der Datenbank adapter das intern eh wieder durch „?“ ersetzt…