Safe string copying

This is a discussion on Safe string copying within the C++ Programming forums, part of the General Programming Boards category; I'm doing some network programming, and I'd like you to review a small piece of code. I want to know ...

  1. #1
    Registered User
    Join Date
    Dec 2006
    Posts
    69

    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

  2. #2
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,892
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  3. #3
    Registered User
    Join Date
    Dec 2006
    Posts
    69
    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;
    }
    Last edited by C+/-; 03-31-2008 at 08:03 AM.

  4. #4
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,892
    Well, now it's unportable, but at least it's safe.

    Shouldn't you return something different on error than on success?
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  5. #5
    Registered User
    Join Date
    Dec 2006
    Posts
    69
    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.

  6. #6
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,892
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 8
    Last Post: 04-25-2008, 02:45 PM
  2. Copying a string, into a string array.
    By m.mixon in forum C Programming
    Replies: 5
    Last Post: 07-31-2006, 05:19 PM
  3. Linked List Help
    By CJ7Mudrover in forum C Programming
    Replies: 9
    Last Post: 03-10-2004, 09:33 PM
  4. Classes inheretance problem...
    By NANO in forum C++ Programming
    Replies: 12
    Last Post: 12-09-2002, 02:23 PM
  5. creating class, and linking files
    By JCK in forum C++ Programming
    Replies: 12
    Last Post: 12-08-2002, 01:45 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21