thorsten wrote:
> The gifinto program segfaults when called without arguments.
>
> The reason is NULL beeing supplied to strcpy as address to copy from. I
> am not a C Programmer so the attached patch just solves the problem for
> the gifinto program, there might be others affected, too since the
> *real* Problem seems to be in the commandline parsing of lib/getarg.c,
> which is just plain chinese to me.
>
> Perhaps someone with C background is willing to have a look?
>
>
> Thorsten
There are some more unpleasant things concerning the sourcecode of
gifinto.c, strcpy() into 80 byte arrays without bounds checking from the
commandline, I tried to fix those. Also got rid of the tmpnam() call
with the attached patch. Looking at that file only, I suspect a whole
lot more trouble within the rest of the code. Which security imlications
this has for a system using giflib is beyond my knowledge.
--- giflib-4.1.6/util/gifinto.c 2005-10-10 08:22:22.000000000 +0200
+++ giflib-4.1.6-new/util/gifinto.c 2011-11-14 14:25:03.000000000 +0100
@@ -39,9 +39,11 @@
#define PROGRAM_NAME "GifInto"
+#define STRLEN 512
+
#define DEFAULT_MIN_FILE_SIZE 14 /* More than GIF stamp + screen desc. */
#define DEFAULT_OUT_NAME "GifInto.Gif"
-#define DEFAULT_TMP_NAME "TempInto.$$$"
+#define DEFAULT_TMP_NAME "TempInto.XXXXXX"
#ifdef __MSDOS__
extern unsigned int
@@ -80,7 +82,7 @@
int Error, NumFiles,
MinSizeFlag = FALSE, HelpFlag = FALSE;
char **FileName = NULL,
- TmpName[80], FoutTmpName[80], FullPath[80], DefaultName[80], s[80], *p;
+ TmpName[STRLEN], FoutTmpName[STRLEN], FullPath[STRLEN],
DefaultName[STRLEN], s[STRLEN], *p;
FILE *Fin, *Fout;
if ((Error = GAGetArgs(argc, argv, CtrlStr, &GifQuietPrint,
@@ -122,7 +124,9 @@
/* Isolate the directory where our destination is, and set tmp file name */
/* in the very same directory. */
- strcpy(FullPath, *FileName);
+ if ( *FileName == NULL ) GIF_EXIT("No valid Filename given.");
+ if ( strlen(*FileName) > STRLEN-1 ) GIF_EXIT("Filename too long.");
+ strncpy(FullPath, *FileName, STRLEN);
if ((p = strrchr(FullPath, '/')) != NULL ||
(p = strrchr(FullPath, '\\')) != NULL)
p[1] = 0;
@@ -131,16 +135,19 @@
else
FullPath[0] = 0; /* No directory or disk specified. */
- strcpy(FoutTmpName, FullPath); /* Generate destination temporary name. */
- /* Make sure the temporary is made in the current directory: */
- p = tmpnam(TmpName);
- if (strrchr(p, '/')) p = strrchr(p, '/') + 1;
- if (strrchr(p, '\\')) p = strrchr(p, '\\') + 1;
- if (strlen(p) == 0) p = DEFAULT_TMP_NAME;
- strcat(FoutTmpName, p);
-
- Fout = fopen(FoutTmpName, "wb");
- if (Fout == NULL)
+ if ( strlen(FullPath) > STRLEN-1 ) GIF_EXIT("Filename too long.");
+ strncpy(FoutTmpName, FullPath, STRLEN); /* First setup the Path */
+ /* then add a name for the tempfile */
+ if ( (strlen(FoutTmpName) + strlen(DEFAULT_TMP_NAME)) > STRLEN-1 )
GIF_EXIT("Filename too long.");
+ strcat(FoutTmpName, DEFAULT_TMP_NAME);
+ int FD;
+ FD = mkstemp(FoutTmpName); /* returns filedescriptor */
+ if (FD == -1 )
+ {
+ GIF_EXIT("Failed to open output.");
+ }
+ Fout = fdopen(FD, "w"); /* returns a stream with FD */
+ if (Fout == NULL )
{
GIF_EXIT("Failed to open output.");
}
@@ -151,7 +158,6 @@
GIF_EXIT("Failed to open output.");
}
#endif /* __MSDOS__ */
-
while (!feof(Fin)) {
if (putc(getc(Fin), Fout) == EOF)
GIF_EXIT("Failed to write output.");
@@ -162,11 +168,12 @@
fclose(Fout);
unlink(*FileName);
if (rename(FoutTmpName, *FileName) != 0) {
- strcpy(DefaultName, FullPath);
+ if ( (strlen(FullPath) + strlen(DEFAULT_OUT_NAME)) > STRLEN-1 )
GIF_EXIT("Filename too long.");
+ strncpy(DefaultName, FullPath, STRLEN);
strcat(DefaultName, DEFAULT_OUT_NAME);
if (rename(FoutTmpName, DefaultName) == 0) {
- sprintf(s, "Failed to rename out file - left as %s.",
- DefaultName);
+ snprintf(s, STRLEN, "Failed to rename out file - left as %s.",
DefaultName);
+
GIF_MESSAGE(s);
}
else {
--
http://linuxfromscratch.org/mailman/listinfo/blfs-dev
FAQ: http://www.linuxfromscratch.org/blfs/faq.html
Unsubscribe: See the above information page
