Mysidia Adoptables Support Forum - Opinions and Suggestions (Security issue inside)

Mysidia Adoptables Support Forum (http://www.mysidiaadoptables.com/forum/index.php)

- Feedback and Suggestions (http://www.mysidiaadoptables.com/forum/forumdisplay.php?f=25)

- - Opinions and Suggestions (Security issue inside) (http://www.mysidiaadoptables.com/forum/showthread.php?t=1879)

Opinions and Suggestions (Security issue inside)

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.

PHP Code:


		
			
//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...

PHP Code:


		
			
$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...

PHP Code:


		
			
$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

recommendation is to make the backend object-oriented

I agree.

Quote:

Originally Posted by nobackseat (Post 14285)

PHP Code:


		
			
//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...

PHP Code:


		
			
$num = mysql_numrows($result);

PHP Code:


		
			
$isloggedin = $loginstatus[loginstatus];

$loggedinname = $loginstatus[username];

$article_title = $pagecontent[title];

$article_content = $pagecontent[content];

$value[content] = $content;

$value[title] = $title;

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.

Quote:

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

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

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

Quote:

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.

Quote:

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

Quote:

Originally Posted by nobackseat (Post 14334)

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.

Quote:

Originally Posted by nobackseat (Post 14341)

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.

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:

Code:

// clean all our data

$_POST = array_map('secure',$_POST);

$_GET = array_map('secure',$_GET);

And then later, the function:

Code:

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. :)

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