Sicherheitspatch für alle Shopversionen (security_fix_2014_12_17.zip)

[andrewx]

Vielen Dank allen für den unermüdlichen Einsatz!

[karl]

Dank auch von mir!

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

[fishnet]

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 ?

[web0null]

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ß

[web0null]

...soll natürlich "Ursprung" heißen.

[webald]

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

[web0null]

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ß

[peterdd]

... 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.

[noRiddle (revilonetz)]

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

[web0null]

Lieber noRiddle,

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,

$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ß

[MW]

Danke an fishnet, noRiddle 

und web0null das du mit gegrübelt hast 

[Tomcraft]

Ich habe soeben Beiträge aus dem Thema gelöscht, die quasi eine Anleitung zur Ausnutzung des Fehlers liefern.

Grüße

Torsten

[innuXTC]

Danke Torsten, ... dann sind wir ja einer Meinung

[h-h-h]

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

[noRiddle (revilonetz)]

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. 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