Sign in to follow this  

Only a buffer-overflow is this evil... (Solved)

This topic is 4717 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I've isolated the error in my MMORPG to the account initialization code. The server isn't loading the account archives from my harddrive correctly: the username & password appear as funny characters when they are suppose to be a series of consecutive characters. Debug Output Image Hosted by ImageShack.us This is the structure of the data file.
struct strHook
{
	/* Header Information */
	char bytUsed;
	char bytInGame;
	unsigned short intObject;

	/* Character Information */
	char chrName[15];
	char chrPassword[15];
	char bytType;
	unsigned short intTileX;
	unsigned short intTileY;
	unsigned short intHealth;
	unsigned short intMana;
	unsigned int intLevel;
	unsigned int intSword;
	unsigned int intShield;
	unsigned int intMagic;
	char chrInventory[43];
};


The specific file my server is loading is called "1.reg", and the file contents are shown below. (Note, the account fields are max'd out at 10, but I made them 15 so that I could use the standard string functions. Hence the 5 blank spaces after both the username and password.) Username: aaaaabbbbb Password: cccccddddd Image Hosted by ImageShack.us The code that loads the file data onto the hook structure
/* This function loads all of the accounts into hooks. */
void sAccInit()
{
	FILE *f;
	char chrName[15];
	unsigned short l;
	unsigned short x = 1;
	memset(arrHooks,0x0,sizeof(struct strHook) * 10000);
	strcpy(chrUserFile,"1.reg");

	/* For as long as accounts exist, load them. */
	while(GetFileAttributes(chrUserFile) != 0xFFFFFFFF)
	{
		for(l=0;l<10000;l++)
		{
			if(arrHooks[l].bytUsed == 0)
			{
				f=fopen(chrUserFile,"r");
				fread(&arrHooks[l].chrName[0],sizeof(char)*15,1,f);
				fread(&arrHooks[l].chrPassword[0],sizeof(char)*15,1,f);
				fread(&arrHooks[l].bytType,sizeof(char),1,f);

				fread(&arrHooks[l].intTileX,sizeof(short),1,f);
				fread(&arrHooks[l].intTileY,sizeof(short),1,f);
				fread(&arrHooks[l].intHealth, sizeof(short),1,f);
				fread(&arrHooks[l].intMana, sizeof(short),1,f);

				fread(&arrHooks[l].intLevel, sizeof(int),1,f);
				fread(&arrHooks[l].intSword, sizeof(int),1,f);
				fread(&arrHooks[l].intShield, sizeof(int),1,f);
				fread(&arrHooks[l].intMagic, sizeof(int),1,f);
				fread(&arrHooks[l].chrInventory[0],sizeof(char) * 43,1,f);
				fclose(f);
				arrHooks[l].bytUsed = 1;

				/* Output the name onto the console. */
				memset(chrName,0x0,15);
				memcpy(chrName,arrHooks[l].chrName,10);
				printf("->"); printf(chrName); printf("\n");
				l = 20000;
			}
		}
		/* Set the loop to load the next account. */
		x++;
		sprintf(chrUserFile, "%d.reg", x);
	}
	printf("\n");
}


My guess is that somewhere there is a buffer overflow, since the code doesn't look wrong (least not to me). Any suggestions, or ideas on how I can further isolate the problem? [Edited by - ProcedureX on January 16, 2005 4:37:33 PM]

Share this post


Link to post
Share on other sites
It occurs to me that you are not null-terminating your strings after loading them. I would use binary read/write to files (as opposed to text mode, which has problems with \0) using "rb" instead of "r", and add a '\0' to every string after loading, just to be sure.

Share this post


Link to post
Share on other sites
Arn't I?

"\0" <=> 0
And since those char[15] have 10 occupied values, and 5 values that equal 0, they both have 5 null terminators on the end. [razz]

fread "r" has problems with "\0"? Interesting, I'll google it, and see if those problems apply to my code. [smile]

Share this post


Link to post
Share on other sites
1. You are hoping that the value read from a file is "correct", which you shouldn't (never trust anything you don't have absolute control over, and a file does not qualify for this).

2. If I remember correctly, a value of 0 in text mode is read incorrectly as EOF (I might be wrong, however), which is why I suggested reading in binary mode.

Share this post


Link to post
Share on other sites
Quote:
Original post by ToohrVyk
2. If I remember correctly, a value of 0 in text mode is read incorrectly as EOF (I might be wrong, however), which is why I suggested reading in binary mode.


IC.

Well, I changed my server to read "rb", inaddition, I modified my CGI code to write to "wb" (Since the accounts are generated via a web interface). I recompiled em, and ran em. The output was identical. [bawling]

Share this post


Link to post
Share on other sites
Two things. First of all, you're using fread incorrectly. Your reads should be structured more like this:


//Use the third parameter to specify number of elements
fread(&arrHooks[l].chrName[0],sizeof(char),15,f);

//NOT like this:
fread(&arrHooks[l].chrName[0],sizeof(char)*15,1,f);




Read the description for fread from MSDN for more information.

Secondly, you're only trying to read in 43 characters for the inventory, not 44. You need to set the read count to 44 for the inventory data.

Share this post


Link to post
Share on other sites
Quote:
Original post by Nemesis2k2
Two things. First of all, you're using fread incorrectly. Your reads should be structured more like this:

*** Source Snippet Removed ***


Yup, I read that when I googled the thingy. However, I knew it wouldn't make a different because I've never used it like that before. Nevertheless, because you mentioned it, I changed it. It didn't seem to make any difference though.

Quote:
Original post by Nemesis2k2
Secondly, you're only trying to read in 43 characters for the inventory, not 44. You need to set the read count to 44 for the inventory data.


lol, am I? Didn't notice. Doesn't fix this problem though, but I'm sure you knew that. ;)

Edit: Wow, lol, this is the first time I've posted a thread that wasn't solved within minutes.

[Edited by - ProcedureX on January 16, 2005 3:52:13 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Nemesis2k2
Have you checked to see if fopen is actually succeding in opening the file?


Nice call.

I disected the 'f' and found it was broken up into several components. I did some comparisons with other programs I had written to determine if a specific value changed when it loaded sucessfully/differently. It wasn't apparanent.

Than I replace chrUserFile with "1.reg" and ran the program again, and it loaded the data correctly.

I'm not sure why that fixed it, but at least I have a basis to work with now.

Thanks!
rating++;

Edit: Found it! strcpy(*,*), two pointers, so saying strcpy(array, "1.reg") is invalid! I used sprintf(array,"%d.reg",1) and it fixed it.

Share this post


Link to post
Share on other sites
Well, I suggest that you don't use plain password as you do now, and that you change your name/password strings to std::string, since these are less prone to errors.

So, for your password, I suggest you use MD5 hashes. I used the RSA Data Center C files for this, which you can get here:
http://www.toolmaker.nl/downloads/md5.zip

Add those files to your project, and then add this function somewhere in your "general purpose functions" files:

string MD5(const string& PlainText)
{
MD5_CTX Context;
unsigned char Digest[16];
char HashValue[33];

HashValue[32] = 0;
MD5Init(&Context);

MD5Update(&Context, (unsigned char *)PlainText.c_str(), (unsigned int)PlainText.length());
MD5Final((unsigned char *) Digest, &Context);

// Copy the digest into the string
int Pos = 0;
for (int i = 0; i < 16; ++i)
{
sprintf(&HashValue[Pos], "%02x", Digest[i]);
Pos += 2;
}

return (HashValue);
}



If you don't want to use std::string, it's easy to convert to char*.

Toolmaker

Share this post


Link to post
Share on other sites

This topic is 4717 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this