conntrack-tools: fscanf() call usage

conntrack-tools: fscanf() call usage

[Thomas Jarosch]

Hi there,

the conntrack-tools have an fscanf() call without a
field width limit. There's a small issue with this:

[conntrack-tools/src/conntrack.c:1875]: (warning) scanf
without field width limits can crash with huge input data

Simple PoC can be found here:
http://marc.info/?l=gimp-developer&m=129567990905823&w=2

The code currently looks like this:
    "if (fscanf(fd, "%d", &count) != 1) {"


A limit to uint32 number of digits should be sane, also on 64bit machines.
That should still give us enough conntrack entries - even for Jesper.

I goofed around with scanf() some more and discovered that basically any 
field limit should do (even 2000+). Otherwise scanf() seems to use a fixed 
default buffer if the size is not known in advance.


Proposed change looks like this:
    "if (fscanf(fd, "%10d", &count) != 1) {"

Is that a sane thing to do or unnecessary?

Cheers,
Thomas

Re: conntrack-tools: fscanf() call usage

[Philip Craig]

From: Thomas Jarosch <thomas.jarosch@intra2net.com>
Sent: Thu 25 Aug 2011 22:22:58 EST

> Hi there,
>
> the conntrack-tools have an fscanf() call without a
> field width limit. There's a small issue with this:
>
> [conntrack-tools/src/conntrack.c:1875]: (warning) scanf
> without field width limits can crash with huge input data
>
> Simple PoC can be found here:
> http://marc.info/?l=gimp-developer&m=129567990905823&w=2

This looks like a bug in scanf, I suggest fixing it there. I don't know
why the cppcheck authors decided that all the users needed fixing.

> The code currently looks like this:
>     "if (fscanf(fd, "%d", &count) != 1) {"
>
>
> A limit to uint32 number of digits should be sane, also on 64bit machines.
> That should still give us enough conntrack entries - even for Jesper.
>
> I goofed around with scanf() some more and discovered that basically any
> field limit should do (even 2000+). Otherwise scanf() seems to use a fixed
> default buffer if the size is not known in advance.

The size limit is 0x200000. It's not clear to me yet what exactly is
causing this.

> Proposed change looks like this:
>     "if (fscanf(fd, "%10d", &count) != 1) {"
>
> Is that a sane thing to do or unnecessary?

Working around the problem in every program that uses scanf is not the
right thing to do.

Re: conntrack-tools: fscanf() call usage

[Thomas Jarosch]

On Friday, 26. August 2011 03:45:29 Philip Craig wrote:
> > Simple PoC can be found here:
> > http://marc.info/?l=gimp-developer&m=129567990905823&w=2
> 
> This looks like a bug in scanf, I suggest fixing it there. I don't know
> why the cppcheck authors decided that all the users needed fixing.
>
> ...
>
> Working around the problem in every program that uses scanf is not the
> right thing to do.

Right. I'll contact the glibc maintainers this week, it looks like an issue
with scanf(). One guy from cppcheck tried the same scanf() code
on another platform and there was no crash detectable.

Best regards,
Thomas

Re: conntrack-tools: fscanf() call usage

[Thomas Jarosch]

On Friday, 26. August 2011 03:45:29 Philip Craig wrote:
> > Proposed change looks like this:
> >     "if (fscanf(fd, "%10d", &count) != 1) {"
> > 
> > Is that a sane thing to do or unnecessary?
> 
> Working around the problem in every program that uses scanf is not the
> right thing to do.

This issue has been resolved upstream in glibc.
(http://sourceware.org/bugzilla/show_bug.cgi?id=13138)

Cheers,
Thomas