On Wed, 13 May 2015 10:57:00 -0400 Jamie Couture <[email protected]> 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 _______________________________________________ CGit mailing list [email protected] http://lists.zx2c4.com/mailman/listinfo/cgit
