SoftwareSecurity2014/Group 3/Verdict

Verification requirements

V5.1 Passed

Requirement 5.1 reads "Verify that the runtime environment is not susceptible to buffer overflows, or that security controls prevent buffer overflows.". Well, we can be short about this one. PHP does not allow direct memory access in any way. Furthermore, PHP's arrays resize automatically if an element is appended while the array is full. The only way a buffer overflow could happen, is by abusing a bug in the PHP interpreter. This does not lie within the scope of this audit however. For this reason, we'll say that FluxBB is not susceptible to buffer overflows, so the security requirement is met.

V5.2 Not Passed

"Verify that a positive validation pattern is defined and applied to all input", says the second requirement. This means that everywhere user input is blocked or rejected by default, and only data that conforms to some predefined whitelist is allowed to pass through. This requirement is not met. Everywhere in the code, the input is not checked, but just sanitized. (Erik: Not so clear how you interpret 'sanitisation' vs 'checking' here. I'd say that they are more or less synonymous - though I agree the terms input validation, sanitisation, escaping, filtering, checking,.. can be a bit confusing.) When a user tries to enter malicious input, this is not detected (or logged) by the system at all, since it does not perform an explicit check. It only filters all data that is put into the database with the mysql_real_escape_string which is called "real escape" because it is"really unsafe".

V5.3 Passed

Security requirement 5.3 is "Verify that all input validation failures result in input rejection or input sanitization". In FluxBB, this requirement is met. FluxBB focuses mainly on input rejection and not so much on input sanitization , after input validation failures (Erik: I'd sya that input validation is the whole process, which can involve some 'escaping' to sanitise data, but also rejecting inputs that are deemed 'illegal'). FluxBB does do input sanitization before validation of input, for example escaping input:

$form_username = pun_trim($_POST['req_username']);

So the focus is on input rejection and we have found that FluxBB does this in three ways:

Firstly FluxBB simply tries a query and checks whether this query can be executed. If the query is not accepted (input validation fails), then the input is rejected and the error function is called.

Secondly FluxBB stores an error message in an "errors" array, after input validation failure. If this array is empty, then the "critical" code may be executed. However, if this array is not empty, then all stored error messages will be shown and new input will be required.

Thirdly FluxBB has several if-then-else-clauses that do input validation. If this validation fails, then an error message is shown directly.

Below some examples of the three possible ways of input rejection:

1.: $result = $db->query('SELECT * FROM '.$db->prefix.'users WHERE '.$username_sql) or error('Unable to fetch user info', __FILE__, __LINE__, $db->error());
2. : if (!is_valid_email($email))

$errors[] = $lang_common['Invalid email'];

3. :if (!file_exists(PUN_ROOT.'lang/'.$language.'/common.php'))

message($lang_common['Bad request'], false, '404 Not Found');

Concluding we think that V5.3 is met by FluxBB as it handles all input validation failures with input rejection. Error messages displayed to the user and these are short and do not reveal any sensitive information.

V5.4 Not passed

Security requirement 5.4 is "Verify that a character set is specified for all sources of input". In FluxBB, this requirement is not met. There are numerous code snippets that read and handle input, and multiple of these code snippets assume the GET or POST value does need no or little validation. This includes validation and specification of a charset. The FluxBB-guys built a function called "pun_trim", which in the end calls "utf8_trim", so it seems that they go with UTF-8 as the default dataset. However, they don't convert their input to UTF-8 for all inputs, nor do they validate it is UTF-8. For example, in the file "register.php". They 'pun_trim' a lot, but they don't trim the language POST value (see line 134) - even worse, they use it directly in a file-request on the server and only replace the chars / and \, which is unsafe of course. The FluxBB code base is littered with examples like these.

V5.5 Possibly passed

Security requirement 5.5 is "Verify that all input validation is performed on the server side.". In FluxBB, this requirement may be met. There is clearly no central point for all input and its validation, but it seems like all input is - one way or another - validated on the server (although poorly in most cases). Some validation is done on the client side as well (the file "header.php" seems to provide some functions for this, such as "process_form"). It is very hard to verify the entire code base, as all input is read locally and verified locally, and there are close to a thousand inputs in FluxBB. Next to this, there is no clear distinction between client side code (HTML, JavaScript), and the PHP code; everything is mixed together.(Erik:Good point!) So it might be the case that we missed something. Furthermore, we do not know all requirements of the software, thus cannot tell for sure if everything that should be validated is validated and the user can't break this by changing client side code.

V5.6 Possibly passed

There are a few functions defined in includes/functions.php which have something to do with string manipulation before processing. These functions are used pretty much all over the place, which is good. (Erik:Your phrasing is a bit ambiguous here. I'd say that using these functions 'all over the place' could be bad, if this is not done in a consistent way or if it is not clear that it done in all the RIGHT places :-) The quality of these functions is not very high though. These functions only trim strings or convert user input to UTF-8, so they don't really ensure a more secure system. Furthermore, all variables that are fed to the queries are escaped first by the database escape function for one of the supported databases (as listed in include/dblayer/). This function only calls the default escape function for the database that is used.

Because not all input is validated through one single function which is easy to maintain and therefore more secure, I think FluxBB did not really pass this requirement. Even though they use only a few different functions to validate input (they also have a dedicated function for CDATA), they do not meet this requirement. Even so, I'd say they do meet the requirement in some way: they did not create numerous different input validation functions all over the place but defined them in a few central places, where they are more easy to find and maintain. Given these two perspectives I'd say that they are very close to meeting this requirement.

V5.7 Not passed

Security requirement 5.7 is " Verify that all input validation failures are logged". In FluxBB, this requirement is not met. Having overviewed the login.php, register.php and functions.php files, we have found that FluxBB has no way of permanent logging after input validation failures. With this we mean that no information about any input validation failure is stored. There is however some very temporary and account-bound logging. Namely after every input validation failure a message is displayed to that current user. The amount of information is sometimes dependent on the privileges that user has. However, this can barely be called logging and therefore we state that FluxBB has not met requirement V5.7.

Overall remarks

FluxBB does not seem to be designed very well. Overall, it looks very, very hacked-together. It is not written object-oriented, there is a huge file called functions.php which contains tons of functions that do all kind of things, and there are no central functions to sanitize or check data input. Although that's not very concrete, this makes FluxBB feel very unsafe, and we'd not use it as forum software.