CODEWITHSUNDEEP
c++strcpyexploit
9Breaker
9Breaker

Reputation: 724

Safe Use of strcpy

Plain old strcpy is prohibited in its use in our company's coding standard because of its potential for buffer overflows. I was looking the source for some 3rd Party Library that we link against in our code. The library source code has a use of strcpy like this:

for (int i = 0; i < newArgc; i++)
{   
     newArgv[i] = new char[strlen(argv[i]) + 1];
     strcpy(newArgv[i], argv[i]);       
}

Since strlen is used while allocating memory for the buffer to be copied to, this looks fine. Is there any possible way someone could exploit this normal strcpy, or is this safe as I think it looks to be?

I have seen naïve uses of strcpy that lead to buffer overflow situations, but this does not seem to have that since it is always allocating the right amount of space for the buffer using strlen and then copying to that buffer using the argv[] as the source which should always be null terminated.

I am honestly curious if someone running this code with a debugger could exploit this or if there are any other tactics someone that was trying to hack our binary (with this library source that we link against in its compiled version) could use to exploit this use of strcpy. Thank you for your input and expertise.

Upvotes: 4

Views: 10468

Answers (4)

Jetski S-type
Jetski S-type

Reputation: 1208

As a general strcpy replacement idiom, assuming you're okay with the slight overhead of the print formatting functions, use snprintf:

snprintf(dest, dest_total_buffer_length, "%s", source);

e.g.

snprintf(newArgv[i], strlen(argv[i]) + 1, "%s", argv[i]);

It's safe, simple, and you don't need to think about the +1/-1 size adjustment.

Upvotes: 1

ckielstra
ckielstra

Reputation: 93

Deviating from a coding standard should always be possible, but then document well why you decided to do so.

The main problem with strcpy is that it has no length limitation. When taking care this is no problem, but it means that strcpy always is to be accompanied with some safeguarding code. Many less experienced coders have fallen into this pitfall, hence the coding guideline came into practice.

Possible ways to handle string copy safely are:

  • Check the string length
  • Use a safe variant like strlcpy, or on older Microsoft compilers, strncpy_s.

Upvotes: 0

Deduplicator
Deduplicator

Reputation: 45654

Well, there are multiple problems with that code:

  • If an allocation throws, you get a memory-leak.
  • Using strcpy() instead of reusing the length is sub-optimal. Use std::copy_n() or memcpy() instead.

Presumably, there are no data-races, not that we can tell.

Anyway, that slight drop in performance is the only thing "wrong" with using strcpy() there. At least if you insist on manually managing your strings yourself.

Upvotes: 3

Martin Bonner supports Monica
Martin Bonner supports Monica

Reputation: 28987

It is possible to use strcpy safely - it's just quite hard work (which is why your coding standards forbid it).

However, the code you have posted is not a vulnerability. There is no way to overwrite bits of memory with it; I would not bother rewriting it. (If you do decide to rewrite it, use std::string instead.

Upvotes: 6

Related Questions