Re: Buffer overflows

"Reece Dunn" [off-list ref] 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.

Does Timo's nonstandard API solve that issue?  Perhaps it does,
perhaps not.  Does it make easier to maintain our code?  I
highly doubt it in the current shape.

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.

But a patch to add a non-standard API that nobody else uses,
without any patch to show the changes to a few places that could
use the API to demonstrate that the use of API vastly cleans the
code up and makes it infinitely harder to make mistakes?

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.

On Thu, Aug 30, 2007 at 10:14:04PM +0000, 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.

  Actually, strncpy solves nothing as it's completely broken in so many
ways: it does not necessarily ends the string with a NUL-char, and it
NUL-pads the buffer, making it really slow when you use it top copy 10
chars in a BUFSIZ-big buffer.

  strncpy should never ever be used, few programmers understand it, and
it's very error prone.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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.

On Friday 31 August 2007, Junio C Hamano wrote:

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.

But a patch to add a non-standard API that nobody else uses,
without any patch to show the changes to a few places that could
use the API to demonstrate that the use of API vastly cleans the
code up and makes it infinitely harder to make mistakes?

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.

So in general, git people seem to be saying that:

1. Yes, we agree that the C string library suX0rs badly.

2. There are more than 0 string manipulation bugs (e.g. buffer overflows) in 
git. The number may be small or large, but I have yet to see anyone claim 
it's _zero_.

3. Timo's patches (in their current form) are not the way to go, because of 
non-standard API, implementation problems, whatever...

So why does the discussion end there? Lukas proposed an interesting 
alternative in "The Better String Library" ( 
http://bstring.sourceforge.net/ ). Why has there been lots of bashing on 
Timo's efforts, but no critique of bstring? I'd be very keen to know what 
the git developers think of it. AFAICS, it seems to fulfill at least _some_ 
of the problems people find in Timo's patches. Specifically, it claims:

- High performance (better than the C string library)
- Simple usage

I'd also say it's probably more widely used than Timo's patches.


If the only response to Timo's highlighting of string manipulation problems 
in git, is for us to flame his patches and leave it at that, then I have no 
choice but to agree with him in that security does not seem to matter to 
us.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On 02/09/07, Johan Herland [off-list ref] wrote:

On Friday 31 August 2007, Junio C Hamano wrote:

quoted

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.

So in general, git people seem to be saying that:

1. Yes, we agree that the C string library suX0rs badly.

2. There are more than 0 string manipulation bugs (e.g. buffer overflows) in
git. The number may be small or large, but I have yet to see anyone claim
it's _zero_.

3. Timo's patches (in their current form) are not the way to go, because of
non-standard API, implementation problems, whatever...

So why does the discussion end there? Lukas proposed an interesting
alternative in "The Better String Library" (
http://bstring.sourceforge.net/ ). Why has there been lots of bashing on
Timo's efforts, but no critique of bstring? I'd be very keen to know what
the git developers think of it. AFAICS, it seems to fulfill at least _some_
of the problems people find in Timo's patches. Specifically, it claims:

- High performance (better than the C string library)
- Simple usage

Performing a brief look at the documentation, the bstring library
looks promising.

It looks like it has an allocate and grow internal buffers on demand
policy. This is similar to what the C++ std::basic_string does, as
well as the string helpers in the Boost version of Jam (written in C).
This hides the buffer management from the user of the library, rather
than obfuscating it like in Timo's patch.

The API defined in the documentation is well thought out and
extensive, moreso than in the efforts by Timo and others. It has the
traditional C API, along with other API found in other string
libraries (such as split and join). I am not sure how much of git
could make use of these, but they have the pontential to simplify some
areas of the codebase.

Looking at the documentation, it is clear that this is a well thought
out library, both from the problems/security issues of the C library
and to how it compares with other string libraries. As well as
covering buffer overflow, it also deals with things like integer
overflow.

They have also done performance tests comparing the bstring library to
the C API and C++ std::string. With the C API comparison, the library
performs about 10% slower for string assignment, but other areas don't
have a slowdown. In fact, string concatenation is _considerably_
improved, something that will help git performance. I suspect (but
have not verified) that the slowdown on assignment is due to buffer
allocation.

I'd also say it's probably more widely used than Timo's patches.

Which is good, as this means that along with the tests in the library,
it will be more stable and less likely to be buggy than something that
is written from scratch.

If the only response to Timo's highlighting of string manipulation problems
in git, is for us to flame his patches and leave it at that, then I have no
choice but to agree with him in that security does not seem to matter to
us.

I would not like to see that happen. It seems that the bstring library
will help git in more ways than security, by improving string
concatenation performance and giving a richer string API without
sacrificing performance (except where noted) and code clarity.

It would be interesting to see how the 10% performance drop on string
assignment impacts git performance, when balanced with the drastic
(92x in the performance table) increase on string concatenation.

The only major issue that I can see with bstring is that it does not
have a wchar_t version, but git is using chars internally, so this is
not a problem for git.

- Reece

Johan Herland schrieb:

So why does the discussion end there? Lukas proposed an interesting 
alternative in "The Better String Library" ( 
http://bstring.sourceforge.net/ ). Why has there been lots of bashing on 
Timo's efforts, but no critique of bstring? I'd be very keen to know what 
the git developers think of it. AFAICS, it seems to fulfill at least _some_ 
of the problems people find in Timo's patches. Specifically, it claims:

- High performance (better than the C string library)
- Simple usage

I'd also say it's probably more widely used than Timo's patches.


If the only response to Timo's highlighting of string manipulation problems 
in git, is for us to flame his patches and leave it at that, then I have no 
choice but to agree with him in that security does not seem to matter to 
us.

Well, a patch (8dabdfcc) from Alex Riesen has made it into 1.5.3 which
fixes some of the problems.  That's a start.

And don't forget that we have our very own string library, viz.
strbuf.c, which could see more use.

That said, I agree that bstring looks well thought out.  It's also quite
large (lots of functions, lots of code where a bug might lurk).  Hmm.

Now if only someone could demonstrate the advantages of using bstring in
git by posting a nice patch.. :-P

René

René Scharfe wrote:

And don't forget that we have our very own string library, viz.
strbuf.c, which could see more use.

That said, I agree that bstring looks well thought out.  It's also quite
large (lots of functions, lots of code where a bug might lurk).  Hmm.

Now if only someone could demonstrate the advantages of using bstring in
git by posting a nice patch.. :-P

René

I'm currently working on rewriting builtin-mailinfo.c to use bstring. 
I'll hopefully have a proof-of-concept ready today or tomorrow.

/Lukas