CPO 4.1.5: CSRF check failed

Dieses Forum enthält Diskussionen zu Bugs in ConPresso 4 und deren Lösungen.
Benutzeravatar
MarkusR
Handbuchversteher
Beiträge: 7199
Registriert: 01.01.1970 02:00
Hat sich bedankt: 104 Mal
Danksagung erhalten: 920 Mal

CPO 4.1.5: CSRF check failed

Beitrag von MarkusR »

Ja, ich weiß, daß es daß schon als Titel gibt, aber da war es ja kein Bug sondern das korrekte Verhalten.

Jetzt bitte ich aber mal jemanden in einer 4.1.5 folgndes zu testen:

Wenn ich bei "Benutzer" auf "Eigene Daten bearbeiten" gehe und eine Änderung vornehme, dann erhalte ich beim Speichern zuerst
Sicherheitsprüfung fehlgeschlagen. Bitte versuchen Sie es noch einmal.
und bei erneutem Absenden ohne jegliche Änderung oder Korrektur
Der Benutzer LOGIN [NAME] wurde erfolgreich geändert.
Mache ich das Gleiche, wenn ich mich als Benutzer über "Benutzer bearbeiten" aufrufe und bearbeite, sind die Änderungen sofort erfolgreich.

Da wird anscheinend im ersten Fall ein falsches Token benutzt.
Einmal wird ja mittels user_self_modify und dann über user_modify das gleiche Formular aufgerufen, oder?
Ciao Markus
ConPresso-Module

Kein Support per PN!!! Für Fragen und Diskussionen ist das Forum da!

Succi recentis officinalis
Hochwertige Kräutersäfte und -Öle
Benutzeravatar
MarkusR
Handbuchversteher
Beiträge: 7199
Registriert: 01.01.1970 02:00
Hat sich bedankt: 104 Mal
Danksagung erhalten: 920 Mal

Beitrag von MarkusR »

Ich habe jetzt mal ein wenig im Code gestöbert und habe mal die CSRF-Session-Daten einblenden lassen.

Dabei fielen mir 2 Dinge auf:
1.) Das Ding füllt sich zunehmend
Array
(
[60db3bc02d9fd3ca498eb3def1bad25d] => users_self_save
[6bbf86b9fb5cfbcdc0f94d71389233d6] => setPerPage
[866602bd6f58c0ea1d70f9be4b013e6e] => setPerPage
[26ac0f52335cee36b9364eacb3b9a759] => setPerPage
[ef4866dad12107536f25b90b9d589f41] => setPerPage
[1f19612e3b6bacf47e3ca855b38cf2d0] => setPerPage
[b3e8987327f68709a756fc4e3594b401] => setPerPage
[eca50474cc8aae3a4468997cd44fe1f3] => users_self_save
[b722fb97d17e718b148eabcda47ee469] => users_self_save
[ab7c062bf4b8c0509e1c67a2a5062b3f] => setPerPage
[86171b69942fd1be28e163e49fd7732b] => setPerPage
[2ac2a01fd0fadd2f46182e3123972d46] => setPerPage
[ddfa202e1d64f5c5af2fb078abb91a08] => setPerPage
[565e37ffb2a44967a8eed31604fb1034] => users_self_save
...
2.) Es wird beim Auftreten des Fehlers gleich zweimal ausgegeben, also wird auch zweimal gecheckt!

Den Punkt 1 muß balu mal beleuchten, den Punkt 2 habe ich mir mal angesehen:
in _admin/users,php wird beim Empfang der abgesendeten Daten folgendes ausgeführt

Code: Alles auswählen

    case 'users_self_save': // store user data in database {{{
        if (!$antiCSRF->checkToken()) {
            $message[] = ___('Security check failed. Please try again.');
            logaction('placeholders delete', 'CSRF check failed.');
            $views = array('edit_form');
            break;
        }
    case 'users_save': 
        if (!$antiCSRF->checkToken()) {
            $message[] = ___('Security check failed. Please try again.');
            if (!empty($_REQUEST['id'])) {
                logaction('edit user', 'CSRF check failed.');
            } else {
                logaction('create user', 'CSRF check failed.');            
            }
            $views = array('edit_form');
            break;
        }
Es passiert also folgendes:
Im Falle von users_self_save wird die Prüfung durchgeführt. Da diese nicht fehlschlägt wird auch nicht abgebrochen sondern weiter ausgeführt. Also kommt anschließend die Prüfung von users_save, die natürlich fehlschlagen muß, weil bei einer erfolgreichen Prüfung der Eintrag aus dem Array entfernt wird, also nicht erneut geprüft werden kann.

Ich habe nun mal probeweise die erste Prüfung komplett entfernt und ... es klappt!
(Die zweite Prüfung findet ja dennnoch statt, also keine Panik :wink: )

Code: Alles auswählen

    case 'users_self_save': // store user data in database {{{
    case 'users_save': 
        if (!$antiCSRF->checkToken()) {
            $message[] = ___('Security check failed. Please try again.');
            if (!empty($_REQUEST['id'])) {
                logaction('edit user', 'CSRF check failed.');
            } else {
                logaction('create user', 'CSRF check failed.');            
            }
            $views = array('edit_form');
            break;
        }
Das mit entfernte

Code: Alles auswählen

logaction('placeholders delete', 'CSRF check failed.'); 
hat da ja eigentlich auch wenig zu suchen... :lol:
Ciao Markus
ConPresso-Module

Kein Support per PN!!! Für Fragen und Diskussionen ist das Forum da!

Succi recentis officinalis
Hochwertige Kräutersäfte und -Öle
Benutzeravatar
hscha
ConPresso-Experte
Beiträge: 573
Registriert: 22.02.2006 23:00
Wohnort: Berlin
Hat sich bedankt: 203 Mal
Danksagung erhalten: 19 Mal

Beitrag von hscha »

Hallo Markus,
ich konnte dieses Verhalten genauso nachvollziehen und habe in admin/users.php die erste Prüfung auskommentiert. Jetzt läufts ohne Fehlermeldung!

Gruß von
Horst
Benutzeravatar
balu
ConPresso-Entwickler
Beiträge: 1748
Registriert: 01.01.1970 02:00
Hat sich bedankt: 81 Mal
Danksagung erhalten: 133 Mal

Beitrag von balu »

Hallo Markus,

zu 1) Das ist richtig. Es gibt immer mal Formulare wie die setPerPage ("Anzahl pro Seite"), die zwar angezeigt, aber nicht abgeschickt und darum auch nicht aus der Session entfernt werden.

Darum wird in anti_csrf.inc.php auch die Anzahl der Tokens in der Session auf 1000 beschränkt (jeweils die ältesten werden gelöscht). So gibt es nicht irgendwann einen Speicherüberlauf wegen zuvieler Tokens.

Ich hatte überlegt, beim Prüfen der Tokens grundsätzlich alle Tokens zu entfernen, weil ja pro Seitenaufruf immer nur eine Aktion durchgeführt werden kann. Wenn man aber mit zwei Tabs arbeitet, gibt es eventuell einen Token für eine zweite ähnliche Aktion. Darum werden alle Token gespeichert.

zu 2) Gut gefunden. Ich hatte da wohl ursprünglich vor, zwei Checks durchzuführen - abhängig von der jeweiligen Aktion. Nachdem ich die Checks eingebaut habe, ist mir aufgefallen, dass $antiCSRF->checkToken(); ja sowieso die Aktion unterscheidet und eins reicht, habe aber die erste Prüfung nicht entfernt.

Das "placeholders delete" zeigt da deutlich, dass ich mir die Stelle Code nicht nochmal genau angeschaut habe. :-/

Balu
Bartels.Schöne
ConPresso Support & Development
Benutzeravatar
MarkusR
Handbuchversteher
Beiträge: 7199
Registriert: 01.01.1970 02:00
Hat sich bedankt: 104 Mal
Danksagung erhalten: 920 Mal

Beitrag von MarkusR »

Mal eine grundsätzliche Frage zur antiCSRF-Funktion...

Beim Betrachten des Codes fällt mir auf, daß nur POST-Variablen berücksichtigt werden, allerdings steht CSRF ja für Cross-Site Request Forgery und betrifft also auch jegliche Requests. Und davon gibt es ja etliche in ConPresso, die GET als Methode benutzen.

Nur mal als Beispiel:
Bei Modulen wird nur die finale Sicherheitsabfrage beim Deinstallieren durch antiCSRF geschützt, weil ein Formular genutzt wird. Alle anderen Aktionen laufen aber über ein GET. Es kann also ein Angreifer nach Lust und Laune Module aktivieren, deaktivieren und auch installieren, wenn sie auf dem Server als Verzeichnis vorhanden sind.

Genausogut könnte er auch Artikel sperren oder freigeben. Und er kann nach Lust und Laune hochgeladene Bilder und Dateien löschen. Und er kann deaktivierte User wiederherstellen.

Ja, mir ist klar, daß das bisher ja sowieso ging und ein Schutz für Formulare besser ist als gar kein Schutz. Aber wenn schon, dann könnte man die Funktion checkToken auf $_REQUEST umbauen.
Die Angabe der Variaben von Hand als Funktionsargumente wäre zwar auch eine Möglichkeit, aber eigentlich müsste die Beschränkung ja nicht sein, oder?

Dann müssten natürlich Aktionen wie das Abschalten eines Moduls mit einem Link in der Form

Code: Alles auswählen

../_admin/modules.php?action=modules_deactivate&module=MODULNAME&csrfToken=eca50474cc8aae3a4468997cd44fe1f3
ausgeführt werden, was man mittels angehängtem

Code: Alles auswählen

'&csrfToken='.$antiCSRF->getToken('modules_deactivate')
erreichen könnte.
Die Prüfung könnte man derzeit mit einem

Code: Alles auswählen

if (!$antiCSRF->checkToken($_REQUEST['action'], $_REQUEST['csrfToken'])) { ...
erzeugen. Schöner wäre es aber, wenn standardmä0g REQUEST statt POST ausgewertet würde und somit auch hier

Code: Alles auswählen

if (!$antiCSRF->checkToken()) { ...
reichen würde.
Ciao Markus
ConPresso-Module

Kein Support per PN!!! Für Fragen und Diskussionen ist das Forum da!

Succi recentis officinalis
Hochwertige Kräutersäfte und -Öle
Benutzeravatar
balu
ConPresso-Entwickler
Beiträge: 1748
Registriert: 01.01.1970 02:00
Hat sich bedankt: 81 Mal
Danksagung erhalten: 133 Mal

Beitrag von balu »

Hi Markus,

das Problem liegt eigentlich an anderer Stelle. Grundsätzlich sollten mit GET keinerlei "ändernde" Aktionen ausgeführt werden.

Siehe auch http://www.w3.org/TR/REC-html40/interac ... #h-17.13.1
The "get" method should be used when the form is idempotent (i.e., causes no side-effects). Many database searches have no visible side-effects and make ideal applications for the "get" method.

If the service associated with the processing of a form causes side effects (for example, if the form modifies a database or subscription to a service), the "post" method should be used.
"Idempotent" in dem Satz bedeutet, dass man einen Request beliebig oft wiederholen kann, ohne dass irgendwelche "Side-Effects" auftreten, also dass keine Änderungen durchgeführt werden.

Nach dem HTML-Standard dürften wir dort also gar keine GET-Anfragen generieren dürfen.

Aber wie Du Dir vorstellen kannst, ist das noch X-mal so viel Aufwand, die auch alle zu ersetzen. Weil der andere Bug aber einen schnellen Release erforderlich gemacht hat, haben wir das erst noch zurückgestellt.

Balu
PS: Siehe auch: http://thinkvitamin.com/code/the-defini ... t-vs-post/
Bartels.Schöne
ConPresso Support & Development
Benutzeravatar
Mr. Magpie
ConPresso-Profi
Beiträge: 1004
Registriert: 01.01.1970 02:00
Wohnort: Wuppertal
Hat sich bedankt: 274 Mal
Danksagung erhalten: 59 Mal

Beitrag von Mr. Magpie »

Balu, das heißt dann, dass ihr daran schon arbeitet, oder?
Günther Ludwig
Benutzeravatar
MarkusR
Handbuchversteher
Beiträge: 7199
Registriert: 01.01.1970 02:00
Hat sich bedankt: 104 Mal
Danksagung erhalten: 920 Mal

Beitrag von MarkusR »

Aber wie Du Dir vorstellen kannst, ist das noch X-mal so viel Aufwand, die auch alle zu ersetzen.
Da habe ich auch einige Zeit (nach meinem Posting) drüber nachgedacht. Auch wenn es grundsätzlich möglich wäre, ist es wenig praktikabel, weil auf jeder Seite Dutzende ungenutzte Tokens entstehen würden und damit die gesetzte 1000er-Grenze rasend schnell erreicht wäre. Wehe, wenn man dann in der Entwicklung mal ein print_r($_SESSION) einbaut :twisted:

Mittlerweile glaube ich, daß es - zumindest in einem komplexen CMS - keinen wirklichen Schutz dagegen gibt, außer eben nicht zu viele Webseiten parallel zur Arbeit im CMS zu nutzen.
Viel besser ist natürlich: Backup, Backup, Backup! Nach Murphy's Law knallt es ja doch irgendwann.
Und wenn sich halt doch ein aufsässiger Redakteur irgendwo ein kleines "automatisches Reaktivieren" in einen Artikel einsetzt, dann hat er ja noch keine Rechte.

Wobei mir ein weiterer "Bug" auffällt:
Wenn ich ein Mitglied reaktiviere (das ja bekanntlich keine Rechte hat und weshalb ja auch der Rechte-Button inaktiv ist) dann werde ich dennoch zur Rechte-Vergabe über einen einblendeten Link aufgefordert. Man kann ihm dann auch Rechte geben und diese erfolgreich abspeichern.
Ist zwar (vermutlich) nur ein kosmetisches Problem, aber vielleicht kann man das ja noch korrigieren (entweder kein Link oder bei der Rechtevergabe erst noch prüfen, wem man da gerade Rechte gibt...)
Ciao Markus
ConPresso-Module

Kein Support per PN!!! Für Fragen und Diskussionen ist das Forum da!

Succi recentis officinalis
Hochwertige Kräutersäfte und -Öle
Benutzeravatar
balu
ConPresso-Entwickler
Beiträge: 1748
Registriert: 01.01.1970 02:00
Hat sich bedankt: 81 Mal
Danksagung erhalten: 133 Mal

Beitrag von balu »

Mr. Magpie hat geschrieben:Balu, das heißt dann, dass ihr daran schon arbeitet, oder?
Das heißt, dass es auf unserer Liste steht und wir über eine Umsetzung nachdenken. Die Frage ist, ob sich der Aufwand im Vergleich zum "Sicherheitsgewinn" in diesem Fall lohnt.

Wie Markus schon richtig geschrieben hat, ist das auch ein Performance- und Speicherproblem.

Auf der Artikelliste gibt es pro Artikel ungefähr 14 Aktionen, die durchgeführt werden können und Änderungen erzeugen. Sortieren, Bearbeiten, Löschen, Freigeben, usw. Alle diese Aktionen können vom User rückgängig gemacht werden.

Da reicht es schon, sich 65 Artikel auf einer Seite anzeigen zu lassen und der CSRF-Speicher ist aufgebraucht. Man könnte den vergrößern, aber das muss auch alles in jeder Session gespeichert und bei jeden Seitenaufruf geladen und wieder gespeichert werden.

Und dann ist da die Frage, wie wahrscheinlich es ist, dass ein Hacker einen Artikel ohne Wissen des Redakteurs blocken oder umsortieren will. Wir reden hier nicht vom Editieren - das ist ja bereits geschützt.

Wir müssen schauen, ob und wie wir das machen können.

Balu
Bartels.Schöne
ConPresso Support & Development
Benutzeravatar
Mr. Magpie
ConPresso-Profi
Beiträge: 1004
Registriert: 01.01.1970 02:00
Wohnort: Wuppertal
Hat sich bedankt: 274 Mal
Danksagung erhalten: 59 Mal

Beitrag von Mr. Magpie »

Ich dachte ja auch eher an so Dinge wie aktivieren/deaktivieren/deinstallieren von Modulen.
Günther Ludwig
dutch
ConPresso-Experte
Beiträge: 547
Registriert: 04.07.2007 17:12
Hat sich bedankt: 27 Mal
Danksagung erhalten: 7 Mal

Beitrag von dutch »

Hallo zusammen,

ich habe gerade 4.1.5 installiert und bekomme die Meldung mit dem misglückten Sicherheits-check jedesmal, wenn ich das backend direkt aufrufe über ....../_admin/index.php
(Bei den bisherigen cpo Versionen klappte das immer so...)

Wenn ich dann im Menü auf "login" klicke und es nochmal versuche, klappt es mit der Anmeldung.

Ist das jetzt ein normales Verhalten und falls ja, wie rufe ich dann korrekt die backend-login Seite auf?

Grüße,
dutch


Nachtrag:
Habe eben nochmal getestet mit anderen Browsern. Der backend-login klappt ohne Probleme beim IE8. Die Sicherheits-check Meldung kommt aber bei Firefox, Chrome und Safari.
Benutzeravatar
MarkusR
Handbuchversteher
Beiträge: 7199
Registriert: 01.01.1970 02:00
Hat sich bedankt: 104 Mal
Danksagung erhalten: 920 Mal

Beitrag von MarkusR »

Prüfe mal, ob Du Dich schon mit der richtigen Domain anmeldest.

http://www.domain.de ist nicht zwingend das gleiche wie http://domain.de

Findet im Anmeldeprozess ein Wechsel statt?
Ciao Markus
ConPresso-Module

Kein Support per PN!!! Für Fragen und Diskussionen ist das Forum da!

Succi recentis officinalis
Hochwertige Kräutersäfte und -Öle
dutch
ConPresso-Experte
Beiträge: 547
Registriert: 04.07.2007 17:12
Hat sich bedankt: 27 Mal
Danksagung erhalten: 7 Mal

Beitrag von dutch »

Nein, es läuft immer über www.domain....
Auch wenn ich das www weglasse, springt die Adresszeile sofort auf www.domain....

Habe es jetzt schon sehr oft getestet aber es bleibt merkwürdig. Wenn die Anmeldung dann funktioniert hat (über den Umweg des "login" buttons), kann ich mich problemlos wieder abmelden und ohne Probleme auch wieder anmelden, nur klappt es eben nicht beim ersten Mal.
Das Problem taucht auch (meistens) dann wieder auf, wenn man die login-Seite neu lädt im Browser...


Gruß,
dutch
Benutzeravatar
balu
ConPresso-Entwickler
Beiträge: 1748
Registriert: 01.01.1970 02:00
Hat sich bedankt: 81 Mal
Danksagung erhalten: 133 Mal

Beitrag von balu »

Hallo,

das kann eigentlich nur mit Cookies / Sesssion-Problemen zusammen hängen. In der Session wird für jedes Formular ein Sicherheitscode gespeichert, der dann beim Übertragen des Formulars wieder geprüft wird.

Eventuell gibt es irgendein Plugin oder ähnliches, was Schabernack mit den Cookies im Browser macht und diese beim ersten Aufruf einer Seite nicht akzeptiert oder mitsendet?

Balu
Bartels.Schöne
ConPresso Support & Development
dutch
ConPresso-Experte
Beiträge: 547
Registriert: 04.07.2007 17:12
Hat sich bedankt: 27 Mal
Danksagung erhalten: 7 Mal

Beitrag von dutch »

Hallo,

es ist wieder eine Weile her, aber das Problem mit der Sicherheitscheck-Meldung taucht bei mir wieder (immer noch) auf.

Das letzte Mal kam es immer bei der Anmeldung im backend (mit einer 4.1.5). Dort taucht die Meldung jetzt nur noch ab und zu auf und ist zu verschmerzen.

Jetzt habe ich aber eine Seite mit cpo 4.1.6 erstellt, inkl. frontend login, und dort erscheint die Meldung bei jedem Loginversuch...

Wenn ich dann im Meldungfenster den Login wiederhole, bin ich eingeloggt.

Kann man das irgendwie beheben?

Gruß,
dutch