CSRF-Token Implementation nicht schlüssig

[noRiddle]

Bitte dazu meinen Thread im Experten-Forum beachten: ​CSRF-Token Implementation nicht schlüssig

Wenn man sich den Code in der /inc/csrf_token.inc.php anschaut, so wird bei aufgerufenen Filenames die sich im Array $module_exclusions und somit im Array $exclusions befinden die Variable $CSRFKeep auf true gesetzt. Es wird jedoch nirgends abgefragt was passieren soll wenn sie auf true steht.
Wenn man nun in einem Script nicht die Funktion xtc_draw_form() benutzt wird der Token ja nicht in einem hidden-field implementiert und es trifft dann der Teil des else in folgendem Code zu
(nämlich, daß $_POST[$_SESSIONCSRFName] nicht isset ist).
Dasselbe gilt wenn man Ajax-Post benutzt und den Token nicht mit postet weil man keinen form-tag benutzt:

if (is_array($_POST) && count($_POST) > 0) {
  if (isset($_POST[$_SESSION['CSRFName']])) {
    if ($_POST[$_SESSION['CSRFName']] != $_SESSION['CSRFToken']) {
      trigger_error("CSRFToken manipulation.\n".print_r($_POST, true), E_USER_WARNING);
      unset($_POST);
      unset($_GET['action']);
      unset($_GET['saction']);
      
      // create CSRF Token
      $_SESSION['CSRFName'] = xtc_RandomString(6);
      $_SESSION['CSRFToken'] = xtc_RandomString(32);
      if (defined('RUN_MODE_ADMIN')) {
        $messageStack->add(CSRF_TOKEN_MANIPULATION, 'warning');
        $messageStack->add_session(CSRF_TOKEN_MANIPULATION, 'warning');
      }
    }
  } else {
    trigger_error("CSRFToken not defined.\n".print_r($_POST, true), E_USER_WARNING);
    unset($_POST);
    unset($_GET['action']);
    unset($_GET['saction']);
    
    // create CSRF Token
    $_SESSION['CSRFName'] = xtc_RandomString(6);
    $_SESSION['CSRFToken'] = xtc_RandomString(32);
    if (defined('RUN_MODE_ADMIN')) {
      $messageStack->add(CSRF_TOKEN_NOT_DEFINED, 'warning');
      $messageStack->add_session(CSRF_TOKEN_NOT_DEFINED, 'warning');
    }
  }
}

Man müsste das } else { imho ersetzen durch

  } else if($CSRFKeep !== true) { //must ask for $CSRFKeep !== true to really exclude files where xtc_draw_form() is not used, noRiddle
    trigger_error("CSRFToken not defined.\n".print_r($_POST, true), E_USER_WARNING);
    .....

Denn hiermit schließt man erst die Filenames aus dem Array $exclusions aus, im Falle $_POST[$_SESSIONCSRFName] nicht isset ist.

Wahrscheinlich muß noch mehr gemacht werden, denn, wie bereits gesagt wird nirgends abgefragt ob $CSRFKeep auf true gesetzt ist, lediglich indirekt mittels

} elseif ($CSRFKeep === false) {

was als elseif für

if (is_array($_POST) && count($_POST) > 0) {

folgt, was immer zutrifft wenn man etwas per Post absendet.

Es sollte also wohl auch das

if (isset($_POST[$_SESSION['CSRFName']])) {

noch ersetzt werden durch:

if (isset($_POST[$_SESSION['CSRFName']]) && $CSRFKeep === false) {

Der ganze Logik-Aufbau der Datei ist imho ein wenig "suspekt", da schwer zu durchschauen ist wan was genau passiert mit den ganzen if elseif else -Konstruktionen, weshalb ich's auch noch nicht ganz durchschaut habe.
Mit den von mir o.g. Änderungen funktioniert's bei mir jedenfalls.

Im Forum finden sich auch einige Posts die beschreiben daß das Erweitern des Array $module_exclusions über auto_include() nicht ausreicht um die Fehlermeldung
"CSRF-Token nicht definiert"
zu umgehen.

Gruß,
noRiddle

[anonymous]

Im bereits zitierten ​Thread im Experten-Forum habe ich noch ein weiteres Problem ergänzt, siehe bitte dort.

Gruß,
noRiddle

[noRiddle]

Ist das hier eigentlich mal nachvollzogen worden ?

Gruß,
noRiddle

[noRiddle]

Ist das hier untergegangen ?
Da es eine Grundfunktion im Backend ist csrf_exclusion setzen zu können, sollte das mal nachvollzogen und gefixt werden.

Gruß,
noRiddle

[Torsten Riemer]

Ja, war in der Tat untergegangen.

[Gerhard Waldemair]

durch das $CSRFKeep = true wird nur ermöglicht, dass der Token mehrfach verwendet werden darf. Was daran nicht schlüssig sein soll, verstehe ich nicht.

[noRiddle]

Vor allem die letzte Ergänzung im oben verlinkten Experten-Forum trifft zu.
Bei Ajax-Requests wo man nicht explizit die CSRF-Token-Daten mitsendet werden ebenfalls die gesetzten $exclusions nicht respektiert.

Allein wenn man sich die if-else-Konstruktionen ansieht muß man doch stutzen, denn es gibt keine offensichtliche logische Verbindung zwischen z.B. hier dem if und dem elseif

if (is_array($_POST) && count($_POST) > 0) {
    ...
} elseif ($CSRFKeep === false) {
    ...
}

$_POST kann gesetzt und größer 0 sein und außerdem auch $CSRFKeep === false sein (weil ja eingangs des Files gesetzt), wieso also elseif ?

Genauso ist diese if und else Konstruktion nicht schlüssig

if (isset($_POST[$_SESSION['CSRFName']])) {
    ...
} else {
    ...
}

Im else müsste noch stehen

} else {
    if($CSRFKeep !== true) {
        ....
    }
}

weil ansonsten nur weil $_POST[$_SESSION[CSRFName]] nicht gesetzt ist ein Token-Error ausgegeben wird.
Bei Ajax-Calls oder bei Nicht-Verwendung von xtc_draw_form() ist aber nun mal $_POST[$_SESSION[CSRFName]] nicht gesetzt.

Kurz:
Probiere es doch einfach aus.
Bei mir wirken die $exclusions bei allen Tests nicht wo $_POST[$_SESSION[CSRFName]] nicht gesendet wird, weil entweder xtc_draw_form() nicht benutzt wurde oder weil die Daten des Tokens in einem Ajax-POST nicht mitgesendet wurden. Das wäre aber ja Sinn der $exclusions.
Das ist auch nach dem Code völlig klar, weil nämlich nie geprüft wird ob $CSRFKeep == true ist und somit greifen die $exclusions nie.

Gruß,
noRiddle

*NACHTRAG*
Nach meiner Anpassung in dieser Form, siehe Kommentare mit noRiddle im Code, funktioniert alles wie es soll:
(Eventuell kann man das noch logischer aufbauen und vereinfachen.)

<?php
/* -----------------------------------------------------------------------------------------
   $Id: csrf_token.inc.php 10396 2016-11-07 13:20:51Z GTB $

   modified eCommerce Shopsoftware - community made shopping
   http://www.modified-shop.org

   Copyright (c) 2009 - 2012 modified eCommerce Shopsoftware
   -----------------------------------------------------------------------------------------
   Released under the GNU General Public License
   ---------------------------------------------------------------------------------------*/

// include needed function
require_once (DIR_FS_INC . 'xtc_create_password.inc.php');

if (defined('CSRF_TOKEN_EXCLUSIONS') && CSRF_TOKEN_EXCLUSIONS != '') {
  $user_exclusions  = preg_replace("'[\r\n\s]+'", '', CSRF_TOKEN_EXCLUSIONS);
  $user_exclusions = explode(',', $user_exclusions);
}

if (!isset($module_exclusions) || !is_array($module_exclusions)) {
  $module_exclusions = array();
}

// keep Token for popups, user_exclusions, module_exclusions
$CSRFKeep = false;
if (defined('RUN_MODE_ADMIN')) {
  foreach(auto_include(DIR_FS_ADMIN.'includes/extra/csrf_exclusion/','php') as $file) require_once ($file);
  
  $exclusions = array(
    'bill', 
    'haendlerbund', 
    'magnalister', 
    'new_attributes', 
    'popup', 
    'popup_memo',
    'print_order', 
    'print_packingslip', 
    'products_tags', 
    'validproducts', 
    'validcategories',
  );
  if (isset($user_exclusions) && is_array($user_exclusions)) {
    $exclusions = array_merge($exclusions, $user_exclusions);
  }
  if (isset($module_exclusions) && is_array($module_exclusions)) {
    $exclusions = array_merge($exclusions, $module_exclusions);
  }
  foreach ($exclusions as $filename) {
    if (strpos(basename($PHP_SELF), $filename) !== false) {
      $CSRFKeep = true;
    }
  }

  //BOC unset CSFR session vars for $exclusions files, otherwise when coming from another backend page CSRF session is already set and post values will be set because of xtc_draw_form(), noRiddle
  if($CSRFKeep === true && isset($_SESSION['CSRFName']) && isset($_SESSION['CSRFToken'])) {
    unset($_SESSION['CSRFName']);
    unset($_SESSION['CSRFToken']);
  }
  //EOC unset CSFR session vars for $exclusions files, noRiddle
}

// verfiy CSRF Token
if (is_array($_POST) && count($_POST) > 0) {
  if (isset($_POST[$_SESSION['CSRFName']])) {
    if ($_POST[$_SESSION['CSRFName']] != $_SESSION['CSRFToken']) {
      trigger_error("CSRFToken manipulation.\n".print_r($_POST, true), E_USER_WARNING);
      unset($_POST);
      unset($_GET['action']);
      unset($_GET['saction']);
      
      // create CSRF Token
      $_SESSION['CSRFName'] = xtc_RandomString(6);
      $_SESSION['CSRFToken'] = xtc_RandomString(32);
      if (defined('RUN_MODE_ADMIN')) {
        $messageStack->add(CSRF_TOKEN_MANIPULATION, 'warning');
        $messageStack->add_session(CSRF_TOKEN_MANIPULATION, 'warning');
      }
    }
  } else {
    if($CSRFKeep !== true) { //must ask for $CSRFKeep !== true to really exclude files where xtc_draw_form() is not used, noRiddle
        trigger_error("CSRFToken not defined.\n".print_r($_POST, true), E_USER_WARNING);
        unset($_POST);
        unset($_GET['action']);
        unset($_GET['saction']);
        
        // create CSRF Token
        $_SESSION['CSRFName'] = xtc_RandomString(6);
        $_SESSION['CSRFToken'] = xtc_RandomString(32);
        if (defined('RUN_MODE_ADMIN')) {
          $messageStack->add(CSRF_TOKEN_NOT_DEFINED, 'warning');
          $messageStack->add_session(CSRF_TOKEN_NOT_DEFINED, 'warning');
        }
    }
  }
} elseif ($CSRFKeep === false) {
  $_SESSION['CSRFName'] = xtc_RandomString(6);
  $_SESSION['CSRFToken'] = xtc_RandomString(32);
}
?>

[Gerhard Waldemair]

Mit deiner Version getestet:
-> Artikel bearbeiten
-> Attribute bearbeiten und speichern
-> Artikel speichern

--> CSRF Token nicht definiert

[Gerhard Waldemair]

  if($CSRFKeep === true && isset($_SESSION['CSRFName']) && isset($_SESSION['CSRFToken'])) {
    unset($_SESSION['CSRFName']);
    unset($_SESSION['CSRFToken']);
  }

darf nicht gemacht werden.

[noRiddle]

Genau in der Reihenfolge gemacht, in der Tat.
Ohne den von dir zitierten Code aber mit der zweiten Änderung in meinem Vorschlag geht es, auch das von mir monierte was vorher nicht ging.

Mir stellt sich die Frage was du allgemein zu meinen Ausführungen sagst, abgesehen von meinem fehlerhaften Vorschlag.
"Bei Ajax-Calls oder bei Nicht-Verwendung von xtc_draw_form()..."
Bei Ajax-Calls ohne Mitsenden der CSRF-Daten und bei Nicht-Verwendung von xtc_draw_form() weil $_POST[$_SESSION[CSRFName]] dann nicht gesetzt ist.

Gruß,
noRiddle

[Gerhard Waldemair]

Kannst du bitte mal meine letzte Version testen ?

Das berücksichtigt deinen Änderungsvorschlag.

Zudem wird der Token immer erneuert wenn kein Fehler auftritt oder $CSRFKeep nicht auf true steht. Ansonsten wird ein Token mehrfach verwendet wie zB beim Anlegen oder Bearbeiten eines Coupons. Dort erfolgt keine Weiterleitung nach dem Post.

<?php
/* -----------------------------------------------------------------------------------------
   $Id: csrf_token.inc.php 12910 2020-09-28 13:48:11Z GTB $

   modified eCommerce Shopsoftware - community made shopping
   http://www.modified-shop.org

   Copyright (c) 2009 - 2012 modified eCommerce Shopsoftware
   -----------------------------------------------------------------------------------------
   Released under the GNU General Public License
   ---------------------------------------------------------------------------------------*/

// include needed function
require_once (DIR_FS_INC . 'xtc_create_password.inc.php');

if (defined('CSRF_TOKEN_EXCLUSIONS') && CSRF_TOKEN_EXCLUSIONS != '') {
  $user_exclusions  = preg_replace("'[\r\n\s]+'", '', CSRF_TOKEN_EXCLUSIONS);
  $user_exclusions = explode(',', $user_exclusions);
}

if (!isset($module_exclusions) || !is_array($module_exclusions)) {
  $module_exclusions = array();
}

// keep Token for popups, user_exclusions, module_exclusions
$CSRFKeep = false;
if (defined('RUN_MODE_ADMIN')) {
  foreach(auto_include(DIR_FS_ADMIN.'includes/extra/csrf_exclusion/','php') as $file) require_once ($file);
  
  $exclusions = array(
    'bill', 
    'haendlerbund', 
    'magnalister', 
    'popup', 
    'popup_memo',
    'print_order', 
    'print_packingslip', 
    'products_attributes', 
    'products_tags', 
    'validproducts', 
    'validcategories',
  );
  if (isset($user_exclusions) && is_array($user_exclusions)) {
    $exclusions = array_merge($exclusions, $user_exclusions);
  }
  if (isset($module_exclusions) && is_array($module_exclusions)) {
    $exclusions = array_merge($exclusions, $module_exclusions);
  }
  foreach ($exclusions as $filename) {
    if (strpos(basename($PHP_SELF), $filename) !== false) {
      $CSRFKeep = true;
    }
  }
}

// verfiy CSRF Token
$error = false;
if (is_array($_POST) && count($_POST) > 0) {
  if (isset($_POST[$_SESSION['CSRFName']])) {
    if ($_POST[$_SESSION['CSRFName']] != $_SESSION['CSRFToken']) {
      $error = CSRF_TOKEN_MANIPULATION;
    }
  } elseif ($CSRFKeep !== true) {
    $error = CSRF_TOKEN_NOT_DEFINED;
  }
  
  if ($error !== false) {
    trigger_error($error."\n".print_r($_POST, true), E_USER_WARNING);
    unset($_POST);
    unset($_GET['action']);
    unset($_GET['saction']);
    
    if (defined('RUN_MODE_ADMIN')) {
      $messageStack->add($error, 'warning');
      $messageStack->add_session($error, 'warning');
    }
  }
}

if ($CSRFKeep === false || $error !== false) {
  // create CSRF Token
  $_SESSION['CSRFName'] = xtc_RandomString(6);
  $_SESSION['CSRFToken'] = xtc_RandomString(32);
}
?>

[anonymous]

Mit deinem Vorschlag geht jetzt bei mir dein eigene Test nicht, also
-> Artikel bearbeiten
-> Attribute bearbeiten und speichern
-> Artikel speichern

--> CSRF Token nicht definiert

Was ich moniert habe, also z.B. Ajax-Post ohne xtc_dra_form() geht.

Übrigens finde ich es auch nervend - das ist auch in der Original-Version aus 2.0.5.1 so - daß wenn man den CSRF-Error einmal angezeigt bekommt ein Neuaufruf der Seite nicht ausreicht, man muß zweimal neu aufrufen oder Seite wechseln und zurück, damit die Meldung verschwindet.

Gruß,
noRiddle

[anonymous]

Du musst in dem exclude array noch new_attributes hinzufügen. Im Trunk gibt es das nicht mehr. Da läuft das über die product_attributes.

Die Meldung kommt doppelt, da nicht klar ist, ob ein Redirect noch stattfindet. Deshalb schreiben wir es in beide Meldungen.

[Gerhard Waldemair]

Ich musste nochmals eine Korrektur vornehmen, ansonsten funktionieren Ajax Calls auf Seiten mit einer Form nicht mehr.

<?php
/* -----------------------------------------------------------------------------------------
   $Id: csrf_token.inc.php 12910 2020-09-28 13:48:11Z GTB $

   modified eCommerce Shopsoftware - community made shopping
   http://www.modified-shop.org

   Copyright (c) 2009 - 2012 modified eCommerce Shopsoftware
   -----------------------------------------------------------------------------------------
   Released under the GNU General Public License
   ---------------------------------------------------------------------------------------*/

// include needed function
require_once (DIR_FS_INC . 'xtc_create_password.inc.php');

if (defined('CSRF_TOKEN_EXCLUSIONS') && CSRF_TOKEN_EXCLUSIONS != '') {
  $user_exclusions  = preg_replace("'[\r\n\s]+'", '', CSRF_TOKEN_EXCLUSIONS);
  $user_exclusions = explode(',', $user_exclusions);
}

if (!isset($module_exclusions) || !is_array($module_exclusions)) {
  $module_exclusions = array();
}

// keep Token for popups, user_exclusions, module_exclusions
$CSRFKeep = false;
if (defined('RUN_MODE_ADMIN')) {
  foreach(auto_include(DIR_FS_ADMIN.'includes/extra/csrf_exclusion/','php') as $file) require_once ($file);
  
  $exclusions = array(
    'bill', 
    'haendlerbund', 
    'magnalister', 
    'new_attributes', // gets removed in 2.0.6.0 
    'popup', 
    'popup_memo',
    'print_order', 
    'print_packingslip', 
    'products_attributes', 
    'products_tags', 
    'validproducts', 
    'validcategories',
  );
  if (isset($user_exclusions) && is_array($user_exclusions)) {
    $exclusions = array_merge($exclusions, $user_exclusions);
  }
  if (isset($module_exclusions) && is_array($module_exclusions)) {
    $exclusions = array_merge($exclusions, $module_exclusions);
  }
  foreach ($exclusions as $filename) {
    if (strpos(basename($PHP_SELF), $filename) !== false) {
      $CSRFKeep = true;
    }
  }
}

// verfiy CSRF Token
if (is_array($_POST) && count($_POST) > 0) {
  $error = false;
  if (isset($_POST[$_SESSION['CSRFName']])) {
    if ($_POST[$_SESSION['CSRFName']] != $_SESSION['CSRFToken']) {
      $error = CSRF_TOKEN_MANIPULATION;
    }
  } elseif ($CSRFKeep !== true) {
    $error = CSRF_TOKEN_NOT_DEFINED;
  }
  
  if ($error !== false) {
    trigger_error($error."\n".print_r($_POST, true), E_USER_WARNING);
    unset($_POST);
    unset($_GET['action']);
    unset($_GET['saction']);
    
    // create CSRF Token
    $_SESSION['CSRFName'] = xtc_RandomString(6);
    $_SESSION['CSRFToken'] = xtc_RandomString(32);

    if (defined('RUN_MODE_ADMIN')) {
      $messageStack->add($error, 'warning');
    }
  }
} elseif ($CSRFKeep === false) {
  // create CSRF Token
  $_SESSION['CSRFName'] = xtc_RandomString(6);
  $_SESSION['CSRFToken'] = xtc_RandomString(32);
}
?>

[anonymous]

Das sieht gut aus.

Gruß,
noRiddle

[Gerhard Waldemair]

In 13461:

fix #1183 - fix CSRF Token

[Gerhard Waldemair]

Danke. Ist so im SVN.