SoftwareSecurity2013/Group 1/Code Scanning/manual

Focus points

We identified the following things as points of interest to check. They are capable of providing user input (or are related to user input).

• $_GET - contains all URL parameters
• $_POST - contains POST data
• $_SERVER - contains information like user agent (which can be spoofed, poisoned by the user)
• $_FILES - contains malicious or prepared attachments
• SQL Queries based on (previous inserted) user data

Division of work

We divided all the relevant files (all non-HTML and language files) of FluxBB among the group. Each file is at least inspected by two persons.

Files	Ruud	Bas	Jeroen	Ruben	Erwin
Main pages
admin_bans.php	X	-	-	-	X
admin_categories.php	X	-	-	-	X
admin_censoring.php	X	-	-	-	X
admin_forums.php	X	-	-	-	X
admin_groups.php	-	X	-	-	X
admin_index.php	X	-	-	-	X
admin_loader.php	X	-	-	-	X
admin_maintenance.php	X	-	-	-	X
admin_options.php	-	X	-	-	X
admin_permissions.php	-	-	X	-	X
admin_ranks.php	X	-	-	-	X
admin_reports.php	X	-	-	-	X
admin_users.php	X	-	-	-	X
db_update.php	X	-	-	-	X
delete.php	X	-	-	X	-
edit.php	X	-	-	X	-
extern.php	X	-	-	X	-
footer.php	-	-	-	X	X
header.php	X	-	-	X	-
help.php	-	-	-	X	X
index.php	X	-	-	X	-
login.php	X	-	-	X	-
misc.php	X	-	-	X	-
moderate.php	X	-	X	-	-
post.php	-	X	X	-	-
profile.php	-	X	-	X	-
register.php	-	X	X	-	-
search.php	-	X	-	-	X
userlist.php	-	-	X	-	X
viewforum.php	-	-	X	-	X
viewtopic.php	-	X	X	-	-
Dependencies
include/cache.php	-	X	-	-	X
include/common.php	-	X	-	X	-
include/common_admin.php	-	X	-	-	X
include/dblayer/common_db.php	-	X	-	-	X
include/dblayer/mysql.php	-	X	-	-	X
include/dblayer/mysql_innodb.php	-	X	-	-	X
include/dblayer/mysqli.php	-	X	-	-	X
include/dblayer/mysqli_innodb.php	-	X	-	-	X
include/dblayer/pgsql.php	-	X	-	-	X
include/dblayer/sqlite.php	-	X	-	-	X
include/email.php	-	X	-	-	X
include/functions.php	-	X	-	X	-
include/parser.php	-	X	-	X	-
include/search_idx.php	-	-	-	X	X
include/utf8/mbstring/core.php	X	X	-	-	-
include/utf8/native/core.php	X	-	X	-	-
include/utf8/ord.php	X	-	-	X	-
include/utf8/str_ireplace.php	X	-	-	-	X
include/utf8/str_pad.php	X	-	-	X	-
include/utf8/str_split.php	X	-	X	-	-
include/utf8/strcasecmp.php	X	X	-	-	-
include/utf8/strcspn.php	X	-	X	-	-
include/utf8/stristr.php	X	-	-	X	-
include/utf8/strrev.php	X	-	-	-	X
include/utf8/strspn.php	X	-	-	X	-
include/utf8/substr_replace.php	X	-	X	-	-
include/utf8/trim.php	X	X	-	-	-
include/utf8/ucfirst.php	X	-	X	-	-
include/utf8/ucwords.php	X	-	-	X	-
include/utf8/utf8.php	X	-	-	-	X
include/utf8/utils/ascii.php	X	-	-	X	-
include/utf8/utils/bad.php	X	-	X	-	-
include/utf8/utils/patterns.php	X	X	-	-	-
include/utf8/utils/position.php	X	-	X	-	-
include/utf8/utils/specials.php	X	-	-	X	-
include/utf8/utils/unicode.php	X	-	-	-	X
include/utf8/utils/validation.php	X	-	-	X	-

Design choice

While evaluating FluxBB, we came across some design choices we thought were bad. They are given below. An explanation is given for each point why we think it is a bad design choice.

• Admin pages do not check data consistently. An admin can put anything on the page (i.e. message of the day). Although an admin is usually a trusted party, an admin might become compromised and thus exploit these fields. Furthermore, Erik highlighted that it could have even worse implications: ie. if someone would move or reuse parts of the badly designed admin area towards some public webpages of the forum, not being aware of the flaws that some other programmer introduced. In general we can say that it is really bad practice to have insecure code somewhere in your design.
• Data is only escaped when displayed. We are aware this is common practice (since you want to save the original data and work with the original data as long as possible). However, it introduces potential problems when a developer forgets to escape data for output.
• In a lot of places, simple data structures could be used. PHP supports classes since PHP4. As an example, building URLs for redirects: header('Location: '.str_replace('&', '&', $destination_url));. A better approach would be to use a class representing an URL and use something like header('Location: ' . $urlStructureInstance->toString());. This way, logic is separated.
• Errors and possible security infractions are not logged. When something unexpected occurs, you do want it to get logged in order to look it up afterwards and fix the problem.
• One can clearly see this project is old. The use of a framework could circumvent a lot of issues we've seen. There isn't a separation between view, controller and database. We continuously had to trace variables back to the $_POST or $_GET to know if it was safe or not.
• A lot of code is repeated (surely not KISS).
• It contains comments like this: // Don't ask me how the following works. It just does, OK? :-)
• (Erik: Also, looking at the code snippets below, I'm not convinced fluxbb uses the safest way of doing SQL calls in php...But maybe that is included in your framework comment above. Maybe coding on fluxbb started way before there were prepared staements in php.)

List of possible vulnerabilities

 Severity: High

 Issue: SQL injection

   File: admin_users.php:
   Line 681:
     $registered_after = isset($_GET['registered_after']) ? trim($_GET['registered_after']) : ;
   Line 723:
     $conditions[] = 'u.registered>'.$registered_after;
   Line 761:	
     $result = $db->query('SELECT COUNT(id) FROM '.$db->prefix.'users AS u LEFT JOIN '.$db->prefix.'groups AS g ON g.g_id=u.group_id WHERE u.id>1'.(!empty($conditions) ? ' AND '.implode(' AND ', $conditions) : )) or error('Unable to fetch user info', __FILE__, __LINE__, $db->error());
   
  The variable $registered_after is supplied by the user, is not checked before it is stored in the $conditions array. Finally the contents of this array are placed directly into the SQL query, without being escaped.

 Severity: High

 Issue: SQL injection

   File: admin_users.php:
   Line 291:
     $new_group = isset($_POST['new_group']) && isset($all_groups[$_POST['new_group']]) ? $_POST['new_group'] : message($lang_admin_users['Invalid group message']);
   Line 294: 
     $result = $db->query('SELECT g_moderator FROM ' . $db->prefix . 'groups WHERE g_id=' . $new_group) or error('Unable to fetch group info', __FILE__, __LINE__, $db->error());
    
   The $new_group variable is not escaped before it is put into the the query.

 Severity: Medium

 Issue: Cross site scripting vulnerability

   File: admin_options.php, line 29:
   'board_desc'			=> pun_trim($_POST['form']['board_desc']),
   The variable ‘board_desc’ is saved to the database on line 202, without being checked for cross site scripting vulnerabilities.
    
   File: header.php, line 178:
   $tpl_main = str_replace('<pun_desc>', '<div id="brddesc">' . $pun_config['o_board_desc'] . '</div>', $tpl_main);      
   As shown above, the variable ‘o_board_desc’ is placed directly into the page, without being checked.

 Severity: Medium

 Issue: Cross site scripting vulnerability

   File: admin_options.php, line 82:
   'rules_message'			=> pun_trim($_POST['form']['rules_message']),
   The variable ‘rules_message’ is saved to the database without being checked for cross site scripting vulnerabilities.
         
   File: misc.php, line 39:
   <div class="usercontent"><?php echo $pun_config['o_rules_message'] ?></div>      
   The ‘o_rules_message’ is placed directly into the page.

 Severity: Medium

 Issue: SQL injection

   File: functions.php:
   Line 1094:
     $remote_addr = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
 
   If FORUM_BEHIND_REVERSE_PROXY is defined, 'HTTP_X_FORWARDED_FOR' is returned by the function without being checked. 
   This results in possible SQL vulnerabilities in post.php lines 176, 192, 295, 301, 407, functions.php line 247 and register.php lines 159, 69.