Hi,
> case 3: if (GT_SCHEME(LIBGGI_GT(vis)) == GT_TEXT) {
> - sprintf(apiname, "generic-text-%d", size);
> + snprintf(apiname, GGI_MAX_APILEN,
> + "generic-text-%d", size);
> return 0;
Make sure to configure-test for snprintf and include an implementation when
it is not there. snprintf is non-ansi and only conforms to ISO/IEC 9899:1999
(`ISO C99''). This might be a portability problem, as snprintf cannot
easily be "emulated".
Using sprintf in the above case is not a security problem, as the maximum
length is known, as size is in a limited range.
> - strcpy(apiname, "generic-planar");
> + strncpy(apiname, GGI_MAX_APILEN,
> + "generic-planar");
> return 0;
NEVER use strncpy without an explicit termination. A program failing to
load a library because its name is too long is o.k.
A program that either loads something wrong or segfaults, because a string
operation goes beyond the end because of a missing terminator is a security
risk.
In that specific case, a runtime check is useless, as a string of _constant_
length is put in the apiname. If you really really want to check that,
please check it at compile-/configure-time.
> if (priv->fix.type == FB_TYPE_INTERLEAVED_PLANES) {
> - sprintf(apiname, "generic-%s",
> + snprintf(apiname, GGI_MAX_APILEN,
> + "generic-%s",
> (priv->fix.type_aux == 2) ?
> "iplanar-2p" : "ilbm");
similar here. Size is known. Why fix what aint broken?
> - sprintf(apiname, "generic-linear-%d", size);
> + snprintf(apiname, GGI_MAX_APILEN,
> + "generic-linear-%d", size);
Same. As I can't derive it from the manpage, can anyone here say, what
snprintf is supposed to do on overflows ?
Will it write a trailing \0 ? If not, that should be generated, to avoid the
Same comment for the other changes.
> - sprintf(apiname, "display-libkgi");
> + snprintf(apiname, GGI_MAX_APILEN, "display-libkgi");
IMHO rather strcpy instead of sprintf.
> - sprintf(apiname, "display-libkgi-%s",
> + snprintf(apiname, GGI_MAX_APILEN, "display-libkgi-%s",
> LIBKGI_PRIV(vis)->suggest);
O.K. - that might be a bad one. Depending on whether snprintf does the \0
thing, I'd say that is a good fix.
> - strcpy(apiname, "generic-stubs");
> + strcpy(apiname, "generic-stubs");
Huh ?
> dest_un.sun_family = AF_UNIX;
> - strcpy(dest_un.sun_path, addr);
> + strncpy(dest_un.sun_path, addr, strlen(addr)+1);
WHAT ? No - that one would limit by the _source_ size. Unless it is
explicitly made sure that dest_un.sun_path is large enough to handle
strlen(addr)+1 (in which case the strcpy would do), this is the wrong thing
to do. It might quieten that strange scanner software, but it doesn't do
anything good. It's a NOP.
CU, Andy
--
= Andreas Beck | Email : <[EMAIL PROTECTED]> =
