[Security Audit Needed] Simple BB CMS Class

[topdown]

Here is a re-write of my Simple BB CMS mod.
I have made it OOP so it can be easily used with in any phpBB3 page.

I'm still quite rough on security, so if one of the STG Developers could take a peak I would appreciate it.
Please see revised version here New Code

Spoiler:

Code: Select all

<?php
/**
*
* @cmsClass.php (CMS controler)
* @version 0.0.3 Beta
* @package - Simple BB CMS
* @copyright (c) 2008 topdown, Webmasters United.org
* @license http://opensource.org/licenses/gpl-license.php GNU Public License
*
*/

/**
* @ignore
*/
if (!defined('IN_PHPBB'))
{
   exit;
}

/**
* CMS class
* @package Simple BB CMS
*/

class cmsClass
{   
   /**
    * @var INT
    * return forum id
    */
   private $a_forum_id;
   
   /**
    * @var INT
    * return the limit for pagnition
    */
   private $limit;
   
   /**
    * @var BOOL
    * return for display article nav
    */
   private $display_nav;
   
   /**
    * @var BOOL
    * return for link to public topic
    */
   private $comments;
   
   /**
    * @var
    * return order ASC / DESC
    */
   private $order;
   
   /**
    * @var string
    * return the article block title
    */
   private $cms_title;
   
   
   /**
    * Let get the topics needed for the CMS articles.
    * @return
    * @param object $a_forum_id       Sets the forum id to get the topics from
    * @param object $limit            Sets the limit of articles for pagnition
    * @param object $display_nav      Display article navigation true/false
    * @param object $comments         Display a link to the topic for comments true/false,
    *                            REMEMBER the forum must be public for comments.
    * @param object $order            Order the articles by 'ASC'/'DESC'
    * @param object $cms_title         Give your page atitle 'Home Page'
    *
    * cmsClass::get_cms($a_forum_id, $limit, $display_nav, $comments, $order, $cms_title);
    * EXAMPLE    cmsClass::get_cms('2', '6', true, true, 'ASC', 'Home Page');
    */
   public static function get_cms($a_forum_id, $limit, $display_nav, $comments, $order, $cms_title)
    {
      /**
         * Set the needed globals
         */
      global $phpbb_root_path, $phpEx, $db, $template, $user, $auth;
      
      /**
       * @var string
       * request needed vars
       */
      $forum_id   = request_var('f', 0);
      $topic_id   = request_var('t', 0);

      /**
       * @var Starting pagnition for the Articles
       */
      $check_params = array(
          $start   = request_var('start', 0),
         $limit   = request_var('limit', $limit),
      );
      
      /**
       * @var put the params for pagnition in an array
       */
      $params = array();
      
      foreach ($check_params as $key => $default)
      {
          if (!isset($_REQUEST[$key]))
          {
              continue;
          }
         
          $param = call_user_func('request_var', $key, $default);
          $$key = $param;
         
          $params[] = urlencode($key) . '=' . ((is_string($param)) ? urlencode($param) : $param);
      }
      
      $pagination_url = append_sid($phpbb_root_path . '../index.' . $phpEx . implode('&amp;', $params));
      
      /**
       * Build SQL array for the Articles
       * @var $a_forum_id as a function param set to your forum_id
       */
      
      $sql_ary = array(
          'SELECT'       => '(t.topic_id), (p.poster_id), (u.user_id), username, user_colour, post_id, post_username, post_text, post_subject, post_time, bbcode_uid, bbcode_bitfield, enable_bbcode, enable_smilies, enable_magic_url',
         'FROM'         => array(
          POSTS_TABLE      => 'p',
         TOPICS_TABLE   => 't',
         USERS_TABLE      => 'u',
          ),
         'WHERE'         => 'p.post_id = t.topic_first_post_id
            AND p.post_time = t.topic_time
            AND   p.forum_id    = ' . (int)$a_forum_id . '
            AND u.user_id   =  p.poster_id',
         'ORDER_BY'      => "p.topic_id $order",
      );
      
      $sql = $db->sql_build_query('SELECT', $sql_ary);
      $result = $db->sql_query_limit($sql, $limit, $start);
      
                  
      while ($post_data = $db->sql_fetchrow($result))
      {
         $poster_id = $post_data['poster_id'];
         //Set BBC and Smiley options
         $post_data['bbcode_options'] = (($post_data['enable_bbcode']) ? OPTION_FLAG_BBCODE : 0) +
            (($post_data['enable_smilies']) ? OPTION_FLAG_SMILIES : 0) +
            (($post_data['enable_magic_url']) ? OPTION_FLAG_LINKS : 0);
                                                        
         //Check
         $poster_id = $post_data['poster_id'];
         // assign the database results to the block_vars loop
         $template->assign_block_vars('post_data', array(
            'POST_AUTHOR_FULL'   => get_username_string('full', $poster_id, $post_data['username'], $post_data['user_colour'], $post_data['post_username']),
            'POST_TITLE'      => $post_data['post_subject'],
            'ARTICLE_LINK'      => $post_data['topic_id'],
            'POST_TEXT'        => generate_text_for_display($post_data['post_text'], $post_data['bbcode_uid'], $post_data['bbcode_bitfield'], $post_data['bbcode_options']),
            'POST_DATE'         => $user->format_date($post_data['post_time']),
            'U_COMMENTS'      => append_sid("{$phpbb_root_path}viewtopic.$phpEx", "f=$a_forum_id&amp;t={$post_data['topic_id']}"),
               
            /** No big permission deal here because they would need permissions to the forum first
            *  only admins should see these links
            */
            'U_EDIT'         => append_sid("{$phpbb_root_path}posting.$phpEx", "mode=edit&amp;f=$a_forum_id&amp;p={$post_data['post_id']}"),
            'U_DELETE'         => append_sid("{$phpbb_root_path}posting.$phpEx", "mode=delete&amp;f=$a_forum_id&amp;p={$post_data['post_id']}"),
         ));            
      }
      
      // free the result
      $db->sql_freeresult($result);
      
      /**
       * Run the query again with a count to set the pagnition
       */
      $sql_ary['SELECT'] = 'COUNT(p.post_subject) as total_articles';
      $sql = $db->sql_build_query('SELECT', $sql_ary);
      $result = $db->sql_query($sql);
      
      // get the total users, this is a single row, single field.
      $total_articles = $db->sql_fetchfield('total_articles');
      // free the result
      $db->sql_freeresult($result);
      
      // Assign index specific vars
      $template->assign_vars(array(
         'S_CMS_PAGES'      => true,
         'CMS_TITLE'         => $cms_title,
         'PAGINATION'        => generate_pagination($pagination_url, $total_articles, $limit, $start),
          'PAGE_NUMBER'       => on_page($total_articles, $limit, $start),
         'PAGE_NUM'          => $start,
         'S_NAVIGATION'      => $display_nav,
         'S_COMMENTS'      => $comments,
          'TOTAL_ARTICLES'    => ($total_articles == 1) ? $total_articles['total_articles'] : sprintf($total_articles['total_articles'], $total_articles),
      ));
    }      
}

?>

oOOo, To many lines for phpBB3 to highlight

[Handyman]

Bad use of camel case on the class name

Code: Select all

    class cmsClass

all variable, class, function and method names must be all lower case with underscores as needed.

Code: Select all

$param = call_user_func('request_var', $key, $default); 

should be

Code: Select all

$param = request_var($key, $default); 

WARNING!

Code: Select all

'ORDER_BY'      => "p.topic_id $order", 

possible security concern? don't know how $order is coming in, but all strings must use sql_escape, so it should be

Code: Select all

'ORDER_BY'      => 'p.topic_id ' . $db->sql_escape($order), 

[topdown]

Thank You Handyman.
All fixed,
here is the revised code if anyone else has input

Spoiler:

Code: Select all

<?php
/**
*
* @cms_class.php (CMS controler)
* @version 0.0.3 Beta
* @package - Simple BB CMS
* @copyright (c) 2008 topdown, Webmasters United.org
* @license http://opensource.org/licenses/gpl-license.php GNU Public License
*
*/

/**
* @ignore
*/
if (!defined('IN_PHPBB'))
{
   exit;
}

/**
* CMS class
* @package Simple BB CMS
*/

class cms_class
{   
   /**
    * @var INT
    * return forum id
    */
   private $a_forum_id;
   
   /**
    * @var INT
    * return the limit for pagnition
    */
   private $limit;
   
   /**
    * @var BOOL
    * return for display article nav
    */
   private $display_nav;
   
   /**
    * @var BOOL
    * return for link to public topic
    */
   private $comments;
   
   /**
    * @var
    * return order ASC / DESC
    */
   private $order;
   
   /**
    * @var string
    * return the article block title
    */
   private $cms_title;
   
   
   /**
    * Let get the topics needed for the CMS articles.
    * @return
    * @param object $a_forum_id       Sets the forum id to get the topics from
    * @param object $limit            Sets the limit of articles for pagnition
    * @param object $display_nav      Display article navigation true/false
    * @param object $comments         Display a link to the topic for comments true/false,
    *                            REMEMBER the forum must be public for comments.
    * @param object $order            Order the articles by 'ASC'/'DESC'
    * @param object $cms_title         Give your page atitle 'Home Page'
    *
    * cms_class::get_cms($a_forum_id, $limit, $display_nav, $comments, $order, $cms_title);
    * EXAMPLE    cms_class::get_cms('2', '6', true, true, 'ASC', 'Home Page');
    */
   public static function get_cms($a_forum_id, $limit, $display_nav, $comments, $order, $cms_title)
    {
      /**
         * Set the needed globals
         */
      global $phpbb_root_path, $phpEx, $db, $template, $user, $auth;
      
      /**
       * @var string
       * request needed vars
       */
      $forum_id   = request_var('f', 0);
      $topic_id   = request_var('t', 0);

      /**
       * @var Starting pagnition for the Articles
       */
      $check_params = array(
          $start   = request_var('start', 0),
         $limit   = request_var('limit', $limit),
      );
      
      /**
       * @var put the params for pagnition in an array
       */
      $params = array();
      
      foreach ($check_params as $key => $default)
      {
          if (!isset($_REQUEST[$key]))
          {
              continue;
          }
         
          $param = request_var($key, $default);
          $$key = $param;
         
          $params[] = urlencode($key) . '=' . ((is_string($param)) ? urlencode($param) : $param);
      }
      
      $pagination_url = append_sid($phpbb_root_path . '../index.' . $phpEx . implode('&amp;', $params));
      
      /**
       * Build SQL array for the Articles
       * @var $a_forum_id as a function param set to your forum_id
       */
      
      $sql_ary = array(
          'SELECT'       => '(t.topic_id), (p.poster_id), (u.user_id), username, user_colour, post_id, post_username, post_text, post_subject, post_time, bbcode_uid, bbcode_bitfield, enable_bbcode, enable_smilies, enable_magic_url',
         'FROM'         => array(
          POSTS_TABLE      => 'p',
         TOPICS_TABLE   => 't',
         USERS_TABLE      => 'u',
          ),
         'WHERE'         => 'p.post_id = t.topic_first_post_id
            AND p.post_time = t.topic_time
            AND   p.forum_id    = ' . (int)$a_forum_id . '
            AND u.user_id   =  p.poster_id',
         'ORDER_BY'      => 'p.topic_id ' . $db->sql_escape($order),
      );
      
      $sql = $db->sql_build_query('SELECT', $sql_ary);
      $result = $db->sql_query_limit($sql, $limit, $start);
      
                  
      while ($post_data = $db->sql_fetchrow($result))
      {
         $poster_id = $post_data['poster_id'];
         //Set BBC and Smiley options
         $post_data['bbcode_options'] = (($post_data['enable_bbcode']) ? OPTION_FLAG_BBCODE : 0) +
            (($post_data['enable_smilies']) ? OPTION_FLAG_SMILIES : 0) +
            (($post_data['enable_magic_url']) ? OPTION_FLAG_LINKS : 0);
                                                        
         //Check
         $poster_id = $post_data['poster_id'];
         // assign the database results to the block_vars loop
         $template->assign_block_vars('post_data', array(
            'POST_AUTHOR_FULL'   => get_username_string('full', $poster_id, $post_data['username'], $post_data['user_colour'], $post_data['post_username']),
            'POST_TITLE'      => $post_data['post_subject'],
            'ARTICLE_LINK'      => $post_data['topic_id'],
            'POST_TEXT'        => generate_text_for_display($post_data['post_text'], $post_data['bbcode_uid'], $post_data['bbcode_bitfield'], $post_data['bbcode_options']),
            'POST_DATE'         => $user->format_date($post_data['post_time']),
            'U_COMMENTS'      => append_sid("{$phpbb_root_path}viewtopic.$phpEx", "f=$a_forum_id&amp;t={$post_data['topic_id']}"),
               
            /** No big permission deal here because they would need permissions to the forum first
            *  only admins should see these links
            */
            'U_EDIT'         => append_sid("{$phpbb_root_path}posting.$phpEx", "mode=edit&amp;f=$a_forum_id&amp;p={$post_data['post_id']}"),
            'U_DELETE'         => append_sid("{$phpbb_root_path}posting.$phpEx", "mode=delete&amp;f=$a_forum_id&amp;p={$post_data['post_id']}"),
         ));            
      }
      
      // free the result
      $db->sql_freeresult($result);
      
      /**
       * Run the query again with a count to set the pagnition
       */
      $sql_ary['SELECT'] = 'COUNT(p.post_subject) as total_articles';
      $sql = $db->sql_build_query('SELECT', $sql_ary);
      $result = $db->sql_query($sql);
      
      // get the total users, this is a single row, single field.
      $total_articles = $db->sql_fetchfield('total_articles');
      // free the result
      $db->sql_freeresult($result);
      
      // Assign index specific vars
      $template->assign_vars(array(
         'S_CMS_PAGES'      => true,
         'CMS_TITLE'         => $cms_title,
         'PAGINATION'        => generate_pagination($pagination_url, $total_articles, $limit, $start),
          'PAGE_NUMBER'       => on_page($total_articles, $limit, $start),
         'PAGE_NUM'          => $start,
         'S_NAVIGATION'      => $display_nav,
         'S_COMMENTS'      => $comments,
          'TOTAL_ARTICLES'    => ($total_articles == 1) ? $total_articles['total_articles'] : sprintf($total_articles['total_articles'], $total_articles),
      ));
    }      
}
?>

[Obsidian]

This isn't about security, but I think this is redundant:

Code: Select all

      /**
       * @var Starting pagnition for the Articles
       */
      $check_params = array(
          $start   = request_var('start', 0),
         $limit   = request_var('limit', $limit),
      );
     
      /**
       * @var put the params for pagnition in an array
       */
      $params = array();
     
      foreach ($check_params as $key => $default)
      {
          if (!isset($_REQUEST[$key]))
          {
              continue;
          }
         
          $param = request_var($key, $default);
          $$key = $param;
         
          $params[] = urlencode($key) . '=' . ((is_string($param)) ? urlencode($param) : $param);
      }

Just use request_var for those, and toss it through intval() or something.

[Handyman]

This isn't about security, but I think this is redundant:

Code: Select all

      /**
       * @var Starting pagnition for the Articles
       */
      $check_params = array(
          $start   = request_var('start', 0),
         $limit   = request_var('limit', $limit),
      );
     
      /**
       * @var put the params for pagnition in an array
       */
      $params = array();
     
      foreach ($check_params as $key => $default)
      {
          if (!isset($_REQUEST[$key]))
          {
              continue;
          }
         
          $param = request_var($key, $default);
          $$key = $param;
         
          $params[] = urlencode($key) . '=' . ((is_string($param)) ? urlencode($param) : $param);
      }

Just use request_var for those, and toss it through intval() or something.

you are correct that those are redundant (didn't check for redundant code? just security issues
however, running it through request_var, if the default is an int, request_var will make sure the value is always an int? same with string, so no need to "toss it through intval()
just do

Code: Select all

$start   = request_var('start', 0),
$limit   = request_var('limit', (int) $limit), 

and they will be forced int.

[Obsidian]

Ah, I didn't know it's forcing the integer typeset if the default is an integer. Thankee, Handy.


Would the private vartype force the class to OOP5+, though? I'm curious, haven't really worked with OOP5 yet.

[topdown]

That part was all taken from the phpBB Wiki on pagination.
There was some other code that I cut from it also that was un-needed but once I got it working I stopped
The whole Pagination thing took me a while to get working in the first place lol, now I can go through and look at what your saying and trim it up.

As for request_var and forcing INT, isn't a value of (int) always a number ?
Just wondering because in the ucp.php they use something like
$user_name = request_var('un', '');

un = a user name obviously
I have had it return other strings also.
Like
$page = request_var('page', 'Home')
It my not be the correct way, but it works.
?page=Home

If so, what are they doing different for user_name

[mtotheikle]

I updated the Wiki and regarding the int as the default that is not always true. request_var() requires two parameters, obsessively. The first is the name and the second is the default value which will also determine the strings type. So for example, $int = request_var('id', 0); will always be an integer so if a user inserts say "id=something" it will just be 0. This adds extra security and is why phpBB requires mods to use the request_var function when obtaining user input. It is always good practice though to type cast all strings and intergers when using them in a SQL statement and don't forget to escape user input. Now what Handyman did was type case $limit as a int so it is the same thing as $limit = request_var('limit', 0);

[topdown]

Changing all of that you end up with Undefined variable: params

From the pagination url

Code: Select all

$pagination_url = append_sid($phpbb_root_path . '../index.' . $phpEx . implode('&', $params)); 

Which also exists in the Wiki

Also the way pagination is set up in members.php

Spoiler:

Code: Select all

        // Build a relevant pagination_url
        $params = $sort_params = array();

        // We do not use request_var() here directly to save some calls (not all variables are set)
        $check_params = array(
            'g'                => array('g', 0),
            'sk'            => array('sk', $default_key),
            'sd'            => array('sd', 'a'),
            'form'            => array('form', ''),
            'field'            => array('field', ''),
            'select_single'    => array('select_single', $select_single),
            'username'        => array('username', '', true),
            'email'            => array('email', ''),
            'icq'            => array('icq', ''),
            'aim'            => array('aim', ''),
            'yahoo'            => array('yahoo', ''),
            'msn'            => array('msn', ''),
            'jabber'        => array('jabber', ''),
            'search_group_id'    => array('search_group_id', 0),
            'joined_select'    => array('joined_select', 'lt'),
            'active_select'    => array('active_select', 'lt'),
            'count_select'    => array('count_select', 'eq'),
            'joined'        => array('joined', ''),
            'active'        => array('active', ''),
            'count'            => (request_var('count', '') !== '') ? array('count', 0) : array('count', ''),
            'ipdomain'        => array('ip', ''),
            'first_char'    => array('first_char', ''),
        );

        foreach ($check_params as $key => $call)
        {
            if (!isset($_REQUEST[$key]))
            {
                continue;
            }

            $param = call_user_func_array('request_var', $call);
            $param = urlencode($key) . '=' . ((is_string($param)) ? urlencode($param) : $param);
            $params[] = $param;

            if ($key != 'sk' && $key != 'sd')
            {
                $sort_params[] = $param;
            }
        }

        $u_hide_find_member = append_sid("{$phpbb_root_path}memberlist.$phpEx", "start=$start" . (!empty($params) ? '&' . implode('&', $params) : ''));

        if ($mode)
        {
            $params[] = "mode=$mode";
        }
        $sort_params[] = "mode=$mode";

        $pagination_url = append_sid("{$phpbb_root_path}memberlist.$phpEx", implode('&', $params));
        $sort_url = append_sid("{$phpbb_root_path}memberlist.$phpEx", implode('&', $sort_params));
 

I can't see how to get it working when you need the $start and $limit in an array and need to send the array to the pagination url.
Anything else results in a broken URL

oOOo, looking at the function I see I can have previous and next buttons

[Handyman]

that way of doing pagination is great if you have a ton of variables like they do in memberlist? but with only start and limit, just add them to the url

Code: Select all

$pagination_url = append_sid($phpbb_root_path . '../index.' . $phpEx, 'start=' . $start . '&limit=' . $limit);