From: Timo Sirainen Date: Thu, 30 Aug 2007 22:41:03 GMT Subject: Re: Buffer overflows Message-ID: <16CA3709-5E04-4CEA-B154-2339C5E4D23C@iki.fi> In-Reply-To: <7vtzqg7jrn.fsf@gitster.siamese.dyndns.org> On 31.8.2007, at 1.14, Junio C Hamano wrote: > "Reece Dunn" writes: > >> Why is it easier? If you have a fixed-size buffer, why not use >> strncpy, which is what a safe string API is essentially doing anyway? > > I would not claim unchecked strcpy is good -- we obviously would > want to fix them. > > But at the same time use of strncpy, strlcpy and friends solves > only half of the problem. Often people say "use strncpy or > strlcpy then you would not overstep the buffer", but that does > not really solve anything, without additional logic to deal with > resulting truncation (barfing with "insanely long string" error > message and dying is the least impact). Continuing the work on > data that the user did not intend to give you is just as wrong > as using corrupt data that overflowed your static buffer. Sure, I agree with that. In my own code I avoid static buffer sizes as much as I can. But since git's code is far from even trying to do that, I thought it would be easier to start with a simpler plan: Try not to have remote code execution holes that can be found with 2 minutes of looking at the code. > Does Timo's nonstandard API solve that issue? Perhaps it does, > perhaps not. That would require moving to dynamically growing strings, which in turn requires freeing the strings afterwards so it's not such a simple job. Simply replacing the current char[] buffers with my static_string would be quick and easy and although it wouldn't fix all potential problems, it would fix most of the buffer overflows. > Does it make easier to maintain our code? I highly doubt it in the > current shape. Depends on what you mean by "maintain". I find the resulting code a lot easier to understand, and a lot easier to verify for correctness and safety. Here's an example: http://marc.info/? l=git&m=117962988914013&w=2 But then again you and Alex didn't seem to think so. > It is well and widely understood idiom to use strlcpy to a > fixed-sized buffer and checking the resulting length to make > sure the result would not have overflowed (and if it would have, > issue an error and die). I would not have anything against a > set of patches to follow such a pattern. I don't like strlcpy()/strlcat() all that much either, because checking the overflow is more difficult than it needs to be. For example: if (strlcpy(dest, src, sizeof(dest)) <= sizeof(dest)) overflow(); // compared to a function that simply returns if it overflowed or not: if (strocpy(dest, src, sizeof(dest)) < 0) overflow(); Actually I'm not even sure if the above strlcpy() check is right. Is it <= or The API needs to justify itself to convince the people who needs > to learn and adjust to that the benefit far outweighes deviation > from better known patterns, and I do not see that happening in > Timo's patch. The better known patterns are being used insecurely all the time now, so I can't really see how this would be anything worse. Anyway my point wasn't to get my code into git. I just wanted that *something* would be done about this. Currently I just can't see myself wanting to use git, because it limits what I can do with it. Any kind of automated processing is completely out of the question because then attackers could easily take over my machine if they wanted to.