What is Involved in a code review of phpBB??

Discussion of general topics related to the new version and its place in the world. Don't discuss new features, report bugs, ask for support, et cetera. Don't use this to spam for other boards or attack those boards!
Forum rules
Discussion of general topics related to the new release and its place in the world. Don't discuss new features, report bugs, ask for support, et cetera. Don't use this to spam for other boards or attack those boards!
projectphp
Registered User
Posts: 6
Joined: Thu Apr 08, 2004 7:14 am

What is Involved in a code review of phpBB??

Post by projectphp »

A code review of phpBB (viewtopic.php?t=19141" target="_blank) was proposed, and I wondered exactly what that meant? If there is a bandwagon one needs to get on, I would like to know exactly how many wheels it has, who is driving, the methodology and what is required.

Also, what sort of numbers are we talking about here, and what will be required to get all this to happen??

I don't really care about what all the other stuff is about, but I am curious about this from a number of angles.

vanderaj
Registered User
Posts: 29
Joined: Sat Oct 25, 2003 6:57 am
Location: Melbourne, Australia
Contact:

Re: What is Involved in a code review of phpBB??

Post by vanderaj »

What sort of code review? If you mean security code review, the metrics I use are 100 to 1000 lines = 1 day of work, depending on the depth you want to go to. It seems to just work out that way.

I am the current editor of the OWASP Guide 2.0, which will become the primary webappsec standard (replacing the OWASP Guide 1.1.1 and Top 10). You can get a copy from here:

http://www.greebo.net/owasp/" target="_blank

This is a handy guide when looking at each of the major headings, and gives you an idea of what's needed to be checked for. I expect the guide to have about 250 things which need to be checked at the end.

Doing a code review is a six phase process:

* classifying assets
* Create an architecture overview
* decomposing the application (into DFDs)
* Identify threats
* Document threats
* Rate the threats

The first phase is easy: user privacy and forum data.

The second phase is helped a lot if architecture / design documentation exists. This is extremely rare in open source projects. Often you have to reverse engineer the application a little, which is made a lot faster when you have a dev who is familiar with the code helping as it removes a lot of the assumptions

Decomposing the app takes each of the major use cases and you sequence them using a component model. Using the components, you use a DFD to map what data goes where. This really helps understand trust boundary transitions. These areas need more attention than internal (ie between front end code and classes) and semi-trusted (ie between classes and the database) transitions.

Identify threats - look through the boundaries using the STRIDE threat model. Think how you can obviate the code for spoofing, tampering, repudiation, information disclosure, denial of service, and elevation of privilege. If you have time, work on the lower risk boundaries.

I tend to look for low lying fruit in each area using grep (findstr for our Windows friends). Some constructs are simply unsafe and they can be found pretty easily. phpBB probably doesn't have many of these left. However, using grep doesn't always work as often its the absence of a control which counts. For that, you need to understand what's missing. Having a dev who is familiar with the code or good detailed design documentation can shortcut this work by days.

Document and rank threats I tend to do together. I have a document template I use for this purpose which mirrors the OWASP headings. Each time I find a risk, I pop it into the right area as this makes remediation easier on larger teams (in commercial software, you'll find some people are good at auth/session and some are data gurus, etc... sectioning up the work into headings makes it easier for the work to be done by one or two specialists and they don't have to flip through a 80-100 page report.

Ranking threats is vital. There's no point in fixing low risk issues when there might be glaring issues elsewhere. When done well, this can really spend the security dollar (or in a volunteer project - volunteer time) in the best and most effective way.

Andrew

BioALIEN
Registered User
Posts: 120
Joined: Tue Jan 25, 2005 3:46 pm
Location: London, UK

Re: What is Involved in a code review of phpBB??

Post by BioALIEN »

Great insight, thanks for that vanderaj!
BioALIEN
█ Secure your name before someone else does: TheMillionDollarAdvert.com
█ Get your permanent listing from $10 USD. You too can own a piece of internet history.
The moment you master the art of programming is the moment you program the art itself! - BioALIEN

vanderaj
Registered User
Posts: 29
Joined: Sat Oct 25, 2003 6:57 am
Location: Melbourne, Australia
Contact:

OT: Why I use MS security threat modelling tools

Post by vanderaj »

In the other thread there were a few people who questioned my use of Microsoft's threat risk modelling, which is a fair enough comment for those aware of Microsoft's reputation in the security area prior to the Trustworthy Computing push (prior to circa 2003).

The reason I use STRIDE / DREAD is simple: it's about the only threat risk modelling tool out there which works well for code reviews and is easy to use (ie it's fast to use), thorough in its approach, and the results are easily grasped by non-techo types (there's a simple number between 0 and 10 instead of "high" "medium" or "low").

There are alternative threat risk models available, such as AS4360 and OCTAVE (which doesn't work very well, and I doubt anyone has read all 18 volumes), but they don't work well to identify threats in software and are better suited at decomposing the risk of overall systems and risk in financial systems.

Microsoft have really improved their enterprise architecture* and security research and output over the last few years. If others were investing in security research to the same extent as MS has, I'd expect competing alternatives to exist which are just as easy to use.

The results of STRIDE/DREAD being applied within MS are unequivocal: Windows 2003 has had far fewer security vulnerabilities than its predecessors, the vulnerabilies are less severe, and affected fewer installations due to attack area surface reduction. The approach works, so I use it.

STRIDE / DREAD has since morphed into CVSS, which is the formal threat modelling tool designated for use by the US Homeland Security.

CVSS:
http://www.dhs.gov/dhspublic/interweb/a ... 2-2-04.pdf" target="_blank

It's home:
http://www.dhs.gov/dhspublic/interapp/e ... l_0353.xml" target="_blank

CVSS will become mandated for federal government departments to comply with, and as it's going to be popular, I'm sure that organizations who have to comply with SOX will fall into line and use it as well.

In code reviews, I use the major difference between STRIDE / DREAD and CVSS: CVSS includes a local / remote and anonymous / authenticated bias, which I quite like, and the temporal bias works well for systems affected by Bugtraq. That way the risks fall to the correct levels. If you need to be authenticated and have local access, the likelihood of that attack is much lower than an attack like Code Red or SQL Slammer.

Why don't I use CVSS? It's a bit more complex than STRIDE / DREAD, and when you have to check not one but somewhere in the range of 250 things over many thousands of lines of code, it's just too cumbersome. CVSS makes sense for reports which make it to Bugtraq or full-disclosure, but it's a bit heavy for a code review. So I use the light weight predecessor.

Andrew

* MSF Agile 4.0 is a beautiful thing from a threat modelling point of view - it has inbuilt security checks - extraordinary and possibly unique for a capital M development methodology.

vanderaj
Registered User
Posts: 29
Joined: Sat Oct 25, 2003 6:57 am
Location: Melbourne, Australia
Contact:

An example of the code review

Post by vanderaj »

Here is an example of a single threat modelled using STRIDE / DREAD.

[ Devs / Mods: I've deliberately picked a very low risk item here. Please leave so people can see the sort of thing I'm talking about. The risk is very unlikely to be realised as $filename is not always generated by the user and thus is for the most part tamperproof.]

This is from PHP 2.2 in the CVS.

Threat

Potential remote OS command injection on Windows platforms with thumbnail cleanup routines

Threat Category
  • Tampering - may alter data on the host system
  • Denial of Service - may be able to delete more than expected
  • Elevation of privilege - injected command runs as the PHP user (Apache (thus PHP) runs as LOCALSYSTEM by default on Windows)
Risk
  • Damage - 10 - could completely destroy the host if running as LOCALSYSTEM (default user context for Windows Apache installs)
  • Reproducability - 1 - very rare - requires a Windows host with PHP 2.2 loaded
  • Exploitability - 2 - requires skill with Unicode and may not be possible even then
  • Affected users - 0 - I would have put 10 (ie all users), but it affects the host and with good backups, this really isn't a huge issue
  • Discoverability - 10 - I always put down 10 as the code is available to anyone
10+1+2+0+10 = 23/5 = 4.6 out of 10
Anonymous? Yes => bias unchanged
Remote? Yes => bias unchanged

Final risk: 4.6 out of 10 (medium risk if threat realised)

Description

It may be possible on Windows hosts to execute commands as the LOCALSYSTEM user. The likelihood is very low due to other controls, and the lack of PHP 2.2 systems installed on the Internet on hosts running Windows.

Code: Select all

// Delete File
function phpbb_unlink($filename, $mode = 'file')
{
	global $config, $user, $phpbb_root_path;

	$filename = ($mode == 'thumbnail') ? $phpbb_root_path . $config['upload_dir'] . '/thumb_' . basename($filename) &#58 $phpbb_root_path . $config['upload_dir'] . '/' . basename($filename);
	$deleted = @unlink($filename);

	if (file_exists($filename))
	{
		$filesys = str_replace('/','\\', $filename);
		$deleted = @system("del $filesys");
	}

	return $deleted;
}
The line constructing the path name does not use realpath() to canocalize the constructed filename, and thus Unicode characters may get through, and the line executing the system call on Windows doesn't escape the input to prevent additional commands from running. In addition, the code doesn't check that the resulting path is sane wrt to open_basedir restrictions before trying to unlink the file, and it doesn't cope well with long file names (cmd.exe, the Windows shell, must have double quotes around filenames containing spaces).

It is best practice to

* canocalize the filenames and then check that they're okay prior to first use
* check the file exists before trying to get rid of it
* wrap system call arguments with escapeshellcmd() or escapeshellarg() to prevent the argument from doing damage to the local system.

In Windows, the continuation character is & but you there are many more. As Windows is a Unicode operating system, this is a higher risk than in Unix or non-Unicode operating systems.

A suggested fix is:

Code: Select all

// Delete File
function phpbb_unlink($filename, $mode = 'file')
{
	global $config, $user, $phpbb_root_path;
	$deleted = false;

	$fn = $phpbb_root_path . $config['upload_dir'] . '/';
	if ( $mode == 'thumbnail' )
	{
		$fn .= 'thumb_';
	}
	$fn = realpath(utf8_decode($fn . basename($filename))); // will return false if file does not exist

	if ( $fn !== false ) // $fn is false or a string, so !== is correct to use
	{
		$deleted = @unlink($fn);
	}

	if ( !$deleted || file_exists($fn) )
	{
		// shouldn't need to do this now that realpath() has done its job
		$fn = escapeshellarg(str_replace('/','\\', $fn));
		$deleted = @system("del $fn");
	}

	return $deleted;
}
The above suggestion may need to be tweaked if basedir directives and safe mode are on.

There is an unstated assumption - the configuration of Apache can't be changed. This is typical of hosted environments. However, if the configuration could be changed, the E portion of STRIDE would indicate changing Apache to run as a low privilege user and enforcing the use of basedir and safe mode. In that case, the damage potential basically drops to 1 or 2 - even if the code was unfixed and the threat was realised, the likelihood of bad things happening is miniscule to impossible.

Andrew

Virtuality
Registered User
Posts: 197
Joined: Sat Apr 19, 2003 6:35 pm
Location: Sweden
Contact:

Re: What is Involved in a code review of phpBB??

Post by Virtuality »

/me thinks vanderaj is a real talent... People like you is needed, please don't give up even after some of the nasty comments here! You're doing a great job. :)
"phpBB3 is never late. Nor is it early. It arrives precisely when it means to."

saltydx
Registered User
Posts: 24
Joined: Sun Jun 06, 2004 1:56 am

Re: What is Involved in a code review of phpBB??

Post by saltydx »

That's pretty impressive. After 3.0 is released, this may be a worthy project to take up.

warmweer
Registered User
Posts: 118
Joined: Wed Jul 09, 2003 5:27 pm
Location: Belgium

Re: What is Involved in a code review of phpBB??

Post by warmweer »

vanderaj does seem to know what he's talking about but then again I'm in no way qualified to give a founded opinion. With the little knowledge I have I can understand the possible exploit but I'm fairly certain I could'nt come up with an idea like that, nor even make use of it (not that I would 8) ).

Whatever, by now PSOTFX and Acyd Burn will have a qualified opinion on this matter and I'm fairly confident that the correct decision has been taken (which may or may not involve a slight delay in releasing Beta's and RCs).

Frankly I'm really getting tired of all the impatience and the false expectations (but what the... I can't do anything about it) that phpBB3 will be the most exeptional thing since the discovery of fire. There is no need for phpBB3, version 2 is as good (or better) than other boards and version 3 will be better for some (but not for everyone). But when (yes, I said when) phpBB3 comes out, it should be secure and therefor a security analysis is necessary (and I'm 100% certain that this has not been neglected).

I'm certainly glad this topic was created (thx phproject) as it clears up the smog for some. I hope further posts (also in other topics) are equally enlightening.
Procrastination is my hobby, but I keep on postponing it.

vanderaj
Registered User
Posts: 29
Joined: Sat Oct 25, 2003 6:57 am
Location: Melbourne, Australia
Contact:

Re: What is Involved in a code review of phpBB??

Post by vanderaj »

saltydx wrote: That's pretty impressive. After 3.0 is released, this may be a worthy project to take up.
The idea is that you do it before 3.0 is released, so the time between 3.0 and 3.0.1 is maximized, and the residual threats to be much smaller :)

Andrew

BioALIEN
Registered User
Posts: 120
Joined: Tue Jan 25, 2005 3:46 pm
Location: London, UK

Re: What is Involved in a code review of phpBB??

Post by BioALIEN »

Andrew, dont forget your brother (BioALIEN) when/if you become part of the dev team :twisted:

Out of curiousity, what languages are you competant in?
BioALIEN
█ Secure your name before someone else does: TheMillionDollarAdvert.com
█ Get your permanent listing from $10 USD. You too can own a piece of internet history.
The moment you master the art of programming is the moment you program the art itself! - BioALIEN

Locked