Hi Gregor,
* gregor herrmann <[EMAIL PROTECTED]> [2008-05-18 15:40]:
> On Tue, 13 May 2008 01:19:19 +0200, Marco d'Itri wrote:
> > Security team: libuu-dev is a static-only library (see #216593).
> > klibido, nget and slrn build-depend on libuu-dev, while
> > libconvert-uulib-perl and kde (I don't know exactly which package,
> > look in the kdesupport directory) contain an embedded copy.
> >
> > This code in uulib/uunconc.c is vulnerable to symlink attacks.
> >
> > if ((data->binfile = tempnam (NULL, "uu")) == NULL) {
> > UUMessage (uunconc_id, __LINE__, UUMSG_ERROR,
> > uustring (S_NO_TEMP_NAME));
> > return UURET_NOMEM;
> > }
> >
> > if ((dataout = fopen (data->binfile, mode)) == NULL) {
>
> I took a look at uulib/uunconc.c in libconvert-uulib-perl and I have
> the impression that it's not vulnerable because it uses mkstemp
> instead of tempnam if available.
>
> This was also already mentioned in
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=320541#30
>
> Still I'd appreciate if someone who speaks better C than me could
> take a look to verify.Confirmed, the version of uunconc.c in libconvert-uulib-perl is not vulnerable. Added this to the security tracker. Thanks for checking! Attached is an updated patch which ports the changes made in libconvert-uulib-perl·to uudeview. Please use this patch instead of the other one as the first one misses the second tempnam call. Kind regards Nico P.S. closing 481048 -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted.
diff -u uudeview-0.5.20/uulib/uunconc.c uudeview-0.5.20/uulib/uunconc.c
--- uudeview-0.5.20/uulib/uunconc.c
+++ uudeview-0.5.20/uulib/uunconc.c
@@ -1311,6 +1311,11 @@
char *mode, *ntmp;
uufile *iter;
size_t bytes;
+#ifdef HAVE_MKSTEMP
+ int tmpfd;
+ const char *tmpprefix = "uuXXXXXX";
+ char *tmpdir = NULL;
+#endif /* HAVE_MKSTEMP */
if (data == NULL || data->thisfile == NULL)
return UURET_ILLVAL;
@@ -1329,13 +1334,35 @@
else
mode = "wbx"; /* otherwise in binary */
+#ifdef HAVE_MKSTEMP
+ if ((getuid()==geteuid()) && (getgid()==getegid())) {
+ tmpdir=getenv("TMPDIR");
+ }
+
+ if (!tmpdir) {
+ tmpdir = "/tmp";
+ }
+ data->binfile = malloc(strlen(tmpdir)+strlen(tmpprefix)+2);
+
+ if (!data->binfile) {
+#else
if ((data->binfile = tempnam (NULL, "uu")) == NULL) {
+#endif /* HAVE_MKSTEMP */
UUMessage (uunconc_id, __LINE__, UUMSG_ERROR,
uustring (S_NO_TEMP_NAME));
return UURET_NOMEM;
}
+#ifdef HAVE_MKSTEMP
+ strcpy(data->binfile, tmpdir);
+ strcat(data->binfile, "/");
+ strcat(data->binfile, tmpprefix);
+
+ if ((tmpfd = mkstemp(data->binfile)) == -1 ||
+ (dataout = fdopen(tmpfd, mode)) == NULL) {
+#else
if ((dataout = fopen (data->binfile, mode)) == NULL) {
+#endif /* HAVE_MKSTEMP */
/*
* we couldn't create a temporary file. Usually this means that TMP
* and TEMP aren't set
@@ -1343,11 +1370,18 @@
UUMessage (uunconc_id, __LINE__, UUMSG_ERROR,
uustring (S_WR_ERR_TARGET),
data->binfile, strerror (uu_errno = errno));
+#ifdef HAVE_MKSTEMP
+ if (tmpfd != -1) {
+ unlink(data->binfile);
+ close(tmpfd);
+ }
+#endif /* HAVE_MKSTEMP */
_FP_free (data->binfile);
data->binfile = NULL;
uu_errno = errno;
return UURET_IOERR;
}
+
/*
* we don't have begin lines in Base64 or plain text files.
*/
@@ -1438,8 +1472,8 @@
break;
}
UUMessage (uunconc_id, __LINE__, UUMSG_MESSAGE,
- uustring (S_OPEN_FILE),
- iter->data->sfname);
+ uustring (S_OPEN_FILE),
+ iter->data->sfname);
_FP_strncpy (uugen_fnbuffer, iter->data->sfname, 1024);
}
@@ -1499,7 +1533,13 @@
*/
if (data->uudet == BH_ENCODED && data->binfile) {
+#ifdef HAVE_MKSTEMP
+ ntmp = malloc(strlen(tmpdir)+strlen(tmpprefix)+2);
+
+ if (ntmp == NULL) {
+#else
if ((ntmp = tempnam (NULL, "uu")) == NULL) {
+#endif /* HAVE_MKSTEMP */
UUMessage (uunconc_id, __LINE__, UUMSG_ERROR,
uustring (S_NO_TEMP_NAME));
progress.action = 0;
@@ -1513,15 +1553,31 @@
free (ntmp);
return UURET_IOERR;
}
+
+#ifdef HAVE_MKSTEMP
+ strcpy(ntmp, tmpdir);
+ strcat(ntmp, "/");
+ strcat(ntmp, tmpprefix);
+ if ((tmpfd = mkstemp(ntmp)) == -1 ||
+ (dataout = fdopen(tmpfd, "wb")) == NULL) {
+#else
if ((dataout = fopen (ntmp, "wb")) == NULL) {
+#endif /* HAVE_MKSTEMP */
UUMessage (uunconc_id, __LINE__, UUMSG_ERROR,
uustring (S_NOT_OPEN_TARGET),
ntmp, strerror (uu_errno = errno));
progress.action = 0;
fclose (datain);
+#ifdef HAVE_MKSTEMP
+ if (tmpfd != -1) {
+ unlink(ntmp);
+ close(tmpfd);
+ }
+#endif /* HAVE_MKSTEMP */
free (ntmp);
return UURET_IOERR;
}
+
/*
* read fork lengths. remember they're in Motorola format
*/
pgp0A1ImVXilY.pgp
Description: PGP signature
