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.
What is Involved in a code review of phpBB??
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!
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!
-
- Registered User
- Posts: 6
- Joined: Thu Apr 08, 2004 7:14 am
-
- 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??
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
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
Re: What is Involved in a code review of phpBB??
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
█ 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
-
- 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
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.
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.
-
- Registered User
- Posts: 29
- Joined: Sat Oct 25, 2003 6:57 am
- Location: Melbourne, Australia
- Contact:
An example of the code review
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
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.
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:
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
[ 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)
- 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
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) : $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;
}
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;
}
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
-
- 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??
/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."
Re: What is Involved in a code review of phpBB??
That's pretty impressive. After 3.0 is released, this may be a worthy project to take up.
Re: What is Involved in a code review of phpBB??
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.
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.
-
- 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??
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 smallersaltydx wrote: That's pretty impressive. After 3.0 is released, this may be a worthy project to take up.
Andrew
Re: What is Involved in a code review of phpBB??
Andrew, dont forget your brother (BioALIEN) when/if you become part of the dev team
Out of curiousity, what languages are you competant in?
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
█ 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