2

I am converting some old c program to a more secure version. The following functions are used heavily, could anyone tell me their secure counterparts? Either windows functions or C runtime library functions. Thanks.

itoa()
getchar()
strcat()
memset()
smwikipedia
  • 61,609
  • 92
  • 309
  • 482
  • 3
    Whose definition of security are you using? Why are you making this change? If the old program works and you're not changing it drastically (e.g. add threading), it's very likely you *should not* be touching it. –  Nov 10 '10 at 07:23
  • 2
    You keep saying C, yet tag the question as C++? Why? Which is it? – GManNickG Nov 10 '10 at 07:30
  • 2
    you have tagged it as c++ so you could consider stringstream as well. But then that is a much more massive change ... – obelix Nov 10 '10 at 07:34
  • @obelix: Changing from C to C++ would be a vastly more massive changed than adding a use of `stringstream` to code that was already C++. – CB Bailey Nov 10 '10 at 07:37
  • @Charles: there's often real value in taking a C program, starting to compile it with a C++ program (small programs typically need no or little porting effort), then cleaning up a few messy spots with std::string, std::ostringstream, std::vector etc.. It may not be a template or OO redesign, but it can be a big step towards simpler, robust, and maintainable code. – Tony Delroy Nov 10 '10 at 07:42
  • 3
    @Tony: Unless the C program is poorly written, I cannot agree. I would say that porting good working C code to C++ is almost never worth the effort. Why would you take proven code and expend effort to get unproven code which is supposed to do exactly the same thing as the old code? – CB Bailey Nov 10 '10 at 07:48
  • 1
    @Charles: because even the few C++ facilities I mentioned earlier - strings, stringstreams and vectors - can sometimes reduce dozens of lines of fragile C code to two or three lines of robust C++, allowing the business logic to shine past the implementation details. Because automatic memory management (std::string, std::vector) is less fragile during software evolution than having to remember frees in all the right places. Because type-specific compile-time template instantiations outperform C-style generality as per qsort or bsearch. Because you can then begin to gradually adopt OO.... – Tony Delroy Nov 10 '10 at 08:04
  • 2
    @Tony: So you end up spending thousands of dollars (on coding and testing) a new C++ code that does nearly the same thing, only slower and with much higher memory usage, and with new failure cases due to possible allocation failures which could not have happened in the original C implementation. Lovely. – R.. GitHub STOP HELPING ICE Nov 10 '10 at 08:17
  • @R: Charles objection was "Unless the C program is poorly written". If the C code was well written, but the functionality needs to evolve quickly, it can be worth the move to C++ as the language is more powerful and expressive, and things like std::map<>, and non-standard but ubiquitous hash maps, allow a lot to be done with a little C++. It also costs money to maintain a lot of needlessly lower level code - cost-efficiency means picking your battles - so can do without your flippancy. All I said was there's "often" value, not always, and I stand by that assertion. – Tony Delroy Nov 10 '10 at 08:23
  • @R: BTW / vector and std::string are only signficantly more costly than C if the C usage was able to avoid the heap (and it's not like you have to use them), and templates are typically faster than the equivalent generic C. But I've seen enough of your posts to know that you understand the efficiency tradeoffs (and so could choose them when porting ;-P). – Tony Delroy Nov 10 '10 at 08:26
  • 4
    @Tony: You seem to be making the assumption that C code is necessarily (or at least likely to be) complex, fragile and unmaintainable and that the moving to C++ is a solution. C code can be robust and maintainable and moving to C++ is not (in and of itself) a solution if it is not. You don't have to look at much C++ code to realize that the language itself is not a panacea for fragility and maintenance problems. – CB Bailey Nov 10 '10 at 08:46
  • @Charles: as you say, some programs/libraries don't need anything more than C, many do, and it's generally pretty obvious where any given app falls so I won't further debate the ratio. Agreed C++ is not a panacea - my conviction is that a good C++ programmer can "surgically" improve on most C programs - by targetting the bits that are *most* complex/fragile/unmaintainable - often usefully and with little porting effort. An average C++ programmer might, but could equally lack the necessary discernment or make things worse. Such is life. – Tony Delroy Nov 10 '10 at 08:56

3 Answers3

8

itoa() is safe as long as the destination buffer is big enough to receive the largest possible representation (i.e. of INT_MIN with trailing NUL). So, you can simply check the buffer size. Still, it's not a very good function to use because if you change your data type to a larger integral type, you need to change to atol, atoll, atoq etc.. If you want a dynamic buffer that handles whatever type you throw at it with less maintenance issues, consider an std::ostringstream (from the <sstream> header).

getchar() has no "secure counterpart" - it's not insecure to begin with and has no buffer overrun potential.

Re memset(): it's dangerous in that it accepts the programmers judgement that memory should be overwritten without any confirmation of the content/address/length, but when used properly it leaves no issue, and sometimes it's the best tool for the job even in modern C++ programming. To check security issues with this, you need to inspect the code and ensure it's aimed at a suitable buffer or object to be 0ed, and that the length is computed properly (hint: use sizeof where possible).

strcat() can be dangerous if the strings being concatenated aren't known to fit into the destination buffer. For example: char buf[16]; strcpy(buf, "one,"); strcat(buf, "two"); is all totally safe (but fragile, as further operations or changing either string might require more than 16 chars and the compiler won't warn you), whereas strcat(buf, argv[0]) is not. The best replacement tends to be a std::ostringstream, although that can require significant reworking of the code. You may get away using strncat(), or even - if you have it - asprintf("%s%s", first, second), which will allocate the required amount of memory on the heap (do remember to free() it). You could also consider std::string and use operator+ to concatenate strings.

Tony Delroy
  • 102,968
  • 15
  • 177
  • 252
  • 1
    I agree with most of that but have to disagree with one small point, the assertion that `strcat(buf, argv[0])` is not safe. Again, it comes down to the competence of the programmer. That statement is _perfectly_ safe inside `if (strlen(buf) + strlen(argv[0]) < sizeof(buf)) {...}`. – paxdiablo Nov 10 '10 at 14:16
  • @paxdiablo: and that only if buf was already properly NUL terminated... for both of us, some context assumed - indeed the examples are preceeded and paragraph opened by "dangerous if...not know to fit" ;-P – Tony Delroy Nov 11 '10 at 01:05
4

None of these functions are "insecure" provided you understand the behaviour and limitations. itoa is not standard C and should be replaced with sprintf("%d",...) if that's a concern to you.

The others are all fine to the experienced practitioner. If you have specific cases which you think may be unsafe, you should post them.

paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
  • 7
    OP sounds like he's been smoking on MS's propaganda about what functions are "unsafe". The basic premise is that any standard function that hasn't been replaced by a MS-specific alternative is "unsafe" - after all they make it easy to port your code to *gasp!* other platforms. – R.. GitHub STOP HELPING ICE Nov 10 '10 at 08:14
  • +0, as the length of an expansion of "%d" is not documented. You can probably work it out, but that's not exactly portable. Consider `snprintf`, instead, or, yes, `_itoa_s`, neither of which suffer from this problem. (Surprisingly long %d expansions have actually caused me problems in the past.) –  Nov 10 '10 at 16:34
  • @R. - Microsoft's categorization of certain functions as 'unsafe' is not 'propaganda' - those functions have been quantitatively found by Microsoft to be incorrectly used enough times to be the root cause of many bugs that led to security issues. While I doubt that portability to other platforms was a major concern for them, it certainly wasn't a motivation either. They spent considerable effort in-house to move away from the functions they consider unsafe - they didn't go to all that effort just to make their own code non-portable. – Michael Burr Nov 10 '10 at 18:11
1

I'd change itoa(), because it's not standard, with sprintf or, better, snprintf if your goal is code security. I'd also change strcat() with strncat() but, since you specified C++ language too, a really better idea would be to use std::string class.

As for the other two functions, I can't see how you could make the code more secure without seeing your code.

Simone
  • 11,655
  • 1
  • 30
  • 43
  • 4
    `strncat` is not the "secure version of `strcat`". It's for a completely different purpose, concatenating data from fixed-size fields onto C strings. – R.. GitHub STOP HELPING ICE Nov 10 '10 at 08:12
  • I think you should read better strncat documentation. Anyway, since the OP included C++ tag, a better approach would be using std::string class. – Simone Nov 10 '10 at 08:19
  • From the Linux Programmer's Manual page "The strncat() function is similar [to strcat], except that only the first n characters of src are appended to dest.". So, it limits how much data is copied, less convenient for safety than a direct limit on how large data in dest attempts to grow, but a suitable calculation can be made for safe usage. @R: "concatenating data from fixed-sized fields" - if anything, copying from a fixed-sized field is the situation when you might be able to guarantee the dest buffer is large enough, where strcat() is ok rather than strncat(). – Tony Delroy Nov 10 '10 at 09:21
  • `strlcat()` would be slightly better than `strncat()`. The size parameter it takes is the total size of the destination buffer, not the maximum number of characters to copy from the source string. It still suffers from the flaw that, while buffer overruns are bad, silently truncating data is bad too. – JeremyP Nov 10 '10 at 11:31