Re: Buffer overflows

From: Timo Sirainen <tss@iki.fi>
Date: Aug 30, 2007, 22:41 UTC
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:

Show 16 quoted lines

> "Reece Dunn" <msclrhd@googlemail.com> 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.

Show 5 quoted lines

> 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.