SoftwareSecurity2014/Group 13/Code Scanning

OWASP

Verify that all untrusted data that are output to HTML (including HTML elements, HTML attributes, javascript data values, CSS blocks, and URI attributes) are properly escaped for the applicable context. [1]

Fortify

The following vulnerabilities which can cause security risk V6 were detected. We give one example per vulnerability.

Dangerous Functions

Abstract: Functions that cannot be used safely should never be used. Sink: mysql.php:161 mysql_escape_string()

return mysql_escape_string($str);

This function does not take the used character encoding in consideration. Therefore, using it will never be safe. Furthermore, this function has been deprecated (in php) for quite some time.

Cross-Site Scripting: Reflected (14)

Abstract: Line 584 of db_update.php sends unvalidated data to a web browser, which can result in the browser executing malicious code.
Source: db_update.php:456 - Read $_REQUEST['req_old_charset']()
Sink: db_update.php:584 - builtin_echo()

456 $old_charset = isset($_REQUEST['req_old_charset']) ? str_replace('ISO8859', 'ISO-8859', 
strtoupper($_REQUEST['req_old_charset'])) : 'ISO-8859-1';
...
584 <input type="text" name="req_old_charset" size="12" maxlength="20" value="<?php echo
$old_charset ?>" /><br />

Here the value of variable old_charset is used as value in the HTML form. This can be a copy of untrusted variable $_REQUEST['req_old_charset'].

Cross-Site Scripting: Poor Validation (2)

Abstract: In search.php, the program uses HTML, XML or other type of encoding that is not always enough to prevent malicious code from reaching the web browser.
Source: search.php:30 - Read $_GET['action']
Sink: search.php:560 - builtin_echo(0)

30 $action = (isset($_GET['action'])) ? $_GET['action'] : null;
...
316 $search_type = array('action', $action);
...
538 $keywords = $search_type[1];
...
547 $crumbs_text['search_type'] = '<a href="search.php?action=search&keywords='.urlencode($keywords).'&
author='.urlencode($author).'&forums='.$search_type[2].'&search_in='.$search_type[3].'&
sort_by='.$sort_by.'&sort_dir='.$sort_dir.'&show_as='.$show_as.'">'.$crumbs_text['search_type'].'</a>';
...
560 <li><span>» </span><strong><?php echo $crumbs_text['search_type'] ?></strong></li>

The variable $_GET['action'] is echo-ed in the HTML output, formatted through urlencode.

Path manipulation (6)

Abstract: Attackers can control the filesystem path argument to move_uploaded_file() at profile.php line 376, which allows them to access or modify otherwise protected files.
Source: profile.php:19 - Read $_GET['id']
Sink: profile.php:376 - move_uploaded_file(1)

19 $id = isset($_GET['id']) ? intval($_GET['id']) : 0;
...
376 if (!@move_uploaded_file($uploaded_file['tmp_name'], PUN_ROOT.$pun_config['o_avatars_dir'].'/'.$id.'.tmp'))

Command injection (1)

Abstract: Line 57 in admin_statistics.php calls exec() to execute a command. This might allow an attacker to inject malicious commands.
Source: admin_statistics.php:57 - exec(0)

57 else if (!in_array(PHP_OS, array('WINNT', 'WIN32')) && preg_match('%averages?: 
([0-9\.]+),?\s+([0-9\.]+),?\s+([0-9\.]+)%i', @exec('uptime'), $load_averages))

(Erik: But is that an output encoding/escaping problem? Or an input validation problem? I always find the border between the two confusing. )

Header manipulation (1)

Abstract: Including unvalidated data in an HTTP response header can enable cache-poisoning, cross-site scripting, cross-user defacement, page hijacking, cookie manipulation or open redirect.
Source: viewtopic.php:18 - Read $_GET['id']
Sink: viewtopic.php:63 - header(0)

18 $id = isset($_GET['id']) ? intval($_GET['id']) : 0;
...
63 header('Location: viewtopic.php?id='.$id.'&action=last');

Other vulnerabilities

There were a total of 165 vulnerabilities reported by Fortify.

Critical (20)

• Cross-Site Scripting: Reflected (14)
• Dangerous File Inclusion (1)
• Password Management: Password in HTML Form (2)
• Privacy Violation (2)
• System Information Leak: External (1)

High (16)

• Insecure Randomness (3)
• Password Management: Hardcoded Password (2)
• Path Manipulation (6)
• Privacy Violation: Autocomplete (5)

Medium (4)

• Cross-Site Scripting: Poor Validation (2)
• Often Misused: File Upload (2)

Low (125)

• Command Injection (1)
• Cross-Site Request Forgery (48)
• Header Manipulation (1)
• Hidden Field (55)
• Open Redirect (1)
• Password Management: Password In Comment (13)
• System Information Leak: External (2)
• Weak Cryptographic Hash (4)

Result

Most of the reported Cross-Site Scripting vulnerabilities seem to us to be true positives. For the poor validation more manual inspection is required to determine wether this is truly a vulnerability. The other reported issues, don't seem to be a serious threat.

Fortify did not find anything about SQL escaping or charset encoding. Maybe this means there are no vulnerabilities, but more likely Fortify does not check for them.

Fortify found some real output escaping vulnerabilities in FluxBB. (Erik: Some indication of how many would be nice, and of how many false positives you have to wade through to get them. Eg is it a handful of vulnerabilities among hundreds of false positives, or vv?)

RATS

RATS seems to be a glorified grep. The database for php is rather small and consists only of 55 possible vulnerable functions. RATS looks for uses of these functions.

basename
bzopen
bzread
chmod
chown
chroot
dirname
eval
exec
fgets
fgetss
file
filegroup
fileowner
fileperms
fopen
fscanf
fsockopen
getallheaders
getenv
gzfile
gzgetc
gzgets
gzgetss
gzopen
gzread
highlight_file
is_dir
is_executable
is_file
is_link
is_readable
is_writable
is_writeable
leak
link
lstat
mail
mkdir
opendir
passthru
pfsockopen
popen
posix_getlogin
posix_mkfifo
posix_ttyname
read
readfile
rename
rmdir
show_source
stat
symlink
system
unlink

Version 2.4, released on Dec 31, 2013.

Entries in php database: 55
../fluxbb-1.5.6/install.php:1660: High: fopen
../fluxbb-1.5.6/include/cache.php:217: High: fopen
../fluxbb-1.5.6/include/srand.php:99: High: fopen
../fluxbb-1.5.6/include/functions.php:2056: High: fopen
../fluxbb-1.5.6/db_update.php:648: High: fopen
Argument 1 to this function call should be checked to ensure that it does not
come from an untrusted source without first verifying that it contains nothing
dangerous.

../fluxbb-1.5.6/include/email.php:260: High: mail
Arguments 1, 2, 4 and 5 of this function may be passed to an external
        program. (Usually sendmail). Under Windows, they will be passed to a
        remote email server. If these values are derived from user input, make
        sure they are properly formatted and contain no unexpected characters or
        extra data.

../fluxbb-1.5.6/admin_statistics.php:39: Medium: is_readable
A potential TOCTOU (Time Of Check, Time Of Use) vulnerability exists.  This is
the first line where a check has occured.
The following line(s) contain uses that may match up with this check:
42 (fopen), 46 (fopen), 57 (exec)

../fluxbb-1.5.6/include/dblayer/sqlite.php:49: Medium: is_readable
A potential TOCTOU (Time Of Check, Time Of Use) vulnerability exists.  This is
the first line where a check has occured.
The following line(s) contain uses that may match up with this check:
44 (chmod)

../fluxbb-1.5.6/include/email.php:310: Medium: fsockopen
Argument 1 to this function call should be checked to ensure that it does not
come from an untrusted source without first verifying that it contains nothing
dangerous.

../fluxbb-1.5.6/include/functions.php:2048: Medium: is_dir
A potential TOCTOU (Time Of Check, Time Of Use) vulnerability exists.  This is
the first line where a check has occured.
The following line(s) contain uses that may match up with this check:
2056 (fopen), 2064 (unlink)

../fluxbb-1.5.6/header.php:53: Low: basename
../fluxbb-1.5.6/header.php:56: Low: basename
../fluxbb-1.5.6/header.php:64: Low: basename
../fluxbb-1.5.6/header.php:169: Low: basename
../fluxbb-1.5.6/include/email.php:144: Low: basename
../fluxbb-1.5.6/include/functions.php:1261: Low: basename
../fluxbb-1.5.6/include/functions.php:1386: Low: basename
../fluxbb-1.5.6/include/parser.php:712: Low: basename
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/admin_categories.php:12: Low: dirname
../fluxbb-1.5.6/delete.php:9: Low: dirname
../fluxbb-1.5.6/post.php:9: Low: dirname
../fluxbb-1.5.6/extern.php:60: Low: dirname
../fluxbb-1.5.6/admin_statistics.php:12: Low: dirname
../fluxbb-1.5.6/search.php:12: Low: dirname
../fluxbb-1.5.6/misc.php:12: Low: dirname
../fluxbb-1.5.6/admin_bans.php:12: Low: dirname
../fluxbb-1.5.6/install.php:23: Low: dirname
../fluxbb-1.5.6/install.php:138: Low: dirname
../fluxbb-1.5.6/viewforum.php:9: Low: dirname
../fluxbb-1.5.6/include/utf8/utf8.php:33: Low: dirname
../fluxbb-1.5.6/userlist.php:9: Low: dirname
../fluxbb-1.5.6/profile.php:9: Low: dirname
../fluxbb-1.5.6/register.php:9: Low: dirname
../fluxbb-1.5.6/admin_forums.php:12: Low: dirname
../fluxbb-1.5.6/admin_maintenance.php:14: Low: dirname
../fluxbb-1.5.6/admin_groups.php:12: Low: dirname
../fluxbb-1.5.6/moderate.php:9: Low: dirname
../fluxbb-1.5.6/login.php:12: Low: dirname
../fluxbb-1.5.6/db_update.php:37: Low: dirname
../fluxbb-1.5.6/db_update.php:791: Low: dirname
../fluxbb-1.5.6/admin_index.php:12: Low: dirname
../fluxbb-1.5.6/index.php:9: Low: dirname
../fluxbb-1.5.6/admin_reports.php:12: Low: dirname
../fluxbb-1.5.6/admin_censoring.php:12: Low: dirname
../fluxbb-1.5.6/help.php:12: Low: dirname
../fluxbb-1.5.6/admin_options.php:12: Low: dirname
../fluxbb-1.5.6/viewtopic.php:9: Low: dirname
../fluxbb-1.5.6/edit.php:9: Low: dirname
../fluxbb-1.5.6/admin_loader.php:12: Low: dirname
../fluxbb-1.5.6/admin_permissions.php:12: Low: dirname
../fluxbb-1.5.6/admin_users.php:12: Low: dirname
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/install.php:1660: Low: fopen
../fluxbb-1.5.6/include/cache.php:217: Low: fopen
../fluxbb-1.5.6/include/srand.php:87: Low: fopen
../fluxbb-1.5.6/include/srand.php:99: Low: fopen
../fluxbb-1.5.6/db_update.php:648: Low: fopen
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/include/cache.php:156: Low: is_dir
A potential TOCTOU (Time Of Check, Time Of Use) vulnerability exists.  This is
the first line where a check has occured.  No matching uses were detected.

../fluxbb-1.5.6/include/functions.php:1720: Low: is_dir
A potential TOCTOU (Time Of Check, Time Of Use) vulnerability exists.  This is
the first line where a check has occured.  No matching uses were detected.

../fluxbb-1.5.6/include/cache.php:157: Low: file
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/include/cache.php:243: Low: unlink
../fluxbb-1.5.6/include/functions.php:710: Low: unlink
../fluxbb-1.5.6/include/functions.php:790: Low: unlink
../fluxbb-1.5.6/profile.php:391: Low: unlink
../fluxbb-1.5.6/profile.php:398: Low: unlink
../fluxbb-1.5.6/db_update.php:1861: Low: unlink
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/profile.php:404: Low: rename
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

../fluxbb-1.5.6/profile.php:405: Low: chmod
A potential race condition vulnerability exists here.  Normally a call to this
function is vulnerable only when a match check precedes it.  No check was
detected, however one could still exist that could not be detected.

Total lines analyzed: 29043
Total time 0.034886 seconds
832511 lines per second

(Erik: So, was any of theis RATS feedback relevant/interesting for you?)

pfff

pfff is a collection of php static analysis tools written by Facebook.

using / for php_root
1 / 98PB with /home/rodin/fluxbb-1.5.6/admin_bans.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
2 / 98PB with /home/rodin/fluxbb-1.5.6/admin_categories.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
5 / 98PB with /home/rodin/fluxbb-1.5.6/admin_groups.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
12 / 98PB with /home/rodin/fluxbb-1.5.6/admin_statistics.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
13 / 98PB with /home/rodin/fluxbb-1.5.6/admin_users.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
14 / 98PB with /home/rodin/fluxbb-1.5.6/db_update.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
16 / 98PB with /home/rodin/fluxbb-1.5.6/edit.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
23 / 98PB with /home/rodin/fluxbb-1.5.6/include/common_admin.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
32 / 98PB with /home/rodin/fluxbb-1.5.6/include/functions.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
60 / 98PB with /home/rodin/fluxbb-1.5.6/install.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
89 / 98PB with /home/rodin/fluxbb-1.5.6/login.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
91 / 98PB with /home/rodin/fluxbb-1.5.6/moderate.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
92 / 98PB with /home/rodin/fluxbb-1.5.6/post.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
93 / 98PB with /home/rodin/fluxbb-1.5.6/profile.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
94 / 98PB with /home/rodin/fluxbb-1.5.6/register.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
95 / 98PB with /home/rodin/fluxbb-1.5.6/search.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
96 / 98PB with /home/rodin/fluxbb-1.5.6/userlist.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
97 / 98PB with /home/rodin/fluxbb-1.5.6/viewforum.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
98 / 98PB with /home/rodin/fluxbb-1.5.6/viewtopic.php, exn = Ast_php_simple_build.ObsoleteConstruct(_)
/home/rodin/fluxbb-1.5.6/include/search_idx.php:246:10: CHECK: Use of undeclared variable $words (did you mean $word?). 
/home/rodin/fluxbb-1.5.6/include/search_idx.php:261:10: CHECK: Use of undeclared variable $words (did you mean $word?). 
/home/rodin/fluxbb-1.5.6/include/search_idx.php:269:7: CHECK: Use of undeclared variable $words (did you mean $word?). 
/home/rodin/fluxbb-1.5.6/include/utf8/str_ireplace.php:41:50: CHECK: Use of undeclared variable $matches (did you mean $matched?). 
/home/rodin/fluxbb-1.5.6/include/utf8/str_ireplace.php:46:18: CHECK: Use of undeclared variable $matches (did you mean $matched?). 
/home/rodin/fluxbb-1.5.6/include/utf8/str_ireplace.php:48:57: CHECK: Use of undeclared variable $matches (did you mean $matched?). 
/home/rodin/fluxbb-1.5.6/admin_forums.php:315:49: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_forums.php:316:169: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_forums.php:317:164: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_forums.php:320:53: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_forums.php:321:175: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_forums.php:322:169: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/delete.php:33:139: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/delete.php:35:53: CHECK: ternary if ("?:") is not necessary here, use the condition or its negation.
/home/rodin/fluxbb-1.5.6/admin_censoring.php:17:4: CHECK: Use of undeclared variable $pun_user. 
/home/rodin/fluxbb-1.5.6/admin_censoring.php:18:9: CHECK: Use of undeclared variable $lang_common. 
/home/rodin/fluxbb-1.5.6/admin_censoring.php:21:25: CHECK: Use of undeclared variable $admin_language. 
/home/rodin/fluxbb-1.5.6/admin_censoring.php:32:10: CHECK: Use of undeclared variable $lang_admin_censoring. 
/home/rodin/fluxbb-1.5.6/admin_censoring.php:34:1: CHECK: Use of undeclared variable $db. 
/home/rodin/fluxbb-1.5.6/admin_censoring.php:34:27: CHECK: Use of undeclared variable $db. 
top 10 most recurring unused variable names
 $page_title -> 11
 $lang_common -> 9
 $focus_element -> 2
 $UTF8_BAD -> 1
 $UTF8_MATCH -> 1
 $UTF8_VALID -> 1
 $badList -> 1
 $cookie_name -> 1
 $db -> 1
 $footer_style -> 1
total errors = 2038

It noticed some funky ternary operator code

$read_forum = ($cur_perm['read_forum'] != '0') ? true : false;

which we think could be easily rewritten to

$read_forum = ($cur_perm['read_forum'] != '0')

and the use of some undeclared variables.

(Erik: Interesting you tried Pff, i have not seen that one before. Did you check if any of its suggestions for possible misspelling, eg "(did you mean $matched?)", were indeed correct? If so, that seems to be a clever way to spot potential trouble.)

PHPLint

PHPLint is a static typed language based on PHP. It has a tool to check for type errors in PHP source. Some snippets from the output of the tool on the FluxBB source code:

==== 70: ERROR: found expression of type int, expected type is boolean. Remember that 0 evaluates to FALSE, and any other integer value evaluates to TRUE.
set_magic_quotes_runtime(0);

==== 129: ERROR: variable $pun_config does not exist

==== 78: ERROR: `...? EXPR1 : EXPR2': type mismatch: EXPR1 is mixed[], EXPR2 is string
function stripslashes_array($array)
          {
                  return is_array($array) ? array_map('stripslashes_array', $array) : stripslashes($array);
          }