Moritz Muehlenhoff wrote:
> Since fbi is not suitable for non-interactive use and the filename would
> need to contain the commands to be executed I don't consider this a
> security problem. Still, it should be fixed.
>
It is at least quite hard to exploit remotely. Even when configuring
fbi as image viewer in your text mode browser you'll usually end up with
an mkstemp()-generated, /tmp/foo-xrks73w style filename being passed to
fbi ...
> CCing upstream. Gerd, the popen() call needs to be sanitised or replaced
>
Fixed in cvs, patch attached for reference.
cheers,
Gerd
Index: fbi.c
===================================================================
RCS file: /home/cvsroot/fbida/fbi.c,v
retrieving revision 1.22
retrieving revision 1.24
diff -u -p -r1.22 -r1.24
--- fbi.c 25 Aug 2006 13:55:52 -0000 1.22
+++ fbi.c 3 Dec 2007 10:23:18 -0000 1.24
@@ -637,7 +637,6 @@ static void free_image(struct ida_image
static struct ida_image*
read_image(char *filename)
{
- char command[1024];
struct ida_loader *loader = NULL;
struct ida_image *img;
struct list_head *item;
@@ -666,11 +665,29 @@ read_image(char *filename)
}
if (NULL == loader) {
/* no loader found, try to use ImageMagick's convert */
- snprintf(command,sizeof(command),
- "convert -depth 8 \"%s\" ppm:-",filename);
- if (NULL == (fp = popen(command,"r")))
+ int p[2];
+
+ if (0 != pipe(p))
+ return NULL;
+ switch (fork()) {
+ case -1: /* error */
+ perror("fork");
+ close(p[0]);
+ close(p[1]);
return NULL;
- loader = &ppm_loader;
+ case 0: /* child */
+ dup2(p[1], 1 /* stdout */);
+ close(p[0]);
+ close(p[1]);
+ execlp("convert", "convert", "-depth", "8", filename, "ppm:-",
NULL);
+ exit(1);
+ default: /* parent */
+ close(p[1]);
+ fp = fdopen(p[0], "r");
+ if (NULL == fp)
+ return NULL;
+ loader = &ppm_loader;
+ }
}
/* load image */
