[PATCH] cgit: use strtol_i instead of atoi

[Natanael Copa]

On Wed, 13 May 2015 10:57:00 -0400
Jamie Couture <jamie.couture at gmail.com> wrote:

> On Wed, May 13, 2015 at 02:45:56PM +0100, John Keeping wrote:
> > On Wed, May 13, 2015 at 02:41:24PM +0100, John Keeping wrote:
> > > On Wed, May 13, 2015 at 03:35:29PM +0200, Jason A. Donenfeld wrote:
> > > > Anybody have any objections to this? In some cases it's slightly more
> > > > verbose, but otherwise, I can't see any downsides.
> > > 
> > > It's worse if there is trailing data.  Since there's nothing obvious we
> > > can do if the input is bad, I'm not sure how much we care (i.e. ignoring
> > > the return value from strtol_i is OK) but whereas atoi will parse a
> > > valid value followed by trailing garbage strtol_i will just fail.
> > > 
> > > Worse than that, if it fails it leaves the result uninitialized, which
> > > doesn't matter in the cases where we just update a variable, but at
> > > least one part of this patch introduces a new variable that is not set
> > > if strtol_i fails.
> > 
> > Oops... I didn't double-check the patch before sending, it does always
> > initialize the variables first so the only worry is trailing garbage.
> > 
> > Of course, if atoi leads to SQL injection, what makes strtol safe?  The
> > test seems fundamentally useless; AFAICT the whole point is that if I
> > parse some user input and use without validating it then I can end up
> > doing something like:
> > 
> > 	int num_items = <user input>;
> > 	items = malloc(num_items * sizeof(*items));
> > 
> > leading to integer overflow.  But the mechanism used to convert the user
> > input from a string to an integer is completely irrelevant.
> 
> Exactly.
> 
> I'm not familiar with the utility, but can nessus have exceptions to
> a rule?

I am not familiar with the utility either and I have no control over
it. I just get a report once in a while with graphs and colors and a
comment "There are some RED sql injections here and your server has the
worst security score".

I did send them an analyze once with a suggestion how to fix nessus
(prefixing the string with a space would have fixed the false positive
while will trigger on real sql injection vulnerabilities), however the
people who generates those reports probably did not understand any of
what i said, nor did they care. They only want their colored graphs to
be green.

(so I have currently added an iptables rule to drop tcp 80 from the
scanning ip addr)

> In cgit's case, I don't see the use of atoi() as a problem in this
> context.

You are absolutely right. atoi in cgit is no problem. But IMHO,
ignoring "1 and 1=0" instead of reading it as 1 is more technically
correct and thus improves quality of cgit slightly.

Should I send a new patch with better commit message?

-nc