On Sun, 27 Jan 2002, Andreas Beck wrote:
> 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.
done.
> > 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
snip from the manual page:
Return value
These functions return the number of characters printed
(not including the trailing `\0' used to end output to
strings). snprintf and vsnprintf do not write more than
size bytes (including the trailing '\0'), and return -1 if
the output was truncated due to this limit. (Thus until
glibc 2.0.6. Since glibc 2.1 these functions follow the
C99 standard and return the number of characters (exclud
ing the trailing '\0') which would have been written to
the final string if enough space had been available.)
> Same comment for the other changes.
>
> > - sprintf(apiname, "display-libkgi");
> > + snprintf(apiname, GGI_MAX_APILEN, "display-libkgi");
>
> IMHO rather strcpy instead of sprintf.
done.
> > - 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 ?
oh, that's just a replacement of 16 spaces by 2 tabs.
> > 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.
fixed.
CU,
Christoph Egger
E-Mail: [EMAIL PROTECTED]
