lockdown source

[brew]

brew: considering how concerned you are about manual optimization, I find it a little surprising you have not even considered security.  Read up about buffer overflows, look at how many patches Microsoft has to issue because their programmers do not understand buffer overflows, then look at your code again.  If you still think it is OK, then I will pick it apart and explain what is wrong.

To be completely honest, I don't see anything wrong with that code. Maybe it is insecure, but honestly who cares. Not like everybody's going to be using it anyways. Anyways, I can't really think of a problem with that code. Please clue me in :9

[Kp]

To be completely honest, I don't see anything wrong with that code. Maybe it is insecure, but honestly who cares. Not like everybody's going to be using it anyways. Anyways, I can't really think of a problem with that code. Please clue me in :9

You should care, as should anyone who runs it or uses it as an example.

• None of your string copies are checked in any way, so if any of the inputs are too long, you will copy past the end of the allocated buffer.  Once you have a buffer overrun, various things can happen.  Since you are using an antiquated compiler without support for SSP, a buffer overrun can lead to arbitrary code execution.  In this case, some of your inputs are clearly derived from data furnished by the server.  Based on your responses thus far, I will assume that you do not have parameter checking code elsewhere.  If that assumption is correct, then any server you connect to, whether battle.net or some rogue third-party BNCS emulation server, could take control of your program just by sending you a specially crafted packet.  Once this happens, the attacker can do anything your user account can do.
• You are using sprintf, rather than snprintf.  This is another form of unchecked string transfer.
• Your subscript operation on mpqName assumes the MPQ name will be at least four elements long.  If it is less, you will overwrite part of the variable which precedes mpqName in memory.
• You are allocating from the heap for a fixed number of fixed size small buffers.  Why bother?  If the allocations are predictable and small, put them on the stack to simplify the logic.
• Your code is not const-correct.  The variable 'data' is never written to, but is not marked as const.
• Your code is not Unicode-correct.  You are using the TCHAR form of GetCurrentDirectory, but the argument is a char.  You should use GetCurrentDirectoryA.

Never assume that external input can be trusted to be correct.  Always validate it, even if you think it will come from a source that would not have an interest in breaking your program.  Expect that eventually, you will make a mistake.  Design your code to minimize the opportunity to make dangerous mistakes.  For instance, even though an input is coming from a function which already validated it, you should still use a checked copy.  Then, if you someday add a path where the input can come in without being validated, the checked copy will still catch an attack.

[brew]

How would I access the "NX" or "XD" features of a CPU? That way, if i did have a buffer overrun, it wouldn't do anything harmful except overwrite a variable (maybe)? And what about DEP?

Your code is not const-correct.  The variable 'data' is never written to, but is not marked as const.

What is the benefit of marking a variable as a const, anyways?

Also wouldn't a buffer overflow just (usually) result in an unhandled memory access violation exception?

[Yegg]

If you mark a variable as const, the compiler won't let you modify it, thus keeping you from modifying variables that you didn't intend on modifying.

[Newby]

Also wouldn't a buffer overflow just (usually) result in an unhandled memory access violation exception?

No... if the payload is constructed properly it could do so much more.

[warz]

Depending what it overflows into, anyways.

[iago]

How would I access the "NX" or "XD" features of a CPU? That way, if i did have a buffer overrun, it wouldn't do anything harmful except overwrite a variable (maybe)? And what about DEP?

Your code is not const-correct.  The variable 'data' is never written to, but is not marked as const.

What is the benefit of marking a variable as a const, anyways?

Also wouldn't a buffer overflow just (usually) result in an unhandled memory access violation exception?

You should read the classic paper "Smashing the Stack for Fun and Profit" by Aleph1 (aka, Elias Levy, who works at Symantec ). It'll explain in gory details why that isn't necessarily true.

[Yegg]

You can get that paper by DJ_Ripper via IRC in #ebooks on irc.tehnet.org/6667.

[Kp]

How would I access the "NX" or "XD" features of a CPU? That way, if i did have a buffer overrun, it wouldn't do anything harmful except overwrite a variable (maybe)? And what about DEP?

Your operating system is responsible for enabling the feature and configuring the hardware appropriately.  If I recall correctly, NX support is present in Windows XP SP2 and Windows Vista.  It is probably supported in some version of Windows Server 2003, but I do not know which Service Pack introduced it.  Even when DEP is on, Windows implements it in a way that is not that hard to bypass, which was necessary for compatibility with stupidly written copy prevention schemes.  Finally, even supposing that DEP cannot be bypassed, an attacker can still transfer control to any other point in your program.  Depending on the program, this could do any of a variety of bad things.

Think of DEP like the safety features on a car: good to have, will probably reduce the damage you sustain if you ever need it, but you are still better off just not needing it in the first place.