Bei der Analyse eines Kundenprojekts ist uns in einem kommerziellen Modul eine schwere Sicherheitslücke aufgefallen. Es handelt sich um eine Lücke vom Typ SQL Injection, die es ermöglicht, als einfacher Besucher quasi beliebige Datenbankoperationen durchzuführen. Das können beispielsweise sein:
- Löschen beliebiger oder aller Datenbanktabellen
- Erstellen eines neuen Admin-Users mit vollen Zugriffsrechten
- Auslesen beliebiger Kundendaten
Erzielt wird das durch eine modifizierte URL zum Aufruf von Formularseiten im Shop. Um das Ausnutzen der Sicherheitslücke nicht zu leicht zu machen, veröffentlichen wir die zum Testen verwendeten präparierten URLs hier nicht.
Das betroffene Modul nennt sich Magento Custom Forms Extension und wird vom Hersteller Magik vertrieben. Es ist auf MagentoConnect und beim Hersteller kostenpflichtig verfügbar.
Wir haben dem Hersteller die Sicherheitslücke gemeldet. Er hat die Template-Datei überarbeitet – leider trat die Sicherheitslücke auch im geänderten Quellcode noch auf.
Der Quellcode
Ermöglicht wird das Ausnutzen der Sicherheitslücke dadurch, dass Parameter, die beim Aufruf des Shops in der URL enthalten sind, nicht ausreichend geprüft werden. Dies ist nur möglich durch Umgehung von mehreren „Best Practices“ in der Magento-Entwicklung. So wird eine direkte SQL-Abfrage im Extension-Code verwendet, anstatt der zur Verfügung stehenden Magento-Methoden (Collections). Auch wäre es möglich gewesen, die Sicherheitslücke durch so genannte „Prepared Statements“ zu umgehen, bei der alle Parameter nochmal explizit gefiltert werden.
Stattdessen befinden sich folgender Code in der Datei app/design/frontend/base/default/template/customforms/customforms.phtml:
|
1 2 3 4 5 6 7 |
$urlFormid=$this->getRequest()->getParams('parametername'); $get_FormId=$urlFormid['parametername']; $get_FormsData = Mage::getModel('customforms/customforms')->load($get_FormId); var_dump($get_FormId); $get_TextureData = Mage::getModel('customforms/texture')->getCollection(); $get_TextureData->getSelect()->where("customform_id='".$get_FormId."'")->order(array('sort_order ASC','texture_id DESC')); |
Wohlgemerkt: dies ist die vom Hersteller verbesserte Version des Quellcodes nach der Meldung der Sicherheitslücke, die zwar direkte SQL-Abfragen durch die SQL-Models des Zend Frameworks ersetzt, aber die Sicherheitslücke nicht schließt.
(Die generell unterirdisch schlechte Qualität des verwendeten Codes sowie die Tatsache, dass hier SQL-Abfragen in einer Template-Datei ausgeführt werden, sind nicht Thema dieses Beitrags, daher gehe ich hier darauf nicht näher ein.)
Schließen der Lücke
Die einfachste und von mir empfohlene Methode zum Beheben des Problems ist das Entfernen des Moduls. Sollte das nicht möglich sein, lässt sich die Sicherheitslücke dennoch einfach schließen. Man ersetze Zeile 2 des obigen Codeabschnitts (bei dem ich den Namen des Parameters ersetzt habe, um das Ausnutzen der Lücke nicht zu einfach zu machen) durch folgenden Code:
Vorher:
|
1 |
$get_FormId=$urlFormid['parametername']; |
Nachher:
|
1 |
$get_FormId=intval($urlFormid['parametername']); |
Damit ist zwar die Sicherheitslücke geschlossen, die generell schlechte Qualität des Moduls bleibt aber. Ich rate daher dringend vom Einsatz dieses Moduls ab.
Autor: Andreas von Studnitz
Andreas von Studnitz ist Diplom-Informatiker, Magento-Entwickler und Geschäftsführer von integer_net. Seine Schwerpunkte sind Entwicklung, Beratung und die Durchführung von Schulungen. Er ist Magento 2 Certified Professional Developer Plus und hat darüber hinaus weitere Magento Zertifizierungen für Magento 1 und Magento 2. Sowohl im Jahr 2019 als auch 2020 wurde Andreas als Magento Master in der Kategorie „Mentor“ ausgezeichnet.
Danke für die Meldung des Bugs. Kenne das Modul nicht komplett, aber es sieht sehr wirr aus was man da oben sieht.. und SQL Queries in Templates? Bitte, was?!