Software Security/Group 1/Manual Code Review

Manual Code Review Results

Having finished the automated Level 1B ASVS code scan, we proceeded to conduct a Level 2B ASVS Code Review of verification requirement 6.

General impressions of the code base

The phpBB code base can safely be said to be of poor quality. The code appears to be hacked together in a very ad-hoc fashion with very little vision or plan. Functions are huge and interdependent in non-obvious ways (through side effects), so it is often very hard to get a sense of what a particular piece of code is doing. Furthermore, since technical documentation and source code comments are exceedingly sparse and generally entirely devoid of insight, it is typically even harder to get a sense of why a particular piece of code is doing what it's doing. In many cases we could really only make guesses about what preconditions a piece of code depended on, which code was responsible for assuring those preconditions, what postconditions the code is trying to establish, and what invariants are at play. All this is assuming such notions even make sense to talk about in the context of code written by a hobby developer who in all likelihood has never heard of these notions in the first place.

Output Escaping

From what we can tell, most of the escaping of user-provided content (needed to combat cross-site scripting) is done once, prior to insertion of that content into the database, rather than every time the data is retrieved for output generation. In particular, the template engine does not seem to do much in the way of escaping at all.

One consequence of this is that any serious SQL injection problem (which we, after noticing query quoting problems just from glancing at the relevant parts of the code, expect to be plentiful in phpBB) also becomes a cross site scripting vulnerability, since an attacker can simply write unescaped data into the database. Of course, one might well argue that if the database is already compromised to that degree, a little cross site scripting is of relatively minor concern.

For the most part, the escaping that is done is done using built-in PHP HTML escaping functions, which seems fairly safe. However, this escaping is not applied consistently to all user-generated content, and we discuss some places where this causes problems, below.

Specific Issues

Avatar URLs

Each phpBB user profile may have an avatar: a small user-supplied picture to be shown in posts made by the user. But rather than letting users upload image files directly, users are supposed to provide an URL pointing to an image file, which phpBB then links to in the output HTML. Unfortunately, the URL that users provide is not escaped when phpBB writes the <img> element in the output HTML. Consequently, sneaky users can provide a string such as

yada"/>evil HTML code here

and the evil HTML code will end up in the final HTML.

The following is a piece of code from includes/usercp_avatar.php used to build the update query to set a user avatar's url, where $avatar_filename is not escaped. You can see that single quotes (') are escaped:

", user_avatar = '" . str_replace("\'", "", $avatar_filename) . "', user_avatar_type = " . USER_AVATAR_REMOTE

Next, the avatar is being rendered in viewtopic.php just by appending the avatar-url to the HTML output. Double quotes (") are not escaped:

'<img src="' . $postrow[$i]['user_avatar'] . '" alt="" border="0" />'

Dangerous links

For administrator actions, phpBB does not verify that they were submitted from actual administrator pages. Consequently, all an evil forum user has to do to cause mayhem is to post a cleverly crafted link and get an admin to click on it. While this is in itself not an escaping-related issue, it becomes relevant since these forum actions include ones that let an admin insert content into the database that will not be escaped when printed, see the example below.

Smiley escaping

Administrators can add custom smileys to phpBB's predefined set of smileys, but the HTML generated for these custom smileys is not escaped. And since smileys can be added with a single GET request, this means that a malicious user need only craft a link encoding a smiley-add GET request where the smiley expands to HTML of their choice, get an administrator to click on it, and then use the smiley wherever they want.

"Odd" characters in usernames

There are some forum functions (such as Search) that do not handle user names containing certain characters (such as hash signs) properly. While these particular forum functions are not terribly security sensitive, this is nevertheless an instance where "odd" user-supplied data is not processed properly, resulting in weird behavior. The software should either disallow user names containing such characters during registration, or not freak out when such user names are actually used.

Email manipulation

In phpBB it is possible to ask for an email notification when a reply has been made on a topic. The subject is included in this email. The topic subject is escaped when the topic is made. This escaped subject is then put in the database. When the email notification is sent, the topic subject is retrieved from the database and unescaped again in order to be viewed correctly in a non-HTML email.

phpBB has no clear separation between email headers and email body. Extra headers can be added to the email by adding the headers with new line in the topic subject.

New lines should of course not be allowed in the topic subject, but this is not checked by phpBB

??Erik: it is unclear to me why this is client side validation