SoftwareSecurity2014/Group 5/Reflection

Reflection on the whole process

Difficulties with analysing the code

In the second phase of the project we decided to focus on the input validation for the pages that handle the user profile. We ran Fortify, Rats and Rips on the source code of SMF but we noticed that it did not pick up code that was stored as string. For example, the parts of the code where some of the input validation is performed are found inside strings which are executed as a functions. An example is the code where Aim valid usernames are validated:

'input_validate' => create_function('&$value', '
				$value = strtr($value, \' \', \'+\');
				return true;
			')

This makes it hard for these tools to analyse the system. The validation code is often mixed with code that retrieves the user input and code that stores it in the database. We also had cases where code was simply copied. This made manual analysis harder because you have to read through all the other code we are not checking (because we focus only on Input Validation). Some user input was validated with validation rules we found a big array. We noticed that this really helped verifying our input validation requirements. (Erik: Good points)

Another very annoying part of analysing the code maunally was that the whole php code was build from functions which used a large amount of globals. This made it very annoying to trace all the information that was used inside the functions. For instance the function loadProfileFields from profile-modify.php:

function loadProfileFields($force_reload = false)
{
	global $context, $profile_fields, $txt, $scripturl, $modSettings, $user_info, $old_profile, $smcFunc, $cur_profile, $language;
        ...
}

Some of these globals are modified all over the place while others are just constant values. This generated lots of issues in Fortify. Also for manual code analysis this was annoying since you really have to check every function that is called, it might have modified some global.

SMF groups functions that are somewhat related to each other, for example the profile modify code is in Profile-Modify.php. The problem is that there are a lot of those functions and you don't know whether they do anything with user input. In modern web applications you often see a Model-View-Controller structure where you can clearly see the entry points of the web application. Because of the SMF code quality we decided to look at the visible elements on the page. We could then search for the strings in the code to see where the user input was handled.

Clarity of ASVS

It was possible to infer the meaning of all the security requirements, but there is also room to make them clearer to prevent multiple interpretations. (Erik: Also, it does not say anytinh about (positive) validation pattern being too "loose", as you came across.) One way to do this would be to add an example sentence after the security requirement which briefly explains some of the terms used in the security requirement. Take for example V5.2: "Verify that a positive validation pattern is defined and applied to all input". A follow up sentence could be added explaining what a positive validation pattern is. This may save users who know what white-listing is but forgot what the term "positive validation pattern" is from having to google it. It could even go a step further by giving hints for things to look out for in code which immediately violates or proves the security requirement. By adding these sentences you remove the elegance of having "one sentence requirements", so there is a trade-off between compactness and clearness.

Another requirement that we think is not very clear is requirement V5.6: "Verify that a single input validation control is used by the application for each type of data that is accepted.". There could be some misinterpretations with the terms "input validation control" and "type of data".

Other requirements such as v5.4 have a very clear description. Overall the requirements helped us to better understand what type of vulnerability belongs to input validation or what not. For example, one could think that SQL injection has something to do with input validation. When you look at the OWASP requirements you can see where SQL injection belongs.

In some cases we did not fully agree with the requirement. One of the requirement states that all input that is rejected should be logged. We believe that this is not necessary in all cases. For instance if the user enters an email without a domain then displaying an error message should be sufficient. (Erik: I agree! Moreover, the danger with too much logging is that no sys-admin will ever have a look at the logs.)

Splitting work among groups

It is hard to conclude that splitting the work is either sensible or non-sensible from just our experience during this assignment. The ASVS is quite large and we only used a part of it. On one hand you can argue that if you have specialized groups for each security requirement, you can be more sure about the results that these groups produce because they have seen the same type of issues multiple times. On the other hand it could be argued that this is not efficient, because some work might overlap because the same code is scanned by multiple groups. If you had one group looking at all of the code they would know that they had already scanned that piece of code and had already checked it for all requirements.

Most of the logic in SMF is organised per page instead of per function. So after a validation of the input you usually get a database query that updates the database. In this case it might be better to split the work per page instead of per ASVS security requirement. In an application where the code is organised by function(so like a Controllers folder that handles all the user input, model that saves to the database) it is easier to split per security requirement since you know where to look for a specific requirement.

Facilitating the security assessment

One way to facilitate a security assessment is to add a list of code patterns that are known to be exploitable or are known to be false positives. Tools like Fortify already search for code patterns that are dangerous but most issues tend to be false positives. If developers create a list of patterns and their security threat level based on their previous experiences, it could save some time during future security assessments. Another possibility is to add new functionality to tools like Fortify, where users can add very common patterns in the current project with their evaluation. In our case with SMF, it would be handy if the first person that saw that a variable was a constant could set a flag notifying all the others that this variable is a constant and that any functions using this variable can be ignored. This would have saved us time, since we had to trace the same variables to check if they were user input or constants. (Erik: Good points! Some convention of naming variables could be used here. Like in C one uses all CAPS for constants, or in C++ code often private fields are given names that begin with an underscore. One could introduce conventions to distinguish constants, input parameters that still need to be validated, and ones that have already been, etc - even in PHP :-)

We checked the documentation from SMF, they have a special page about the security[1]. We looked at the input validation part of it. All we found there was an explanation of SQL injection and "look how SMF does it". As a solution to SQL injection they propose some strange construction with htmlspecialchars, stripslashes and un_htmlspecialchars which does not make a good impression.

Perspective: reviewer vs developer

It is interesting to see how other programmers handle input validation in their code. It is educational because you learn from the positive examples and especially from the negative examples of how not to handle input validation. This is information we can take with us into our future projects. It can be frustrating to validate someone else's code because you frequently get the urge to change something in the code to your liking. We had to deal with many globals in the code which made it more work to trace where some variables were coming from (user input or constant). We learned how important it is to make a well thought design of your application. We now have a bit experience with verifying OWASP code requirements and we hope we can use this knowledge to produce code that is easier to verify.

Any other interesting, frustating, boring, educational, etc. aspects of the whole experience.

In the first part of the project, the 1B validation, we analysed a lot of issues Fortify reported. Although a lot of the issues had nothing to do with input validation, we thought it was good to check them to see if found any security issue. After the 1B deadline we started with the 2B verification. We have spend a lot of time on the fortify issues while it would have been better to start earlier with the manual code check. The tools were not useful to us for checking input validation issues. So we should have spent less time with the tools so that we could check more SMF pages for input validation issues. A lot of code we manually checked was in the Profile-Modify.php file. We checked Fortify, RIPS and RATS for the issues they reported in this file, none of them were of any use for us.

We found it remarkable how much input validation issues we found. It is understandable that SMF does not check if your ICQ number is valid. Issues like the strange birthday arithmetic were fun to exploit. We had more fun in trying to exploit some unvalidated input than checking all the issues the code scanners generated.