SoftwareSecurity2012/Group 4/Wanted Documentation

The FluxBB system has no official documentation at all. To follow this tradition, the module that we had to analyze did not have any documentation either. Now we have done our code analysis of the New Private Messaging System, we could determine what we would have liked to have as documentation to make the process of doing the security analysis easier. We would also elaborate a bit what we thought about the quality of the code.

Structured Code

The structure of the code of the fluxBB forum is a bit messy. The fluxBB forum does not make use of a template engine or something that parses HTML files to produce nice formatted webpages. This resulted in PHP scripts that were interleaved with HTML blocks which makes the code hard to analyze (because we are not interested in the HTML parts). The fluxBB is not implemented in an object oriented way and with the use of defining constants the system is checking if particular needed parts of the code are executed earlier. For example, when the FluxBB base code is executed, the PUN constant is set to 1. When the PMS module is executed, it is checked if the base is already executed by checking if the PUN constant is equal to 1. These dependencies could have been better implemented with the use of classes.

The used structure which really made the code less readable and understandable is the structure where function definitions were interleaved with HTML code blocks. This means that a PHP section was opened and closed with the function header in it, followed by HTML code that has to be outputted to the user every time the function is executed and after this a new PHP section was opend to close the function body. In this HTML code sometimes even new PHP sections were included to echo variables that were used in the current function. The structure looked something like the following:

<?php function startUglyFunction($mess, $moreMess) { doSomethingWith($mess);?>
    <div>
        <p><?php echo $mess[0] ?></p>
    </div>
    <div>
        <p><?php echo $mess[1] ?></p>
    </div>
    <div>
        <p><?php echo $mess[2] ?></p>
    </div>
<?php } ?>

Modular System for Plugins

The installation of the plugin was also not as nice as we have seen with other forum systems. We had to manually insert code in some fluxBB scripts to make sure the PMS module is integrated into the forum. This was defined in approximately 25 steps which was written in a readme.txt file that came with the New PMS mod package. There was also some sort of installation script which creates the extra database tables that are needed by the PMS module.

We did not like this approach because you rather want to keep the code of different modules separated (where we see the fluxBB base also as a module). Now we had to manually insert some code which could be error prone and different modules could interfere with each other. The manual installation assumes that you want to install the module for a fresh install of the fluxBB forum, but it is also possible that you already have installed tons of other modifications. This makes the chance of conflicting code reasonably big. These conflicts could then also pose some security problems because they could introduce vulnerabilities. There is no sandboxing or interfacing for the modules, so in principle, there is no control of what the modules are able to do and creating vulnerabilities by installing some mixture of modifications is not excluded.

Also, when a better modular setup was chosen, the sanitation steps could have been implemented at one point. Defence-in-depth is then still possible, but you have more certainty that you work with sanitized data. When you want to change the way the input is sanitized, you only have to change one or two functions in a certain module class. For example, you could have a form class where you can create input fields with certain properties (data type, required or not). When some data is inputted, the data you retrieve from this forms class is already sanitized and in the correct format. This makes programming other aspects of modules a lot less error prone because you have one centralized place for these sanitation steps. During our manual analysis, we have seen that making an error when you have to apply the sanitation functions every time you get some input. Only because some fortunate combination of the rest of the script trace, these errors could not do any harm or being exploited. But this means that when you do not centralize the sanitation steps, the chance that some exploit is possible is bigger.

More Step by Step Comments and PHPDoc

The PMS module that we have taken a look at, was a combination of work of several people. Some code files have the following header:

 * Copyright (C) 2010-2011 Visman (visman@inbox.ru)
 * Copyright (C) 2008-2010 FluxBB
 * based on code by Rickard Andersson copyright (C) 2002-2008 PunBB

Also the comments show that the code of the module is probably not all written by the same person because the comments are alternately written in the languages Russian and English. The Russian comments are still hard to follow for us (even after google translate magic) and we would have liked it more if the comments would have all been written in English.

Another thing that was a bit strange, was that there was a number of lines of code commented out in the code files. This looked like old, deprecated code and this probably should not be present in code that is released to the public. This code could easily be removed, but it made the files more messy.

Overall, some more detailed comments on what is done where in the code would have been very helpful. Namely because the alternating use of HTML blocks and PHP code blocks make the code less easy to comprehend, small comments which give the overall course of the process would really help.

For the fluxBB system itself, we would have liked some more documentation in the form of the arguments that the function can get, what the returned variable would probably look like and some pre and post conditions for the function. This all would make it also possible to generate some decent PHPDoc. When these pre and postconditions were formulated by the developer, we could learn more about the design choices that the developer made and the assumptions he made which are relevant for security. For example assumptions about the quality of the arguments. Do the arguments need to be sanitized before they are used as input for the function, or is this still done by the function itself. Can the output of the function be trusted or not? This would be some sort of implicit tainting of the variables which make the analysis a lot easier, because you know what goals the developer had when writing the code inside the function. This makes it easier to check if the goals were met and it could be used as a guideline for analyzing the global code structure.

Documentation of the PUN global objects

Most of the state information of the forum, is passed from the fluxBB forum base code to the modules in the form of global objects. These objects for example consisted of the user data (userid, username, user preferences), forum configuration settings, language definition etc. Here, we assume that the variables contain objects, but it is also possible for some variables that they are arrays or matrices.

The "recall" of these globals is done in the initialization code of the PMS module:

common_pmsn.php
    Line 18:     global $pun_config, $pun_user, $lang_pmsn, $lang_common, $pmsn_kol_list, $pmsn_kol_new, $pmsn_kol_save;

First of all, we do not think that this is the most decent way of passing information between different functions of the system. The use of globals should be avoided because they can be changed everywhere in the code that is included. (You can just retrieve them by redeclaration of the same global variables). When someone is able to include some code in the form of a Local File Inclusion or a Remote File Inclusion vulnerability, he is able to change every aspect of this global object.

But, for the documentation part, we would like the same documentation for the layout of the global objects as we would have liked to have for the functions. Information about what data format is expected for every field of the object. What can be assumed from the data that we can retrieve from the global objects and what is the overall layout of the objects. Now, we can only find what properties the objects have by dumping the contents at a certain place in the code (with var_dump()) or we can analyze all the places where information is assigned to fields (or attributes) of the objects. Because the objects are globals, this assignment can be done everywhere in the code of the whole system. This makes it very hard to analyze.