You are here:
  1. Home
  2. Blog
  3. Interwebs
  4. Faqtastic: Insecure and Error-Prone

Faqtastic: Insecure and Error-Prone

 |  Interwebs

I recently took a look at faqtastic, by Cine of INEXISTENT scripts, to try and figure out why a friend was repeatedly being hacked. Much to my disappointment I found several holes in the script most commonly caused by a lack of validation. It’s also jam-packed with errors (only noticeable when error_reporting() is whacked up to E_ALL) which annoyingly (for the users) are all easily fixed. Errors like:

Notice: Use of undefined constant step – assumed ‘step’ in [install path]install.php on line 11

..are caused by the likes of $_GET[step]. You should always use single quotation marks around array keys, making the correct usage $_GET['step']. The only exceptions to this are when using echo/query statements, e.g: echo "this is a value from an array: $array[key]"; although some people (myself included) like to leave them in and wrap curly braces around ’em: echo "this is a value from an array: {$array['key']}";

A closer look at the code for install.php also shows:
mysql_query("INSERT INTO $tablesettings SET type='option', name='username', value='$_POST[username]'") or die(mysql_error());

Not a big deal in terms of security — if only the intended person runs this — but this is placing the contents of the new username field directly into the database. If a user accidentally adds special characters (by mistyping) they will end up with a garbage username. I would check this field against a regular expression to make sure that only letters and numbers are being entered. (Or ctype_alnum(), if you have the ctype functions enabled.)

After I get my install success message and a load more errors I try and log in.. only I can’t. I’m getting ‘Cannot modify header information’ messages, probably caused by data being sent to the browser or whitespace in admin.php. Tut tut, should have tested this before releasing the latest version. Too lazy to debug the admin panel, I think I’ll just manipulate my cookies with my handy dandy Firefox extension..

Anyway, this error garbage is all irrelevant to those of you already using the script — you want to know where the security issues lie and how to fix them, right? Okay, okay, let’s get into it.

Faq.php is the biggest offender, and the file I’ll really be coverering here. Excusing the 41(!) errors on the page, this page is where the validation issues lie. On lines 21 and 22, information from a cookie is being assigned to a variable without any cleaning. This data gets echo-ed info the FAQ form — let’s inject some HTML and break the form, shall we.. ? Not such a good idea.

I personally think that cookies at the point in the script are pointless. If a person is asking that many questions that they need their name remembering for them, they need to get a social life. My suggestions? Remove:
$name=(isset($_COOKIE[faqtastic_guestname]))?$_COOKIE[faqtastic_guestname]:"Name";
$email=(isset($_COOKIE[faqtastic_guestname]))?$_COOKIE[faqtastic_guestemail]:"Email";

..and on lines 30 and 31, swap <?=$name?> (short tags in a public script: tsk!) for Name and <?=$email?> for Email.

On line 62 there’s a desperate attempt at validation with if (strpos($_POST[question], '?') === false) which checks for the existance of a question mark. That’s all well and good, but inject some JavaScript and as long as you leave a question mark at the end — you’re good to go!

The biggest problem with the entire script, errors aside, is this line:
mysql_query("INSERT INTO $tablefaqs SET ip='".$_SERVER['REMOTE_ADDR']."', name='$_POST[name]', email='$_POST[email]', question='$_POST[question]'") or die(mysql_error());
Now if you’ve been paying attention to my blog recently you’ll know why. Take a guess? :D Okay, time’s up: data is being sent to the database with no cleaning or validation/sanitisation. In fear of beating a dead horse, I’ll let you read my other entries to figure out why this is a bad thing.

The fix! Aha! After include('config.php'); at the top of the page, add:
function clean($data) {
   $data = trim(htmlentities(strip_tags($data)));
   return $data;
}

And just before that mysql_query stuff I mentioned before, add:

foreach ($_POST as $key => $val) {
   $clean[$key] = clean($val);
}

Now you need to delete that mysql_query() line and replace it with:
mysql_query("INSERT INTO $tablefaqs (`name` , `email` , `question`, `ip`) VALUES ('$clean[name]', '$clean[email]', '$clean[question]', '" . clean($_SERVER['REMOTE_ADDR']) . "')") or die(mysql_error());

You will also need to swap $_POST[name] for {$clean['name']} on the line that starts with mail(get_setting(email),

If you deleted the cookie code mentioned above, you can also delete the following lines:
setcookie("faqtastic_guestname", $_POST[name], time()+((3600*24)*31));
setcookie("faqtastic_guestemail", $_POST[email], time()+((3600*24)*31));

The next issue, although not one of security, is header('Location: faq.php?a=1');. This is executed irrelevant of whether the database query was successful, which means your visitors get the success message even if their question did not go through. Bad practise, that.

Further on we have an occurence of $PHP_SELF (oops — left over from a previous register_global dependant version?) which needs to be swapped for faq.php. Be sure to swap the one near the bottom of the file too. We could swap it for the register_globals off ‘safe’ equivalent ($_SERVER['PHP_SELF']) but that’d mean cleaning it, which can become tedious and is unnecessary here.

Next we have more unvalidated data being checked against the database in the form of a search query. Not only does this have no cleaning of the input, but if a person enters a single letter like “e” into the search box and you have a few thousand questions, your MySQL is going to die trying to recover every instance of the letter “e” in each question, answer and name. I’m not going to bother writing a ‘safe’ search box for the script, but do make sure that you swap the three %$_GET[s]% for %".clean($_GET['s'])."%.

Going down, replace $start_limit = get_setting(numperpage) * $_GET[page]; with $start_limit = get_setting(numperpage) * (int)$_GET['page'];. This ensures that the potential page number really is a number. Lastly, I’d remove $version from the credits line. This discourages script kiddies from exploiting known ‘version’ bugs.

These few changes, while not foolproof and certainly not going to rid the script of the major problems it has, will help to secure it to some degree. Of course, you could go the 100% secure route and ditch the script. I’d recommend PHPAskIt as a replacement — it’s been through rigorous and repeated penetration and exploitation testing, and while no script is 100% secure I’d trust it enough to run it on my website (had I not already wrote my own private FAQ script).

Please note: none of these changes have been approved by Cine. Despite what I would consider an ‘urgent’ e-mail outlining these issues sent to her on the 17th, I’ve received no response. Nonetheless, I believe the security of my visitors is too important to leave dangling in the air (how are you going to link to me if your websites get hacked?!) Further posts outlining issues in other Inexistent scripts will appear at some point.

Jem Turner jem@jemjabella.co.uk +44(0)7521056376

Comments are closed.

Follow on Instagram