Safe string copying

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.

Code:

---

int f(unsigned char* data, unsigned int dataLength)
{
    std::string acc(reinterpret_cast<char*>(data), 1, 20);
    std::string pass(reinterpret_cast<char*>(data + acc.length() + 2), 0, 20); //works
    //std::string pass(reinterpret_cast<char*>(data), acc.length() + 2, 20);    //doesn't work
   
    std::cout << acc << " " << pass << std::endl; // for testing only.
    return 0;
}

---

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

Thanks

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.

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.

Code:

---

int f(unsigned char* data, unsigned int dataLength, SystemID sourceSystem)
{
    char buff[21];
   
    if(dataLength < 8 || dataLength > 27)
    {
        std::cout << "WARNING: corrupt LOGIN_REQUEST packet received from: \n"
                  << "SystemID: " << sourceSystem << "\n"
                  << "(IP:port): " << (net->getSystemAddress(sourceSystem)).ToString(true) << "\n"
                  << (dataLength < 8 ? "packet too small" : "packet too large") << std::endl;
        return 0;
    }

    strncpy_s(buff, 21, reinterpret_cast<char*>(data + 1), 6);
    std::string acc(buff);
   
    strncpy_s(buff, 21, reinterpret_cast<char*>(data + 1 + 6), dataLength - 7);
    std::string pass(buff);

// testing only:
    std::cout << acc << " " << acc.length() << "\n" << pass << " " << pass.length()
              << " " << dataLength << std::endl;
    return 0;
}

---

Well, now it's unportable, but at least it's safe.

Shouldn't you return something different on error than on success?

Hmm, I guess the strncpy_s makes it non-portable? Do you have a suggestion for a portable and safe alternative?

The return value indicates what to do with the packet that fired the event. I want it to be deallocated in both cases, so this is intended.

strncpy() is reasonable portable (C99) and safe enough for this purpose. For the account number, which is of a fixed size, I'd just use memcpy() and manual termination.