Opinions and Suggestions (Security issue inside)

[nobackseat]

First off, I wanted to say that I have worked with several clients using the Rusnak script, and the code in general is just really horrid.

With that said, I have a few things to point out, just from looking over the latest update for 5 minutes.

//here the user posts a comment
$comment = $_POST["comment"];
if ($comment != "") {
    $date = date("Y-m-d H:i:s");
    // $date = "10-23-3 21:02:35";
    $user = $loggedinname;
    if ($isloggedin!="yes") {
        $user = "Guest";
        }
    $comment = $comment;
    mysql_query("INSERT INTO ".$prefix."shoutbox VALUES ('', '$user', '$date', '$comment')");

So, it appears $comment is directly inserted into the database, with no protection, with the exception of if it is empty...

Also, why is $comment assigned to itself in there?

Anyways, next...

$num = mysql_numrows($result);

I believe mysql_numrows() is deprecated. So why is it being used on a script that is freely available to public? Which, of course, which would be run on all different types of systems and PHP versions. You can't guarantee compatibility...

$isloggedin = $loginstatus[loginstatus];
$loggedinname = $loginstatus[username];
$article_title = $pagecontent[title];
$article_content = $pagecontent[content];
$value[content] = $content;
$value[title] = $title;

I haven't dug deep enough into the script, but I'm assuming that $loginstatus, $pagecontent and $value are arrays.

I see this mistake done everywhere, and it is quite a frustrating habit to see other people make. Put quotes in the brackets! Else, it has to check if it is DEFINE'd and what not. Generally bad practice, and it is tremendously slower.

Additionally, I believe seeing the encryption was MD5(). Seriously? Not even a salt? Yeah I suppose double MD5 is minorly safer, but that is just silly. MD5 was created in 1991... upgrade much?

Also, one last thing. For your major 2.0 release, I highly recommend that you re-do the template system if you can. In fact, a general recommendation is to make the backend object-oriented so it is easier for developers like us to build "mods" and what not.

Thanks,

NBS

 

[Tony]

recommendation is to make the backend object-oriented

I agree.

 

[Kaeliah]

First off, I wanted to say that I have worked with several clients using the Rusnak script, and the code in general is just really horrid.

With that said, I have a few things to point out, just from looking over the latest update for 5 minutes.

//here the user posts a comment
$comment = $_POST["comment"];
if ($comment != "") {
    $date = date("Y-m-d H:i:s");
    // $date = "10-23-3 21:02:35";
    $user = $loggedinname;
    if ($isloggedin!="yes") {
        $user = "Guest";
        }
    $comment = $comment;
    mysql_query("INSERT INTO ".$prefix."shoutbox VALUES ('', '$user', '$date', '$comment')");

So, it appears $comment is directly inserted into the database, with no protection, with the exception of if it is empty...

Also, why is $comment assigned to itself in there?

Anyways, next...

$num = mysql_numrows($result);

I believe mysql_numrows() is deprecated. So why is it being used on a script that is freely available to public? Which, of course, which would be run on all different types of systems and PHP versions. You can't guarantee compatibility...

$isloggedin = $loginstatus[loginstatus];
$loggedinname = $loginstatus[username];
$article_title = $pagecontent[title];
$article_content = $pagecontent[content];
$value[content] = $content;
$value[title] = $title;

I haven't dug deep enough into the script, but I'm assuming that $loginstatus, $pagecontent and $value are arrays.

I see this mistake done everywhere, and it is quite a frustrating habit to see other people make. Put quotes in the brackets! Else, it has to check if it is DEFINE'd and what not. Generally bad practice, and it is tremendously slower.

Additionally, I believe seeing the encryption was MD5(). Seriously? Not even a salt? Yeah I suppose double MD5 is minorly safer, but that is just silly. MD5 was created in 1991... upgrade much?

Also, one last thing. For your major 2.0 release, I highly recommend that you re-do the template system if you can. In fact, a general recommendation is to make the backend object-oriented so it is easier for developers like us to build "mods" and what not.

Thanks,

NBS

First off, we just got this script and have not yet had time to go through and fix all these errors. Many of these things have already been corrected for the next release(which has not been released yet as we're still working on it). While I appreciate you bringing these things to our attention, please understand that this is a FREE script being worked on completely by VOLUNTEERS, so progress may not be as quick as some may like. The next release, 1.2.* will happen when our Development team is satisfied with the changes, and the additions to the script. Until then, please be patient.

 

[nobackseat]

Kaeliah,

Please, tell me what made you treat my suggestion post differently than others?

I don't recall mentioning that I was impatient. Or that this script should charge money as well as the staff?

Many of these things have already been corrected for the next release

Then there was clearly no reason to get defensive, other than that of you feeling talked down to. Which I did not do intentionally.

I was just sharing my honest thoughts. It was for your benefit, as I am not anticipating the next release of the script...

I was not directly blaming you or the 'development team', for the obvious faults of the original developer.

NBS

 

[Hall of Famer]

Umm this may just be my interpretation. Most of those programming flaws were made a long time ago by BMR, the script's creator. It is fine if you bring it up to me in a private message, but I do think its unfair for my dev staff to have to take this blame that is supposed to be on BMR(In fact, no one blames BMR for this, we pretty much realize that there are mistakes made before and hope to correct them one by one). Dont you think so?

But yeah, we are indeed working on it and trying our best to improve it over the next a few weeks and months. Fixing all those stuff will take some time, and I hope you understand and give us more time before making a judgment on the direction this script goes. I appreciate your interest in Mys, have fun!

Hall of Famer

 

[nobackseat]

You mustn't of taken the time to read my post.

I was not directly blaming you or the 'development team', for the obvious faults of the original developer.

She assumed that it was directed at her/her team.

Fixing all those stuff will take some time, and I hope you understand and give us more time before making a judgment on the direction this script goes.

Again, what is with this time restraint that I am apparently implying?

Obviously, in aiming to assist the direction of the script, I created this thread.

NBS

 

[Kaeliah]

Kaeliah,

Please, tell me what made you treat my suggestion post differently than others?

I don't recall mentioning that I was impatient. Or that this script should charge money as well as the staff?



Then there was clearly no reason to get defensive, other than that of you feeling talked down to. Which I did not do intentionally.

I was just sharing my honest thoughts. It was for your benefit, as I am not anticipating the next release of the script...

I was not directly blaming you or the 'development team', for the obvious faults of the original developer.

NBS

I apologize, I took the post kind of personally and felt as though I(and the rest of the Dev Staff) was being blamed for the serious issues with the original script, or at least being blamed for not having fixed them yet... My apologies, I didn't mean to sound rude or anything, although I apologize for being defensive.

 

[Hall of Famer]

You mustn't of taken the time to read my post.



She assumed that it was directed at her/her team.



Again, what is with this time restraint that I am apparently implying?

Obviously, in aiming to assist the direction of the script, I created this thread.

NBS

I did read your post, but that statement was not in your original post(yeah, you wrote a reply later to clear this confusion, thanks). I was just stating a possibility why Kaeliah found your thread offensive, she replied to it before you explained you were not blaming this current dev staff team for what BMR did years ago.

 

[Arianna]

The script in general is pretty bad, and we're trying to improve it. The shoutbox script, if I'm not mistaken, is made by me - and I have to admit, it's a pretty bad error. However, in future versions, I have the following code in functions.php:

// clean all our data
$_POST = array_map('secure',$_POST);
$_GET = array_map('secure',$_GET);

And then later, the function:

function secure($data) {
	//This function performs security checks on all incoming form data
	if(is_array($data)) {
		die("Hacking Attempt!");
	}
	$data = htmlentities($data);
	$data = mysql_real_escape_string($data);
	$data = strip_tags($data, '');
	return $data;
}

So rather than sanitizing the $_POST data every time we get a variable, it just does it once.

As for object-oriented-ness, I actually previously recoded the whole script in OOP for my own site. The thing is that people have been working on the current version, so either the edits made to this would need to be scrapped, or it would need to be programmed from scratch. :/

Anyway, you make very good suggestions, especially because the script is really flawed in the first place. As Kaeliah and HoF have already said, a lot of these have been/are being corrected for 1.2.x, but some issues you brought up haven't been thought over in detail yet. Thank you.

 

[nobackseat]

Hello Arianna!

Thanks for the reply.

Your approach is "bad" for a few reasons...

1) The site already manually protects variables, so the variables that are used, would be escaped twice.
2) You left out $_COOKIE
3) More information would be escaped than would be needed, so it is much slower, as are arrays in general.
4) What if, say for user profile input, you wanted to permit certain HTML tags or something (not BBCode)? Since the input is stripped already, there isn't much you can do.
5) Encourages bad practices; user who learned PHP from Mysidia, may leave the part out of the code, following their habit of simply putting it directly in queries.

Make sense?

NBS

 

[Arianna]

Thanks for bringing those up, NBS. As far as I can see,

1) Well, yeah, but in previous things where I've used this, there isn't any escaping in the first place.
2) Ooops. :/ Might as well add that.
3) Hmm, I don't see why. Assuming the script always uses all of $_POST and $_GET (which is usually does), it needs to secure everything in them.
4) Well, that's a very valid point, only currently, there isn't anything in the script which requires this. I do get that this could be an issue in the future, but for now it's okay. xD

It mostly makes sense, though. I use this approach because I hate having to secure variables from forms before using them, because I inevitably end up forgetting about them. xP

 

[nobackseat]

because I inevitably end up forgetting about them. xP

I understand completely and this is a very common problem.

Which is why I use a database class...

As for object-oriented-ness, I actually previously recoded the whole script in OOP for my own site. The thing is that people have been working on the current version, so either the edits made to this would need to be scrapped, or it would need to be programmed from scratch. :/

Yeah, I understand. I was just throwing that out there. Something to work towards eventually hopefully.

NBS

 

[nobackseat]

Sorry to double post, but just adding to my original post...

I have noticed the excessive use of mysql_result. Perhaps it is because that is the only function that one may know, or one copies and pastes from the current script. In either case, it is pretty bad. mysql_result is not the best option in most cases, simply because its operation is resource intensive. In fact its use is only recommended for SELECTing ONE column (from what I've gathered with colleagues).

I highly recommend mysql_fetch_array. Look into it if you are interested.

Last thing, and I think this is a huge issue, is so many people are suppressing errors, well, eveywhere. This is extremely bad practice. Seriously, if you are that paranoid about an error showing, then create an error handler...?

It is bad practice, besides the obvious, because sometimes if you change the code that the suppressed line is dependent on, there is a good chance the interpreter will output a different error (and line number) than the one you were trying to ignore. Then how do you find out what is wrong?

Just my two cents.

NBS

 

[Arianna]

We're already doing our best to switch to mysql_fetch_array - the problem with this kind of feedback (it's just a minor problem, but still) is that we're working on a really updated version behind the scenes and so it's hard to know what we have and haven't done. :/

Anyway, an error handler does sound like a good idea. I'll look into that soon (or if anyone else on the dev team feels like it, then they can).