What are the potential security vulnerabilities? C++

Question

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?

Just throw it away and write it in a proper C++ way. strtok is not thread-safe, for starters. — Cat Plus Plus, May 04 '11 at 21:36
I hope your boss is not on SO. ;) By "security" it seems you think it relates to hacking. Can he not simply mean memory issues, wrongly formatted input, etc. Perhaps that might be less out of your comfort zone? — Bart, May 04 '11 at 21:44
@Bart Maybe he does. I just assume hacking when I hear security. I'll look at it in this way! — Briz, May 04 '11 at 21:49
Look up [Buffer Overflow](http://en.wikipedia.org/wiki/Buffer_overflow). Also you can search on [ITsec.SE](http://security.stackexchange.com) — AviD, May 04 '11 at 22:00

score 6 · Answer 1 · answered May 04 '11 at 21:46

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 :)

score 5 · Accepted Answer · answered May 04 '11 at 21:57

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

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).
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)
I'm assuming this is a typo:
```
charpoor_method(
```
You're missing a space between char and poor_method(
You're not checking if first or last are indeed valid pointers (unfortunately, the best you can do is to check them against NULL).
There's no guarantee that the buffers first or last can indeed hold whatever you're copying to them.
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.

score 2 · Answer 3 · answered May 04 '11 at 21:46

2

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


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

charlenght here is undeclared, so is length, they should be declared as unsigned int.
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')
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.

answered May 04 '11 at 21:46

daniel

9,732
7
42
57

Shouldn't `charlength` be a `size_t`? – Mike Samuel May 04 '11 at 21:49
Yeah, size_t is typedef as unsigned int – daniel May 04 '11 at 21:50
From http://stackoverflow.com/questions/131803/unsigned-int-vs-size-t "[size_t] may be bigger, equal or even smaller than an unsigned int and your compiler might make assumption about it for optimization purpose." – Mike Samuel May 04 '11 at 21:52
It's what he showed me. I'm assuming it's written horribly just for the purpose of teaching new hires. – Briz May 04 '11 at 21:53

Jason · Answer 4 · 2011-05-04T22:08:58.237

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.

What are the potential security vulnerabilities? C++

4 Answers4