Software Security/Group 1/Code Scanning Reflection

Uit Werkplaats
< Software Security‎ | Group 1
Versie door Eelis van der Weegen (overleg | bijdragen) op 20 jun 2011 om 14:09 (Comparison)
(wijz) ← Oudere versie | Huidige versie (wijz) | Nieuwere versie → (wijz)
Ga naar: navigatie, zoeken

Fortify

Our group focuses on HTML output generation, but as described in our log, the way phpBB generates its HTML output presents an obstacle: phpBB2 uses a template approach whereby an almost-HTML .tpl file is parsed for special variable substitution and iteration directions, "compiled" into PHP code consisting predominantly of echo statements, which is then eval()'d. Because this PHP code is only generated dynamically, it is not "visible" to source code analysis tools. These templates only output html with a few simple PHP constructs like variables, ifs and for-loops. So this mostly just applies to HTML encoding and XSS. What is possible in theory is another story, it seems the templates can include PHP themselves. To work around this problem, we have instrumented the phpBB code to write the generated output generation code into files on disk. We then took these files (containing PHP code), added them to the phpBB source code tree, and then ran Fortify on it.

This generated an analysis file containing 6088 "issues", though it must be said that the way Fortify counts issues appears to be a bit zealous---the actual number of reasonably distinct issues seems to be much lower. A small number could be applied to HTML ouput encoding. Of that number most were false-positives, because Fortify was not aware that phpBB stores content in the database in escaped form.

In any case, Fortify has helpfully categorized these issues. Of the respective categories, only the roughly "400" ones related to cross-site scripting would seem to fall under our group's jurisdiction. However, all of them appear to pertain to code that is either only run during phpBB installation, or used when phpBB is run in debug mode. Neither are at play during normal forum operation.

It appears, then, that despite our efforts (described above) to make the dynamically generated output generation code visible to Fortify, it was simply unable to "see" what the code is doing. Hence, it failed to diagnose any actual HTML output problems. We speculate that this is due to the fact that the dumped output generation code is not actually invoked through normal PHP function calls (but rather through eval()), and does not contain any include-directives, making it understandably hard for Fortify to relate it to the rest of the code base and code paths.

We tend to believe that dynamically composing strings containing PHP code, and using eval() to execute it (the way phpBB does), is essentially inherently inimical to static source code analysis, resulting in our sadly meager harvest. :-(

Codescan

Codescan has various categories to scan for, like XSS, SQL injection and poor coding practices.

Codescan also the capability to track which includes are used and so to track which functions/methods/procedures are from the include. So during the scan the tool tries to derive include paths, but in phpbb the paths in the include statement are reliant on the variable $phpEx to be declared. This is done in a file named "extension.inc" which is not interpreted as php by codescan and causes all the includes to fail. To counter this problem we have manually replaced all $phpEx variable uses with .php.

To review the vulnerabilities found there is no good way to filter so you will only get to see the actual problems you want, you can only filter on the categories given at the start. Then you still have to manually check all vulnerabilities in that category which can be hundreds, this is time consuming.

Comparison

Fortify Codescan
Runtime 45 minutes 30 minutes (+30 minutes preparing the code)
Number of warnings 6088 1029
Clarity of warnings There is a default description per type of warning, which makes a lot of warnings look like each other and they are categorized. Which makes it easier to disable/ignore irrelevant warnings. There is also possibility to trace them to the source of the warning which makes it easier to find the actual responsible line of code. There is a default description per type of warning, but there are too few categories which makes them useless for filtering (still too many warnings are left). There is also a possibility to trace the warning source, but this works less intuitive then Fortify for example.