I'm doing some network programming, and I'd like you to review a small piece of code. I want to know if this is safe (regarding bufferoverflows and other security risks).

I'm getting an unsigned char* data and an unsigned int dataLength from a networking API. This is the data from the packet received. The data parameter SHOULD contain (in order):
a message type byte
a null-terminated account number string
a null-terminated password string.

..but ofcourse an attacker could put anything in there.

I'm also having a strange issue with the std::string. When I create it in one way, it works, and in another way it doesn't, see below.

Any other suggestions (like an alternative to the cast maybe?) are welcome too.

Thanks

03-30-2008

CornedBee

The code is unsafe.

std::string doesn't have a constructor that takes (char*, int, int), which is what you pass.
It has one that takes (char*, size_t), a data pointer and a length.
It has one that takes (std::string, size_t, size_t), an existing string, an offset and a length.

Because you pass three arguments, only the three-argument constructor is considered. Conversion of int to size_t is safe in this case. However, the char* is implicitly converted to a temporary std::string, and that's not only definitely unsafe (if the attacker sends no nul byte, the constructor will run off the buffer), it also doesn't work correctly, as you've found out.

03-31-2008

C+/-

Thanks, how about this:

I've changed the packet format a bit. The first byte is still the message type, then followed by 6 characters for the account number, followed by 1 to 20 characters for the password. So the minimum packet size is 8 bytes, and maximum 27 bytes.