Why is this code vulnerable to buffer overflow attacks?

On most compilers the maximum value of an unsigned short is 65535.

Any value above that gets wrapped around, so 65536 becomes 0, and 65600 becomes 65.

This means that long strings of the right length (e.g. 65600) will pass the check, and overflow the buffer.

Use size_t to store the result of strlen(), not unsigned short, and compare len to an expression that directly encodes the size of buffer. So for example:

char buffer[100];size_t len = strlen(str);if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;memcpy(buffer, str, len + 1);

The problem is here:

strncpy(buffer,str,strlen(str));                   ^^^^^^^^^^^

If the string is greater than the length of the target buffer, strncpy will still copy it over. You are basing the number of characters of the string as the number to copy instead of the size of the buffer. The correct way to do this is as follows:

strncpy(buffer,str, sizeof(buff) - 1);buffer[sizeof(buff) - 1] = '\0';

What this does is limit the amount of data copied to the actual size of the buffer minus one for the null terminating character. Then we set the last byte in the buffer to the null character as an added safeguard. The reason for this is because strncpy will copy upto n bytes, including the terminating null, if strlen(str) < len - 1. If not, then the null is not copied and you have a crash scenario because now your buffer has a unterminated string.

Hope this helps.

EDIT: Upon further examination and input from others, a possible coding for the function follows:

int func (char *str)  {    char buffer[100];    unsigned short size = sizeof(buffer);    unsigned short len = strlen(str);    if (len > size - 1) return(-1);    memcpy(buffer, str, len + 1);    buffer[size - 1] = '\0';    return(0);  }

Since we already know the length of the string, we can use memcpy to copy the string from the location that is referenced by str into the buffer. Note that per the manual page for strlen(3) (on a FreeBSD 9.3 system), the following is stated:

 The strlen() function returns the number of characters that precede the terminating NUL character.  The strnlen() function returns either the same result as strlen() or maxlen, whichever is smaller.

Which I interpret to be that the length of the string does not include the null. That is why I copy len + 1 bytes to include the null, and the test checks to make sure that the length < size of buffer - 2. Minus one because the buffer starts at position 0, and minus another one to make sure there's room for the null.

EDIT: Turns out, the size of something starts with 1 while access starts with 0, so the -2 before was incorrect because it would return an error for anything > 98 bytes but it should be > 99 bytes.

EDIT: Although the answer about a unsigned short is generally correct as the maximum length that can be represented is 65,535 characters, it doesn't really matter because if the string is longer than that, the value will wrap around. It's like taking 75,231 (which is 0x000125DF) and masking off the top 16 bits giving you 9695 (0x000025DF). The only problem that I see with this is the first 100 chars past 65,535 as the length check will allow the copy, but it will only copy up to the first 100 characters of the string in all cases and null terminate the string. So even with the wraparound issue, the buffer still will not be overflowed.

This may or may not in itself pose a security risk depending on the content of the string and what you are using it for. If it's just straight text that is human readable, then there is generally no problem. You just get a truncated string. However, if it's something like a URL or even a SQL command sequence, you could have a problem.

Even though you're using strncpy, the length of the cutoff is still dependent on the passed string pointer. You have no idea how long that string is (the location of the null terminator relative to the pointer, that is). So calling strlen alone opens you up to vulnerability. If you want to be more secure, use strnlen(str, 100).

Full code corrected would be:

int func(char *str) {   char buffer[100];   unsigned short len = strnlen(str, 100); // sizeof buffer   if (len >= 100) {     return -1;   }   strcpy(buffer, str); // this is safe since null terminator is less than 100th index   return 0;}