Hi!

While testing gimp with cppcheck I saw a lot of warnings caused by the usage of scanf and fscanf without specifying a width limit.

One example:

========= [./app/gegl/gimpcurvesconfig.c:392]: (warning) scanf without field width limits can crash with huge input data. To fix this error message add a field width specifier:
%s => %20s
%i => %3i

Sample program that can crash:

#include int main()
{
int a;
scanf("%i", &a);
return 0;
}

To make it crash:
perl -e 'print "5"x2100000' | ./a.out =========

Indeed it's possible to make gimp crash by using a curve file with such big value; you can test by trying to import http://people.debian.org/~naoliv/misc/gimp/curve.cur

A full list of fscanf/scanf warnings is available at http://people.debian.org/~naoliv/misc/gimp/scanf.txt

Thank you!

Best regards, Nelson

Hi!

On Mon, Jan 24, 2011 at 1:11 AM, Christopher Curtis wrote:

On Sat, Jan 22, 2011 at 2:04 AM, Nelson A. de Oliveira wrote:

To make it crash:
perl -e 'print "5"x2100000' | ./a.out

I believe the unbounded '%s' is a legitimate bug, but is the '%i' assertion true?

The example it gives doesn't crash when I run it.  Instead scanf returns ERANGE and (oddly) sets 'a' to -1.  This may be Linux specific behavior though.

Both on Linux and FreeBSD I get a segmentation fault with the example code. I can't test on other platforms however.

Best regards, Nelson

Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Hi!

On Mon, Jan 24, 2011 at 8:26 AM, Simon Budig wrote:

For Gimp itself there is a bug report on this issue at https://bugzilla.gnome.org/show_bug.cgi?id=639203

I guess I'll commit the patch attached to the bugreport soon unless someone has a better suggestion.

But here, for example (from your patch):

snprintf (fmt_str, sizeof (fmt_str), "%%d %%d %%%lds", sizeof (endbuf) - 1); if (sscanf (line, fmt_str, &t->majtype, &t->type, end) != 3)

Won't it still be affected by a very large integer (like the example that I sent on my initial message) at the first or second position in the file?

I get this when trying to load an example with the the big number: =====
Plug-in crashed: "sphere-designer"
(/usr/lib/gimp/2.0/plug-ins/sphere-designer)

The dying plug-in may have messed up GIMP's internal state. You may want to save your images and restart GIMP to be on the safe side. =====

But I can't say how bad or ignorable it is.

Best regards, Nelson

Hi!

On Mon, Jan 24, 2011 at 9:41 AM, Simon Budig wrote:

Ah sorry, should have mentioned that. The bug report is older than your mail to the list. We had a report on the %s conversion earlier which is what the patch attached to the bug attempts to fix.

Here (also from your patch):

snprintf (fmt_str, sizeof (fmt_str), "%%%lds %%%lds %%%lds %%%lds", sizeof (colorstr_r) - 1, sizeof (colorstr_g) - 1, sizeof (colorstr_b) - 1, sizeof (colorstr_a) - 1);

sscanf (ptr, fmt_str, colorstr_r, colorstr_g, colorstr_b, colorstr_a);

It will protects against the overflow, but there is a chance to get wrong data (if the first string is also very big). For example, with this ugly example code that I think that is similar to the one from your patch:

#include
int main()
{
char str1[16];
char str2[16];
char str3[16];
char str4[16];
char fmt[128];
char buf[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbb ccccccccccccccc ddddddddddddddd";

snprintf(fmt, sizeof(fmt), "%%%lds %%%lds %%%lds %%%lds", sizeof(str1) - 1, sizeof(str2) - 1, sizeof(str3) - 1, sizeof(str4) - 1);
sscanf(buf, fmt, str1, str2, str3, str4); printf("*%s* *%s* *%s* *%s*", str1, str2, str3, str4); return 0;
}

See that we have one big string first and all the four vars (wrongly) were used by it:
*aaaaaaaaaaaaaaa* *aaaaaaaaaaaaaaa* *aaaaaaaaaaaaaaa* *aaaaaaaaaaaaaaa*

Right?

Best regards, Nelson