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); }