Any suggestions on the coding of my script would be appreciated

schwim

I asked whether it was ok to post complete scripts(more than just a page) in the sticky at the top of the page, but was never answered, so I'm posting it. If the forum wasn't intended for this, please just let me know and I'll remove the post:

The script I wrote is a testimonial management system. I couldn't find one(everyone seems to make do with a random quote generator), so I have tried to write one myself.

Any suggestions at all are appreciated, but I'm really concerned about script vulnerabilities. I'd hate for people to use it, and end up having problems because of something I've written.

Download

The zip file is just under a meg. Unpacked, it's a little over two(FCKEditor adds a lot to it, the script itself is quite small).

Again, any help you can provide would be greatly appreciated.

thanks,
json

zabmilenko

It's just that you are kind of asking alot.

Tell you what... This Sunday night if I am not busy with work and school, I will make a cursory review of your huge package.

In the meantime, what kind of vulnerabilities are you worried about? Security? Bad coding habits? Also, what file(s) in particular in the package. Anything you can do to save some time would be helpful to me, and in turn, to you.

schwim

Hi there zabmilenko, and thanks very much for the reply.

I understand that it's a lot of trouble to do for a nobody, and I understand completely if it's not worth the hassle for someone to look at. I just thought I would ask, and if nothing happened, I wouldn't have had my feelings hurt.

The main portion of the script consists of:

/index.php
/test_block.php
/form.php
/submit.php
/includes/functions.php
/admin/index.php

and the rest is either the editor or simple included files(for js, etc.). I provided the whole script for two reasons, so you can see the files that I'm attempting to call from the script, and so if anyone wanted to run the script and make suggestions on it's user-friendliness, they could do so.

By vulnerabilities, I mean that I am totally new to writing php scripts, and this is my "hello world", so I worry about weaknesses in the script that would be caused by my code(db queries, forms, etc.) Also, my code is probably very poorly written and formatted. I've tried to format it just like I would HTML, and it looks OK to me, but I bet it's not the way you're supposed to do it. Also, I've tried to comment absolutely everything that might raise a question to outside viewers.

I do appreciate your time, if you can spare it, and I know that it's asking a lot, so like I said, it won't hurt my feelings if people can't spare the time to do it.

If there's anything else I might be able to provide that would help you out, I would be happy to do so, please just let me know.

thanks,
json

zabmilenko

This is a good start. One way or the other you will hear from me by Monday, depending on what time zone you are calling from.

schwim

thanks very much zabilmenko, I appreciate you taking the time. If there's anything I can do to make the task easier, please just let me know.

thanks,
json

zabmilenko

PRELIMINARY CHECK:

Line 82 of index.php pulls $id from $_GET and puts it RIGHT INTO A QUERY without checking it for safety. Someone could inject SQL into your script like this.
Config checks for the $nodb parameter, but then does nothing else with it. Your app doesn't check for it before running queries. Looks like a stab at portability that was poorly implemented throughout. Do you want to allow database disabling?
Instead of using addslashes() for your db input protection, you should use mysql_real_escape_string(). It works with your database server encoding method, which addslashes cannot do.
includes/functions.php line 44. The word "install" is listed as a constant. Is it supposed to be a string? You should quote them if so.
admin/checklogin.php line 53 allows sql injection.
Same file line 68 registers session variables, but I can't find a session_start nearby. Is there some magic going on there?
You appear to use addslashes on your db input, then use the inverse stripslashes on the output. In order for this to make sense, you would have to addslashes twice per field. The first time you use addslashes, it would escape the string for the query. The end result of your overuse of stripslashes is parts of the text disappearing if any backslashes are used.

Use mysql_real_escape_string instead of addslashes, then don't use stripslashes on the output UNLESS you addslashes to the input BEFORE you mysql_real_escape_string. Unless of course magic_quotes is enabled.

EDIT: 8. MD5 appears to be your password encryption method. MD5 is not encryption, and can be reversed. See http://www.md5lookup.com for examples. I usually do not encrypt db passwords (to allow users to send lost passwords to email) but if you want to encrypt them, better to use real encryption. Choice is yours here.

I'll stop here for now and give you a chance to catch up. :-)

Others may have a thing or two to add about my assessment as well.

schwim

Hi there zabmilenko, and thank you so much for taking the time to look at it:

1) What is the best way to protect from this? Simply make sure that the input contains nothing but numbers, with a limit(ie; only 5 characters)?

2) This thread explains it completely, but in short, I needed a way to turn of the db connection just for step 1 of the install script(checking db connection variables), or else they would get an error. Step one sets the value, but nothing else in the script ever uses it. Is this ok?

3) If I use mysql_real_escape_string() to input data, what do I use when I pull the data(synonymous with addslashes)? I checked the php manual for the answer, but I don't see mention of anything. Am I just to use mysql_real_escape_string() again?

4) Do you mean:

if ((@is_readable(install)) && is_dir(install)){

should be:

if ((@is_readable('install')) && is_dir('install')){

? It's intended to check for the existence of the install directory, and stop the script if it's found.

$myusername=$_POST['myusername'];
$mypassword=$_POST['mypassword'];

Same as number 1. What is the best method for protecting against this? There should only be one admin, so maybe the script can match against it and null the input if it isn't what was expected?

6) Unfortunately, I can't answer that, as I simply used a tutorial at PHP Easy Step to build the login system. Should that part be stripped?

7) before I added the stripslashes to my retrieval of the content, my text looked like this when it was printed to screen:

I\'m thinking now that I\'m not so good that this php stuff.

When I added stripslashes to all of the content, that went away. I don't understand how I doubled up on it in any way. Are there instances where stripslashes shows up twice for a variable?

Also, Magic Quotes: Do I have to check for this being enabled on the host server before being able to use it?

(This isn't mine, but instead is from the PHP.net site)

// Quote variable to make safe
function quote_smart($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }
   // Quote if not a number or a numeric string
   if (!is_numeric($value)) {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}

// Connect
$link = mysql_connect('mysql_host', 'mysql_user', 'mysql_password')
   OR die(mysql_error());

// Make a safe query
$query = sprintf("SELECT * FROM users WHERE user=%s AND password=%s",
           quote_smart($_POST['username']),
           quote_smart($_POST['password']));

mysql_query($query);

Is this what I need to do, both for the login and for all data being inserted?

8) I did this only because it's what I've seen in all of the scripts I've worked with. I understand now that it's reversible after looking it up. 🙁 I'll look into other encryption methods.

Thanks again for taking the time to help, and I'm sorry that your effort has so far only been rewarded with more questions.

thanks,
json

zabmilenko

1) If you know that the input is only numbers, simply add (int) in front of the $GET, like such:
$id = (int) $GET['id']; or similar. If it's text input, use preg_match or a match list or similar.

2) Only an inconsistency. Sounds like you have it covered already.

3) You shouldn't have to. The only reason you are escaping is to prevent sql injection. Consider this query:

UPDATE players SET experience = experience + '$new_experience';

There is no issue unless $new_experience contains a quote, like: 0' + '4634345521
If someone was able to malform a piece of text just like that, your query would put it inline, looking like this:

UPDATE players SET experience = experience + '0' + '4634345521';

Which is almost textbook sql injection. Escaping the string turns the malformation to: o\' + \'4634345521
which would make it safe for your query only. You will get pure input into your db, so when you query it back out there will be no need for strip_slashes.

Call it a step saver and a safety net at the same time.

4) Yep

5) Same as 1. Just use mysql_real_escape_string on the $_POST values just to be safe. Also, recommend checking it for length using strlen, because php would probably allow your 2 meg query to get executed if someone wanted to DoS you.

6) Skip it. If it works, don't worry about it for now.

7) Thats a matter of implementation. stripslashes is not the opposite of mysql_real_escape_string, and addslashes is a poor choice for db escaping. My guess is that magic quotes is enabled. I would check for magic quotes AFTER the select query and only stripslashes if it's on. PHP6 will remove magic quotes altogether so if you have control over your php.ini, I would disable it.

8) Plain old crypt() works wonders compared to md5 and sha1 without having to load additional extensions.

Don't feel bad. You sound like you genuinely want to learn something and you are asking smart questions. I feel better about helping you out because of this (it's refreshing).

Good luck.

schwim

Hi there zabmilenko,

I've made all of the changes you had commented on(aside from the encryption, I need to learn more about that), and if you had more to comment on, I'm all ears.

I know it could become a never ending process, so if you feel that the script will suffice in the wild as it stands, I'm a-ok with that.

You can grab the updated script here.

I just wanted to say thanks very much for the help you've already provided. I have learned a bunch in regards to thinking ahead when it comes to security. It seems that the hardest part of writing a script is protecting against any form of abuse. It was very helpful to see things like the ID being passed unchecked and the needed limitations on string sizes for logins and forms. It got me looking into other areas and reading up on added security measures.

thanks again,
json

zabmilenko

:-) Yeah, once again give me until Sunday night and I will take another look.