[geeklog-cvs] geeklog: Bad design decision: COM_checkList would use the table ...

[geeklog-cvs at lists.geeklog.net]

details:   http://project.geeklog.net/cgi-bin/hgweb.cgi/rev/41c7d6494b98
changeset: 6962:41c7d6494b98
user:      Dirk Haun <dirk at haun-online.de>
date:      Sat Apr 18 23:37:42 2009 +0200
description:
Bad design decision: COM_checkList would use the table name in the HTML. Added a new parameter for the name to use instead.

diffstat:

3 files changed, 34 insertions(+), 24 deletions(-)
public_html/docs/history     |    3 +++
public_html/lib-common.php   |   30 ++++++++++++++++++------------
public_html/usersettings.php |   25 +++++++++++++------------

diffs (119 lines):

diff -r 1f91c5b92808 -r 41c7d6494b98 public_html/docs/history
--- a/public_html/docs/history	Sat Apr 18 22:26:05 2009 +0200
+++ b/public_html/docs/history	Sat Apr 18 23:37:42 2009 +0200
@@ -11,6 +11,9 @@
 + Comment moderation and editable comments, by Jared Wenerd
 
 Other changes:
+- COM_checkList would use the table name for the name of the checkbox array in
+  the HTML(!) - added a new parameter for the name (pointed out by Bookoo in
+  the exploit for usersettings.php, cf. Geeklog 1.5.2sr4) [Dirk]
 - Fixed wrong use of COM_allowedHTML and COM_checkHTML in plugins: Functions
   were called without specific permissions, so they defaulted to 'story.edit'.
   I.e. as a Story Admin, you could use the admin_html set in events, but as a
diff -r 1f91c5b92808 -r 41c7d6494b98 public_html/lib-common.php
--- a/public_html/lib-common.php	Sat Apr 18 22:26:05 2009 +0200
+++ b/public_html/lib-common.php	Sat Apr 18 23:37:42 2009 +0200
@@ -88,7 +88,7 @@
 * Configuration Include:
 * You do NOT need to modify anything here any more!
 */
-require_once 'siteconfig.php' ;
+require_once 'siteconfig.php';
 
 /**
 * Configuration class
@@ -1703,16 +1703,16 @@
 *
 * Creates a group of checkbox form fields with given arguments
 *
-* @param        string      $table      DB Table to pull data from
-* @param        string      $selection  Comma delimited list of fields to pull from table
-* @param        string      $where      Where clause of SQL statement
-* @param        string      $selected   Value to set to CHECKED
-* @see function COM_optionList
-* @return   string  HTML with Checkbox code
-*
-*/
-
-function COM_checkList( $table, $selection, $where='', $selected='' )
+* @param    string  $table      DB Table to pull data from
+* @param    string  $selection  Comma delimited list of fields to pull from table
+* @param    string  $where      Where clause of SQL statement
+* @param    string  $selected   Value to set to CHECKED
+* @param    string  $fieldname  Name to use for the checkbox array
+* @return   string              HTML with Checkbox code
+* @see      COM_optionList
+*
+*/
+function COM_checkList($table, $selection, $where = '', $selected = '', $fieldname = '')
 {
     global $_TABLES, $_COM_VERBOSE;
 
@@ -1755,9 +1755,15 @@
             $access = false;
         }
 
+        if (empty($fieldname)) {
+            // Not a good idea, as that will expose our table name and prefix!
+            // Make sure you pass a distinct field name!
+            $fieldname = $table;
+        }
+
         if( $access )
         {
-            $retval .= '<li><input type="checkbox" name="' . $table . '[]" value="' . $A[0] . '"';
+            $retval .= '<li><input type="checkbox" name="' . $fieldname . '[]" value="' . $A[0] . '"';
 
             $sizeS = sizeof( $S );
             for( $x = 0; $x < $sizeS; $x++ )
diff -r 1f91c5b92808 -r 41c7d6494b98 public_html/usersettings.php
--- a/public_html/usersettings.php	Sat Apr 18 22:26:05 2009 +0200
+++ b/public_html/usersettings.php	Sat Apr 18 23:37:42 2009 +0200
@@ -590,7 +590,8 @@
     // excluded items block
     $permissions = COM_getPermSQL ('');
     $preferences->set_var ('exclude_topic_checklist',
-        COM_checkList($_TABLES['topics'],'tid,topic',$permissions,$A['tids']));
+        COM_checkList($_TABLES['topics'], 'tid,topic', $permissions, $A['tids'],
+                      'topics'));
 
     if (($_CONF['contributedbyline'] == 1) &&
         ($_CONF['hide_author_exclusion'] == 0)) {
@@ -639,13 +640,13 @@
         } elseif ($user_etids == '-') { // this means "no topics"
             $user_etids = '';
         }
-        $tmp = COM_checkList ($_TABLES['topics'], 'tid,topic', $permissions,
-                              $user_etids);
-        $preferences->set_var ('email_topic_checklist',
-                str_replace ($_TABLES['topics'], 'etids', $tmp));
-        $preferences->parse ('digest_block', 'digest', true);
+        $tmp = COM_checkList($_TABLES['topics'], 'tid,topic', $permissions,
+                             $user_etids, 'topics');
+        $preferences->set_var('email_topic_checklist',
+                str_replace($_TABLES['topics'], 'etids', $tmp));
+        $preferences->parse('digest_block', 'digest', true);
     } else {
-        $preferences->set_var ('digest_block', '');
+        $preferences->set_var('digest_block', '');
     }
 
     // boxes block
@@ -1372,11 +1373,11 @@
         }
     }
 
-    $TIDS  = @array_values($A[$_TABLES['topics']]);     // array of strings
-    $AIDS  = @array_values($A['selauthors']);           // array of integers
-    $BOXES = @array_values($A["{$_TABLES['blocks']}"]); // array of integers
-    $ETIDS = @array_values($A['etids']);                // array of strings
-    $AETIDS = USER_getAllowedTopics();                  // array of strings (fetched, needed to "clean" $TIDS and $ETIDS)
+    $TIDS  = @array_values($A['topics']);       // array of strings
+    $AIDS  = @array_values($A['selauthors']);   // array of integers
+    $BOXES = @array_values($A['blocks']);       // array of integers
+    $ETIDS = @array_values($A['etids']);        // array of strings
+    $AETIDS = USER_getAllowedTopics();          // array of strings (fetched, needed to "clean" $TIDS and $ETIDS)
 
     $tids = '';
     if (sizeof ($TIDS) > 0) {