Neuigkeiten
  • Die modified eCommerce Shopsoftware ist kostenlos, aber nicht umsonst.
  • Damit wir die modified eCommerce Shopsoftware auch zukünftig kostenlos anbieten können:

Autor Thema: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)  (Gelesen 51270 mal)

Offline andrewx

  • Fördermitglied
  • *****
  • Beiträge: 12
  • Geschlecht: Männlich
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #15 am: 18. Dezember 2014, 09:53:13 »
Vielen Dank allen für den unermüdlichen Einsatz!

Offline karl

  • Schreiberling
  • ****
  • Beiträge: 439
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #16 am: 18. Dezember 2014, 14:19:21 »
Dank auch von mir!

Und wer hat in der anderen Streitfrage nun Recht? noRiddle oder h-h-h ?

Offline fishnet

  • Fördermitglied
  • *****
  • Beiträge: 4.641
  • Geschlecht: Männlich
    • Teile Beitrag
    • Fishnet Services
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #17 am: 18. Dezember 2014, 14:30:28 »
Das ist letztendlich egal, weil du die Lösung vom Modified Team (Post 1) nehmen solltest, die zwar nicht die performanteste aber die sicherste (doppelt gemoppelt) ist.
Ich habe auch noch eine Frage, ab wann wird sich der Fix auch im Download des Shops finden ?

Offline web0null

  • Experte
  • *****
  • Beiträge: 1.998
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #18 am: 18. Dezember 2014, 14:38:11 »
Ich habe mir das jetzt nicht "ganz genau" angesehen, aber mir stellt sich eigentlich die Frage, bzw. Überlegung,
... sollte man nicht einfach dafür sorgen dass $_SESSION['customer_id'] richtig bzw. überhaupt gefüllt wird, damit man nicht später solche Verrenkungen machen muss.

Ich meine fishnet´s Lösung ist sicher nicht verkehrt, aber das "Problem" sollte doch am Uhrsprung gelöst werden.

Oder habe ich da etwas falsch verstanden?
Gruß

Offline web0null

  • Experte
  • *****
  • Beiträge: 1.998
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #19 am: 18. Dezember 2014, 15:29:05 »
...soll natürlich "Ursprung" heißen.

Offline webald

  • modified Team
  • *****
  • Beiträge: 2.712
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #20 am: 18. Dezember 2014, 15:41:21 »
Deshalb hatte ich im anderen Post auch geschrieben, dass die Admingruppe im Standard nicht 0 sonder etwas anderes etwa -1 sein sollte.

Offline web0null

  • Experte
  • *****
  • Beiträge: 1.998
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #21 am: 18. Dezember 2014, 15:51:25 »
Ja ich weiß was du meinst, hat aber nur indirekt etwas damit zu tun.
Fakt ist, dass die $_SESSION['customer_id'] falsch gesetzt wird, und das ist dass Problem.
Denn das falsche befüllen bzw. setzen der $_SESSION['customer_id'] ist der Ursprung des Fehlers, und nur da sollte man eingreifen.
Alles andere ist nur "vorhandene Fehler im Nachhinein wieder umzubiegen".

Aber ich "glaube" schon zu wissen warum die Umsetzung so erfolgt ist, wie sie im "FIX" ist.  ;-)

Gruß

Offline pl1

  • Neu im Forum
  • *
  • Beiträge: 31
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #22 am: 18. Dezember 2014, 17:21:49 »
... sollte man nicht einfach dafür sorgen dass $_SESSION['customer_id'] richtig bzw. überhaupt gefüllt wird, damit man nicht später solche Verrenkungen machen muss.

Ja, und ließe sich nicht die gesamte includes/write_customers_status.php vereinfachen?

Also sowas in der Art (gedanklich, ungetestet!)
Code: PHP  [Auswählen]
if(isset($_SESSION['customer_id'])) {
  $customers_status_query = xtc_db_query("SELECT cs.* FROM ".TABLE_CUSTOMERS." c
    JOIN "
.TABLE_CUSTOMERS_STATUS." cs
    ON cs.customers_status_id=c.customers_status
    WHERE c.customers_id = '"
.$_SESSION['customer_id']."'
    AND cs.language_id = '"
.$_SESSION['languages_id']);
  if(xtc_db_num_rows($customers_status_query_1) > 0) {
    $_SESSION['customers_status']= xtc_db_fetch_array($customers_status_query);
  }else{
    unset($_SESSION['customer_id']);
    xtc_redirect(xtc_href_link(FILENAME_LOGOFF));
  }
}else{
  $customers_status_query = xtc_db_query("
    SELECT * FROM "
.TABLE_CUSTOMERS_STATUS."
    WHERE customers_status_id = '"
.DEFAULT_CUSTOMERS_STATUS_ID_GUEST."'
    AND language_id = '"
. $_SESSION['languages_id'] . "'");
  $_SESSION['customers_status']= xtc_db_fetch_array($customers_status_query);
}

Der Dateiname "write_customers_status" suggeriert etwas anderes als dann in der Datei wirklich gemacht wird. Es werden ja nur die passenden Werte aus der Datenbanktabelle customers_status in die $SESSION mit geladen.

Und gerade sehe ich, dass includes/write_customers_status.php ähnlich ist mit
inc/xtc_get_customer_status_value.inc.php

Dort wird in die $_SESSION geschrieben
Code: PHP  [Auswählen]
function xtc_get_customer_status_value($customer_id) {
....
# XXX Setzt! Sessionstatus des aktuellen Nutzers auf den Status des angefragen Nutzer mit customer_id! Unerwartetes Verhalten IMHO
$_SESSION['customer_status_value'] = $customer_status_value;
return $customer_status_value;
}
Allerdings scheint das toter Code zu sein, denn xtc_get_customer_status_value wird nirgends aufgerufen
Code: PHP  [Auswählen]
grep -r xtc_get_customer_status_value
xtc_get_customer_status fände ich passender als xtc_get_customer_status_value. Diese Funktion gibt es im admin-Bereich in der admin/includes/general.php

Man beachte aber den Unterschied zwischen JOIN und LEFT JOIN.

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.989
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #23 am: 18. Dezember 2014, 18:10:19 »
Dank auch von mir!

Und wer hat in der anderen Streitfrage nun Recht? noRiddle oder h-h-h ?

Das haben h-h-h und meine Wenigkeit doch geklärt und ich habe es hier ausführlich erklärt.
Ich hatte Recht, allerdings mit der verkehrten Begründung, da $_SESSION['customers_status']['customers_status_id'] ohne den Fix auf die Konstante NULL gesetzt wurde und nicht auf Boolean 0 (== false).

Im übrigen hat fishnet Recht mit seiner Antwort, es ist egal da der hier vorgestellte Fix an der Wurzel angreift.

Ich habe mir das jetzt nicht "ganz genau" angesehen, aber mir stellt sich eigentlich die Frage, bzw. Überlegung,
... sollte man nicht einfach dafür sorgen dass $_SESSION['customer_id'] richtig bzw. überhaupt gefüllt wird, damit man nicht später solche Verrenkungen machen muss.

Ich meine fishnet´s Lösung ist sicher nicht verkehrt, aber das "Problem" sollte doch am Uhrsprung gelöst werden.

Oder habe ich da etwas falsch verstanden?
Gruß
Lieber web0null.
  • ist das Problem ja am Ursprung/an der Wurzel gepackt indem die
    $_SESSION['customers_status']['customers_status_id']
    durch den Fix nicht mehr auf 0, false oder NULL gesetzt wird wenn die Query in der /includes/write_customers_status.php kein Result liefert
    und
  • geht es nicht um die $_SESSION['customer_id'] (= customers_id aus der Tabelle customers)
    - die ist nämlich immer gesetzt sobald jemand eingeloggt ist -
    sondern um die
    $_SESSION['customers_status']['customers_status_id'] (= customers_status aus der Tabelle customers)
    die in besagter Datei gefüllt bzw. definiert wird.

Deshalb hatte ich im anderen Post auch geschrieben, dass die Admingruppe im Standard nicht 0 sonder etwas anderes etwa -1 sein sollte.

In der Tat wäre es von vornherein klüger für den customers_status keine Werte zu benutzen die als Boolean mißverstanden werden könnten, also weder 1 noch 0 (vielleicht gar überhaupt keine numerischen Werte), damit man beim Codieren nicht ständig aufpassen muß.
Da das allerdings noch eine sog. Altlast ist müsste man zuviel Code anfassen wollte man das ändern.
Man sollte halt immer so programmieren, daß man an der Wurzel Fehler verhindert und das ist mit dem vorliegenden Fix geschehen.
Niemand hat geahnt, daß ein leeres Result aus einer Query die mittels mysql_fetch_array() in ein Array geschrieben wird einen Wert von NULL ausgibt wenn er in eine Session geschrieben wird.
Ich hätte gedacht, daß es false ergeben müsste, da das PHP-Manual sagt, daß mysql_fetch_array() false ausgibt wenn keine Row gefunden wurde. Im Manual steht zwar auch dies:
Zitat:
"Diese Funktion setzt NULL-Felder auf den PHP Wert-NULL."
aber was ist ein NULL-Feld ?, ich verstehe es so, daß es ein Feld ist dessen Inhalt NULL ist, wenn man jedoch gar kein Result hat weil es keinen Eintrag gibt ?

Gruß,
noRiddle

Offline web0null

  • Experte
  • *****
  • Beiträge: 1.998
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #24 am: 18. Dezember 2014, 18:35:45 »
Lieber noRiddle,
Zitat
ist das Problem ja am Ursprung/an der Wurzel gepackt
Sehe ich anders.
Denn anscheinend ist ja in $_SESSION['customer_id']  "ein Wert" drinnen womit in weiterer folge hiermit,
Zitat
$customers_status_query_1 = xtc_db_query("SELECT customers_status FROM " . TABLE_CUSTOMERS . " WHERE customers_id = '" . $_SESSION['customer_id'] . "'");

nichts raus kommt.

Also ist meiner bescheidenen Meinung nach, die Wurzel, eben das richtige befüllen der $_SESSION['customer_id'].

Denn wenn man dort schon einen Wert hinterlegt der auch in der TABLE_CUSTOMERS zu finden ist, hätte man das Problem eines leeren Resultats nicht.

Eigentlich kann die $_SESSION['customer_id'] nur folgende "Fehler" aufweisen,
  • sie ist gesetzt und leer
  • oder gesetzt und mit der Zahl 0 gefüllt
  • oder gesetzt und mit einem falschen bzw. alten Wert gefüllt

Und alle 3 Möglichkeiten wären falsch und somit schon vor dem setzen von $_SESSION['customer_id'], abzufangen.

Gruß

Offline MW

  • Fördermitglied
  • *****
  • Beiträge: 420
  • Geschlecht: Männlich
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #25 am: 18. Dezember 2014, 19:34:52 »
Danke an fishnet, noRiddle  :thx:

und web0null das du mit gegrübelt hast  ;-)

Offline Tomcraft

  • modified Team
  • *****
  • Gravatar
  • Beiträge: 43.635
  • Geschlecht: Männlich
    • Teile Beitrag
    • https://www.modified-shop.org
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #26 am: 18. Dezember 2014, 19:48:38 »
Ich habe soeben Beiträge aus dem Thema gelöscht, die quasi eine Anleitung zur Ausnutzung des Fehlers liefern.

Grüße

Torsten

Offline innuXTC

  • Schreiberling
  • ****
  • Beiträge: 478
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #27 am: 18. Dezember 2014, 19:55:42 »
Danke Torsten, ... dann sind wir ja einer Meinung :)

Offline h-h-h

  • modified Team
  • *****
  • Beiträge: 4.564
    • Teile Beitrag
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #28 am: 18. Dezember 2014, 20:31:06 »
Ja, und ließe sich nicht die gesamte includes/write_customers_status.php vereinfachen?

Also sowas in der Art (gedanklich, ungetestet!)
Code: PHP  [Auswählen]
if(isset($_SESSION['customer_id'])) {
  $customers_status_query = xtc_db_query("SELECT cs.* FROM ".TABLE_CUSTOMERS." c
    JOIN "
.TABLE_CUSTOMERS_STATUS." cs
    ON cs.customers_status_id=c.customers_status
    WHERE c.customers_id = '"
.$_SESSION['customer_id']."'
    AND cs.language_id = '"
.$_SESSION['languages_id']);
  if(xtc_db_num_rows($customers_status_query_1) > 0) {
    $_SESSION['customers_status']= xtc_db_fetch_array($customers_status_query);
  }else{
    unset($_SESSION['customer_id']);
    xtc_redirect(xtc_href_link(FILENAME_LOGOFF));
  }
}else{
  $customers_status_query = xtc_db_query("
    SELECT * FROM "
.TABLE_CUSTOMERS_STATUS."
    WHERE customers_status_id = '"
.DEFAULT_CUSTOMERS_STATUS_ID_GUEST."'
    AND language_id = '"
. $_SESSION['languages_id'] . "'");
  $_SESSION['customers_status']= xtc_db_fetch_array($customers_status_query);
}

gefällt mir, gute Idee.

Bei aktivierten Kundengruppenrechten lässt sich die Sicherheitslücke übrigens nicht ausnutzen.

Gruß, h-h-h

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.989
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)
« Antwort #29 am: 18. Dezember 2014, 20:49:21 »
Ich habe soeben Beiträge aus dem Thema gelöscht, die quasi eine Anleitung zur Ausnutzung des Fehlers liefern.

Grüße

Torsten

Jau, jau, sorry. :hust: Dachte das wäre ohnhin bekannt und wenn nicht wäre es aus der Code-Diskussion ableitbar gewesen, zumindest für Leute die sowas ausnutzen wollen.

@h-h-h
Mhh, mit dem Nachteil, daß viele Code-Stellen geändert werden müssten, denn die Session-Variable um die es hier ging heißt ja nun $_SESSION['customers_status']['customers_status_id'] und hieße dann $_SESSION['customers_status']['customers_status'].

Gruß,
noRiddle

Haendlerbund_Leistungen_728x90_animiert

Teile per facebook Teile per linkedin Teile per twitter

 


             
anything