CODEWITHSUNDEEP
c++securitycode-access-security
Briz
Briz

Reputation: 536

What are the potential security vulnerabilities? C++

My boss told me to look at the following code and tell him what the potential security vulnerabilities were. I'm not very good at this kind of thing, since I don't think in the way of trying to hack code. All I see is that nothing is declared private, but other than that I just don't know.

#define NAME_SIZE (unsigned char) 255
// user input should contain the user’s name (first name space
// middle initial space last name and a null
// character), and was entered directly by the user.
// Returns the first character in the user input, or -1 if the method failed.
char poor_method(char* user_input, char* first, char *middle, char* last)
{
   char*buffer;
   char length;

   // find first name
   buffer = strtok(user_input, " ");
   if(buffer==0)
   {
        return -1;
   }
   length = strlen(buffer);
   if(length <= NAME_SIZE)
   {
        strcpy(first, buffer);
   }

   // find middle name
   buffer = strtok(NULL, " ");
   if(buffer==0)
   {
        return-1;
   }
   if(middle)
       *middle = buffer[0];

   // find last name
   buffer = strtok(NULL, "\0");
   length = strlen(buffer);
   if(length <= NAME_SIZE)
   {
       strcpy(last, buffer);
   }
   // Check to make sure that all of the user input was used
   buffer = strtok(NULL, "\0");
   if(buffer != NULL)
   {
       return-1;
   }
   return first[0];
}

What security vulnerabilities are there?

Upvotes: 2

Views: 3040

Answers (4)

Jason
Jason

Reputation: 32510

Rather than using strcpy(), use strncpy() with a specific length parameter, as that function, like strtok(), assumes a NULL-terminated buffer for the source, and that may not be the case, giving you a buffer overflow for the data copied into the buffer pointed to by either first or last. Additionally, you have no idea how long the buffers are that have been allocated for first and last ... Don't assume that the user of your function has properly allocated enough memory to copy into unless they've passed you a parameter telling you there are enough memory slots in the buffers. Otherwise again, you could (and most likely will) end-up with buffer overflows.

Also you may want to use the restrict keyword if you're using C99 in order to prevent the caller of your function from aliasing the same memory location for buffer, first, and last.

Upvotes: 2

helloworld922
helloworld922

Reputation: 10939

What I saw (by no means a complete list):

  1. There's no guarantees you're going to get a char pointer which points to a null-terminating string (unless you're allowed to make that assumption, not really a safe one to make).
  2. strtok and strcpy are the C way of doing things and come with the fun stuff of programming C code. If you must use them, so be it (just make sure you can guarantee you're inputs to these functions will indeed be valid). Otherwise, try switching your code to use std::string and the "C++ way" (as Cat Plus Plus put it)

  3. I'm assuming this is a typo: 

    charpoor_method(
    You're missing a space between char and poor_method(

  4. You're not checking if first or last are indeed valid pointers (unfortunately, the best you can do is to check them against NULL).

  5. There's no guarantee that the buffers first or last can indeed hold whatever you're copying to them.
  6. Another typo:
returnfirst[0];

missing space between return and first[0]

Learning to write secure code is something that's very important to do. Follow Brecht's advice and get good at it.

Upvotes: 5

daniel
daniel

Reputation: 9835

  1. Ok strtok assumes user_input is NULL terminated, this might not be true.

 charlength = strlen(buffer);
  if(length <= NAME_SIZE)
  {
        strcpy(first, buffer);
  }

  1. charlenght here is undeclared, so is length, they should be declared as unsigned int.

  2. strlen wont count the '\0' as a part of the length, so later strcpy will copy the '\0' to whatever is after First if the len of buffer is 255 + 1('\0')

  3. Also is unknown if char *first size is, it should be NAME_SIZE but the comparisson should be length <= NAME_SIZE - 1 or allocate char *first to NAME_SIZE + 1

I'd probably rewrite the whole thing, is quite ugly.

Upvotes: 2

Demian Brecht
Demian Brecht

Reputation: 21368

Get good at writing secure code

You most likely don't want systems that you are responsible for finding their way onto bugtraq or cve. If you don't understand it, be honest with your boss. Tell him you don't understand and you want to work on it. Pick up Writing Secure Code. Read it, learn it, love it. Asking this question on SO and giving your boss the answer definitely doesn't help you in the long run.

Then look at the sample code again :)

Upvotes: 6

Related Questions