Random values being added to string with a html post type function

I have a post function that was made by someone else (an advanced coder) which works fine. I'm using this function to interact with php files and an online database. Theirs a file that does all the database interaction on the site so the database information isn't leaked out. The file i created for this purpose works fine the way I made it to but for some reason after adding a new case to it, it won't work for that case. Everything else works the way it should. When I open the debug file (wrote the content from the post function to it) their is 4 random characters on the first line, the actual data that comes up on the second, a random character on the third and fourth, and a couple blank lines under it.

Post function:

function post($url, $data, $ref = "") {
$url = parse_url($url);
$http = fsockopen($url['host'], 80, $en, $es, 45);
if($http) {
fputs($http, "POST ".$url['path']." HTTP/1.1\r\n");
fputs($http, "Host: ".$url['host']."\r\n");
if($ref != "") { fputs($http, "Referer: ".$ref."\r\n"); }
fputs($http, "Content-type: application/x-www-form-urlencoded\r\n");
fputs($http, "Content-length: ".strlen($data)."\r\n");
fputs($http, "Connection: close\r\n\r\n");
fputs($http, $data);
while(!feof($http)&&!is_bool($http)) {
@$result .= fgets($http, 128);
}
} else {
return array(
"status" => "error",
"error" => "(".$en.") ".$es
);
}
fclose($http);
$result = explode("\r\n\r\n", $result, 2);
$header = (isset($result[0])) ? $result[0] : false;
$content = (isset($result[1])) ? $result[1] : false;
return array(
"status" => "ok",
"header" => $header,
"content" => $content
);
}


remoteDB.php (the bridge on the site)

<?php
if(isset($_POST['process'])){
//Database information
$sHost = "localhost";
$sUser = "";
$sPass = "";
$sDatabase = "";
mysql_connect($sHost,$sUser,$sPass) or die(mysql_error());
mysql_select_db($sDatabase) or die (mysql_error());

//Get the variables and set them again
foreach($_POST as $k => $v){
if(isset($v)) ${$k} = $v;
}
unset($process);//Value only needed to submit the request
if(!empty($password)) $password = md5(md5($password));
/*
$username
$password
$botID
$action
$table
$column
$value
*/

/*
//Debug
print "Username: $username<br />";
print "Password: $password<br />";
print "botID: $botID<br />";
print "action: $action<br />";
print "Table: $table<br />";
print "Column: $column<br />";
print "Value: $value<br />";
*/
function getAccount($user,$pass){
$query = getDB("users","id","username='{$user}' AND password='{$pass}'");
$num = mysql_num_rows($query);
if($num!=1) return false;
$account = mysql_fetch_assoc($query);
return $account;
}
function getBot($account,$botID){
$query = getDB("bots","*","id='{$botID}' AND owner='{$account['id']}'");
$num = mysql_num_rows($query);
if($num!=1) return false;
$bot = mysql_fetch_assoc($query);
return $bot;
}
function updateDB($column,$value,$where = null){
if($where!=null) $where = "WHERE ".$where;
mysql_query("UPDATE bots SET $column='{$value}' {$where}") or die(mysql_error());
}
function getDB($table,$column,$where = null){
if($where!=null) $where = "WHERE ".$where;
$query = mysql_query("SELECT {$column} FROM {$table} {$where}") or die(mysql_error());
return $query;
}

$account = getAccount($username,$password);
if($account === false) die("Login failed!");
if(!$action) die();
switch($action){
case 'checkBot':
$bot = getBot($account,$botID);
if($bot === false)
die("Validation failed!");
elseif($account['id']==$bot['owner'])
die("Validation passed!");
else
die("An unknown error occured.");
break;
case 'getDB':
switch($table){
case 'access':
$result = array();
$query = getDB($table,'*');
while($row = mysql_fetch_assoc($query)){
$result[$row['userid']] = $row['rank'];
}
break;
case 'bots':
$query = getDB($table,$column,"id={$botID}");
$results = mysql_fetch_assoc($query);
if($column=='*') $result = $results;
else $result = $results[$column];
break;
case 'madgabList':
$result = array();
$query = getDB($table,'*');
while($row = mysql_fetch_assoc($query)){
$result[$row['phrase']] = $row['solution'];
}
break;
case 'powers':
$result = array();
$query = getDB($table,'*');
while($row = mysql_fetch_assoc($query)){
$result[$row['id']] = $row['name'];
}
break;
case 'wordList'://Happens with this one
$result = array();
$query = getDB($table,'*');
while($row = mysql_fetch_assoc($query)){
$result[$row['id']] = $row['word'];
}
break;
}
break;
case 'updateDB':
switch($table){
case 'bots':
updateDB($column,$value,"id={$botID}");
die("Updated!");
break;
}
break;
default:
die;
}
die(print_r($result)); //Debug
print serialize($result);
die;
}
?>
<html>
<body>
<form method="post">
<table>
<?
$postArray = array('username','password','botID','action','table','column','value');
foreach($postArray as $name){
print "<tr>";
print "<td>{$name}</td>";
switch($name){
case 'password':
print "<td><input type='password' name='{$name}' value='{$_GET[$name]}'/></td>";
break;
case 'action':
print "<td><select name='{$name}'>";
print "<option value='checkBot'>checkBot</option>";
print "<option value='getDB'>getDB</option>";
print "<option value='updateDB'>updateDB</option>";
print "</select></td>";
break;
default:
print "<td><input type='text' name='{$name}' value='{$_GET[$name]}'/></td>";
}
print "</tr>\r\n";
}
?>
<tr>
<td><input type="submit" name="process"/></td>
</tr>
</table>
</form>
</body>
</html>


When the 'die(print_r($result));' happens, the array is printed as desired. Their is no extra characters before or after it either. When I print the serialized version of it (used to pass the data from one file to the other), their is still no data before or after it. The post function hasn't changed at all and all the other actions work properly but this one. Any ideas?

The array should be something like this:

$wordList = array(
1 => 'word1',
2 => 'word2',
3 => 'word3'
);


When serialized, it should be like this:

a:3:{i:1;s:5:"word1";i:2;s:5:"word2";i:3;s:5:"word3";}


When it's saved to the debug.txt it looks something like this:

f67a
a:3:{i:1;s:5:"word1";i:2;s:5:"word2";i:3;s:5:"word3";}
0
*extra line*
*extra line*

I have a post function that was made by someone else (an advanced coder) which works fine.

Is this the same person you speak of here?:


Also with the unset part, an advanced php coding friend of mine who originally coded this bot told me to use array_diff and not unset because unset just clears the value of the memory while array_diff unallocates it after clearing it, freeing up space

If so I wouldn't really go with their code and / or advice every time as it's plagued you previously and no offence intended but you've been taken in by their 'advanced' coding skill before.

Also I'd not personally be using fputs, that code looks a bit long winded and sloppy for my liking. cURL makes life a lot easier and allows for much neater ad easier to read and debug code.

Additionally when writing my remote script I had similar troubles to yours and narrowed it down to corruption of the data being transmitted. I ended up sending it as base64 encoded and then decoding it at the other end.

Why do you keep feeling the need to praise your coding friend up as 'advanced' ? - Are we not so worthy or something? (even though we're the ones you turn to to fix their code!) :confused:

At what point is this written to a file? I don't see post called anywhere in here unless I'm missing it completely.
Also, I'm not sure exactly what the issue is here. Is it with what is written as the result of the post function? What have you done to it prior to writing? You can't just write an array directly to a file, and the post returns an array.

Also, there are quite a few other things to mention. The first is:

foreach($_POST as $k => $v){
if(isset($v)) ${$k} = $v;
}

Register globals were removed for a reason. Recreating them is not a great idea.

username='{$user}' AND password='{$pass}' is a security hole, which is across the board in these queries. These have to be escaped with mysql_real_escape_string or bound using MySQLi or PDO.

Is this the same person you speak of here?
Yes, actually. I don't know why I assume that he's so advanced. It's probably because I've seen the output of his work before and it was pretty nice. He also told be he took advanced php classes but he must have missed a lesson or two xD

I'm not too familiar with cURL and from what you've said, you have used something like this before. I don't want to try to wrote something like this and have it be buggy in the long run after I release what I'm working on (I am looking for a way to update it with an easy tool of some type if you know of any :p). Would you be able to assist me with this or post a function you wrote before that would work the same way? I have little experience with coding with the cURL and all the requesting and posting and all that the way he wrote it. If not then I guess I'll just have to spend a lot more time learning something for a little problem. I do intend to learn it at some point but I want to finish what I'm doing so I can release it publicly. Thanks for the help!

P.S. My friend also mentioned that he was working on a new style of coding when coding this project and that's why it was written with so many bugs. He also told me he added some bugs intentionally (simple things like swapping lines around) when releasing it so only people that knew how to code could fix it just by looking at it.

P.S. My friend also mentioned that he was working on a new style of coding when coding this project and that's why it was written with so many bugs. He also told me he added some bugs intentionally (simple things like swapping lines around) when releasing it so only people that knew how to code could fix it just by looking at it.

I'd call BS on that. No programmer purposely puts bugs in their code - especially security ones. Employees in some organizations believe coding in such a fashion grants job security, but I assure you they are wrong and that just makes people like me more money :).
Indicating a bug as a "feature" is not the signs of an advanced developer.

I'd call BS on that. No programmer purposely puts bugs in their code - especially security ones. Employees in some organizations believe coding in such a fashion grants job security, but I assure you they are wrong and that just makes people like me more money :).
Indicating a bug as a "feature" is not the signs of an advanced developer.

To be more specific on the bugs he added, it was with the bot's actions (yes the project is a bot). He simply added stuff such as when you sent a command the bot would try to join a chat before connecting the server (swapped the lines and put the join after the connect). The commands actuall worked before the release too when he had it hosted online through a VPS. He had nothing to do with the remoteDB file. All that was added by me so the bot's information could be saved on an online database and could be accessed from another computer if you moved without having to bring anything but the program and if you forgot it, you could just redownload it again.

Also, there are quite a few other things to mention. The first is:

foreach($_POST as $k => $v){
if(isset($v)) ${$k} = $v;
}

Register globals were removed for a reason. Recreating them is not a great idea.

I just did this out of lazyness because it was easier for me and I didn't have to worry about changing much. Is their a different thing I should use in place of this?

Also, the debug.txt was written in the actual bot script. Each time it got a value from the database it would save the page to the text file. It did overwrite itself each time but when it got to the bugged load it stopped so it was the last to write.
The security holes with the login was my fault because I learned from youtube (not the best place). I'll add the patches soon to make sure its safer, but even if someone got through the only thing on the other side is settings so it wouldn't help them much unless they wanted to mess with the settings.

**EDIT**
I attempted to do an SQL injection from the following example

<?php
// We didn't check $_POST['password'], it could be anything the user wanted! For example:
$_POST['username'] = 'aidan';
$_POST['password'] = "' OR ''='";

// Query database to check if there are any matching users
$query = "SELECT * FROM users WHERE user='{$_POST['username']}' AND password='{$_POST['password']}'";
mysql_query($query);

// This means the query sent to MySQL would be:
echo $query;
?>

Found here http://php.net/manual/en/function.mysql-real-escape-string.php
I'm still going to add it to the getAccount function however just to be on the safer side.

I also just noticed another way I patched it that you may have looked over. As soon as the arrays are set, $password is set again to a double md5 encryption which causes the injections to get encrypted to an md5 hash which then ruins the ability to SQL inject it :p (unless their are other ways o_O)

I just did this out of lazyness because it was easier for me and I didn't have to worry about changing much. Is their a different thing I should use in place of this?


Yeah, don't use globals.
But if you are intent on being lazy and not verifying/validating, at the very least use the extract function since you can prefix or deny write on a collision.

I see a lot more than settings here. I see users and other tables. Never underestimate the destruction that can be caused by acquiring the control of another's database.


As for your injection test, it doesn't matter if you can or not. One of the reasons why it may not is if you have magic_quotes_gpc in use which was a (horrible) idea to add to PHP to prevent injections automatically.
I expect magic_quotes_gpc to be terminated in PHP 6 which will leave a lot of sites open for the taking.
Stripslashes the data if magic_quotes is in use first, then use mysql_real_escape_string to escape it.

I see a lot more than settings here. I see users and other tables. Never underestimate the destruction that can be caused by acquiring the control of another's database.

You are right, their is more than the bot's settings. The users table holds the username, password, and id of the account. powers holds a list of names and id's of the powers for the site the bot is for which is only used to assign them to the chat. The access table is a table that stores a list of access to all the bots (universal access) mainly for developers and support. And finally the lists are just a giant table of words used for games. I do have the file that I made to create the list (the words were already typed in an array by the previous developers and I was not going to type over 2 thousand words into one table and then over 100 madgabs into another) still in case i need it for some reason.



As for your injection test, it doesn't matter if you can or not. One of the reasons why it may not is if you have magic_quotes_gpc in use which was a (horrible) idea to add to PHP to prevent injections automatically.
I expect magic_quotes_gpc to be terminated in PHP 6 which will leave a lot of sites open for the taking.
Stripslashes the data if magic_quotes is in use first, then use mysql_real_escape_string to escape it.

Wouldn't a str_replace also work to strip out all the characters that aren't alphanumerical?

I'd call BS on that. No programmer purposely puts bugs in their code - especially security ones.

...

Indicating a bug as a "feature" is not the signs of an advanced developer.

Just what I thought when I read that.

Dubz, you need to stop relying on this friend if you want to learn PHP properly. They're a s**t programmer and they're making excuses for being bad. Either that or you're making excuses on their behalf.

Just what I thought when I read that.

Dubz, you need to stop relying on this friend if you want to learn PHP properly. They're a s**t programmer and they're making excuses for being bad. Either that or you're making excuses on their behalf.

He knew some of the bugs were intentional. I found them and they were an easy line swap fix (the ones he purposely added). He was also coding with someone else who always wanted to take the lazy way out. I don't know whose fault it was for some of the unintentional bugs and I know that he isn't a perfect coder (no one is) but he does know a lot more than me with it and e has good ideas on how to do some things. I'm not trying to make him seem better than he really is but he does have decent coding skills he's just careless with it.

Yeah, don't use globals.
But if you are intent on being lazy and not verifying/validating, at the very least use the extract function since you can prefix or deny write on a collision.

I looked on php.net and it said not to extract untrusted data like user input
http://php.net/manual/en/function.extract.php

The way I have it set works fine for whats needed and I don't see what the problem is with it. I made sure to check the code and its pretty strict on what the user can do with their view of it. Yes I did come across some useful tips that may help but as far as i can see the only problem I have at the moment is the post action. Heres what the bot does with the getDB and updateDB


function updateDB($column,$value,$table = 'bots') {
global $account,$remoteDB;
$result = $this->post($remoteDB,"process=yes&username={$account['username']}&password={$account['password']}&botID={$account['botID']}&action=updateDB&table={$table}&column={$column}&value={$value}",$remoteDB);
//Important to let you know if it passed or failed. Removing it will only remove the results message.
$errorCheck = array(
"Login failed!",
"Validation failed!",
"An unknown error has occured!"
);
foreach($errorCheck as $error){
if(strpos($result['content'], $error) > -1){
echo "\r\n".$error."\r\n";
//Make sure you still have access to change your bot's settings in the db
//Should only hapen if you change your account password when running the bot
die("Please relogin again.");
}
}
}
function getDB($column,$table = 'bots') {
global $account,$remoteDB;
$result = $this->post($remoteDB,"process=yes&username={$account['username']}&password={$account['password']}&botID={$account['botID']}&action=getDB&table={$table}&column={$column}",$remoteDB);
//Important to let you know if it passed or failed. Removing it will only remove the results message.
$errorCheck = array(
"Login failed!",
"Validation failed!",
"An unknown error has occured!"
);
foreach($errorCheck as $error){
if(strpos($result['content'], $error) > -1){
echo "\r\n".$error."\r\n";
//Make sure you still have access to change your bot's settings in the db
//Should only happen if you change your account password when running the bot
die("Please relogin again.");
}
}
$fw = fopen('debug.txt','w+');
fwrite($fw,'');
fwrite($fw,$result['content']);
fclose($fw);
print("\r\nWrote data to debug.txt!\r\n");
$value = unserialize($result['content']);
return $value;
}


The remoteDB was also tweaked because i noticed only the account was needed to update the database (meaning you could update any bot's database as long as you had an account because it didn't check to make sure they had access to the bot)

He knew some of the bugs were intentional. I found them and they were an easy line swap fix (the ones he purposely added). He was also coding with someone else who always wanted to take the lazy way out. I don't know whose fault it was for some of the unintentional bugs and I know that he isn't a perfect coder (no one is) but he does know a lot more than me with it and e has good ideas on how to do some things. I'm not trying to make him seem better than he really is but he does have decent coding skills he's just careless with it.

He's bluffing you. As for decent coding skills, if you have decent coding skills you're not careless with them unless you're a bad programmer.

Stop making excuses for him. I've seen other php coders who think they're good at php programming, when I see their code its nothing but pure slop.

I remember helping one guy out over teamviewer. He was an american and thought he was the best at coding. I took one look at his code and realised I had a mammouth job on my hands. Not only had he duplicated the code in multiple files (making updates hard) he had pretty much filled the code with nothing but if else clauses all over the place and was mixing so much php and html that it really was hard to understand. He thought he was a good coder. I had to teach him a thing or two. I shot myself in the foot there really as he was a good payer and I ended up showing him other techniques to make his coding better and never got another job from him again :mad:

He's bluffing you. As for decent coding skills, if you have decent coding skills you're not careless with them unless you're a bad programmer.


I'm not trying to make excuses for him and I knew he added some after he told me because the code worked when he added it up but he just kind of disabled some things from working. When he had it running on his VPS the commands were fine but when he released it they wouldn't and it was because he just moved the join before connect instead of after it. I know he isn't the best and he made a few mistakes but the technique he used was pretty good. I think a lot of the errors was from the other coder he worked with because I've seen him work on teamviewer and although he can get it to work, it's not pretty looking after. I can't prove whose fault it was for some of the bugs but they both had their share of them. I do know they should have been more careful with the code but sometimes you have a bad day.