Re: Buffer overflows

On 31.8.2007, at 1.14, Junio C Hamano wrote:

"Reece Dunn" [off-list ref] writes:

quoted

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.