4

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.

9Breaker
  • 724
  • 6
  • 16
  • 11
    If somebody has privileges to run your code with a debugger... it really does not matter what library function you call or don't call. That person got root on the machine and can do whatever they like. – nvoigt May 07 '18 at 16:06
  • Side comment: the above looks like a naive way to copy program arguments because it makes a separate memory allocation for each of the argument. What it should rather do is allocate one memory block for all arguments. – Maxim Egorushkin May 07 '18 at 16:06
  • 1
    `memcpy()` would be more efficient in this case, but anyway you just better use `std::string` instead – Slava May 07 '18 at 16:06
  • @nvoigt Thanks for the comment, yes that makes sense. Besides the debugger, do you think there are other ways to exploit this use of strcpy? – 9Breaker May 07 '18 at 16:11
  • @HansPassant That is a good point. I was just wondering if the way the library writers originally wrote the source could be exploited in my final binary since I link against their code. – 9Breaker May 07 '18 at 16:15
  • Have you tried `strncpy`? – Thomas Matthews May 07 '18 at 16:20
  • @ThomasMatthews Well this is source from a library that we link against. Since I have the source, I know I could change it, but I am just wondering if the original way the library source was written is safe or if it can be exploited with normal strcpy. – 9Breaker May 07 '18 at 16:23
  • 2
    @ThomasMatthews Plese don't recommend strncpy, it is in many ways worse than strcpy. It does not always nul terminate the string, and when it does , it also also wastes time filling the remaining part of the buffer, with nul bytes. – nos May 07 '18 at 16:25
  • 2
    @ThomasMatthews It doesn't matter what the question is, `strncpy` is usually not the answer. It doesn't guarantee to put in the trailing `'\0'`, and it does guarantee to assign to an unnecessarily large amount of the buffer when copying a small string. Either `strcpy_s` or `strlcpy` are much better choices (and it really annoys me that neither are mandatory). (@nos: snap!) – Martin Bonner supports Monica May 07 '18 at 16:25

4 Answers4

6

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.

Martin Bonner supports Monica
  • 28,528
  • 3
  • 51
  • 88
3

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.

Deduplicator
  • 44,692
  • 7
  • 66
  • 118
  • Comparing the number of characters that have been copied to `n` is not inherently slower than comparing the element being copied to `\0`. I don't see the claimed performance hit here. – Pete Becker May 07 '18 at 17:55
  • @PeteBecker: You are using a wrong and far too simple model of computation. If you know how many bytes to copy, you can use specialized instructions and / or move bigger blocks at once. – Deduplicator May 07 '18 at 18:19
  • @Deduplicator : Given that Mr Becker was once half a company that wrote a widely used C++ runtime library, I am pretty sure he knows that. a) You can use some of those specialized instructions in strcpy; b) the tests on whether everything is aligned etc can eat a lot of performance benefit; c) as always with performance, the only way to tell is to measure with realistic data (lengths, alignment). – Martin Bonner supports Monica May 07 '18 at 19:04
  • @MartinBonner: No, those use a length. And whether it matters in a specific case of course cannot be definitely decided without extensive realistic testing, what else is new... – Deduplicator May 07 '18 at 20:45
0

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.
ckielstra
  • 93
  • 1
  • 4
  • strncpy_s is included in C11 standard as an option: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1969.htm and https://embeddedgurus.com/barr-code/2017/08/cs-strcpy_s-c11s-more-secure-version-of-strcpy/ – Jetski S-type Nov 14 '18 at 04:42
  • Thanks for the additional info on strncpy_s being optional in C11. Your link also mentions that in 2017 "no mainstream C standard lib has implemented it yet". When it was not implemented 6 years after the standard came out I guess it never will be. – ckielstra Nov 15 '18 at 21:56
  • Yes, I agree. It's a shame the standard does not reflect real usage. – Jetski S-type Nov 15 '18 at 22:24
0

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.

Jetski S-type
  • 1,138
  • 2
  • 16
  • 32