Package: jpegoptim
Version: 1.2.3-2
Severity: important
Tags: patch, security
X-Debbugs-Cc: t...@iki.fi
(cc-ing Debian BTS report to jpegoptim upstream)
For each image that it processes, jpegoptim currently creates a
temporary file {destdir}/jpegoptim-{uid}-{pid}.tmp, where {destdir} is
either the directory specified with the -d option or the dirname of
the input filename, {uid} is the user id of the user running jpegoptim
and {pid} is the process id of the jpegoptim process. It doesn't check
whether the file already exists before opening it for writing.
In certain circumstances, if a local attacker succeeds in creating a
symlink with the same name, pointing to another file writeable to the
user running jpegoptim, that file would be overwritten. The attacker
would require sufficient permissions to {destdir}; in particular the
attack would work if {destdir} == /tmp/.
If jpegoptim is processing multiple files, the attacker wouldn't even
have to guess the pid, as jpegoptim reuses the temporary filename.
The attached patch uses mkstemp to create the temporary file, I think
this enough to solve the problem in the case {destdir} == /tmp/
(maybe jpegoptim should also check that the permissions on the
{destdir} path aren't too lax).
diff --git a/jpegoptim.c b/jpegoptim.c
index a216c66..57d689a 100644
--- a/jpegoptim.c
+++ b/jpegoptim.c
@@ -458,7 +458,7 @@ int main(int argc, char **argv)
strncpy(newname,argv[i],sizeof(newname));
}
snprintf(tmpfilename,sizeof(tmpfilename),
- "%sjpegoptim-%d-%d.tmp", tmpdir, (int)getuid(), (int)getpid());
+ "%sjpegoptXXXXXX", tmpdir);
}
retry_point:
@@ -576,9 +576,10 @@ int main(int argc, char **argv)
outfname=NULL;
if ((outfile=tmpfile())==NULL) fatal("error opening temp file");
} else {
+ int tmpfd = mkstemp(tmpfilename);
+ if (tmpfd == -1 || ((outfile=fdopen(tmpfd,"w"))==NULL))
+ fatal("error creating temporary file");
outfname=tmpfilename;
- if ((outfile=fopen(outfname,"w"))==NULL)
- fatal("error opening target file");
}
if (setjmp(jcerr.setjmp_buffer)) {
