[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Accepted lesstif1-1 1:0.93.94-11.3 (i386 source all)



Matej Vela wrote:
>    * NMU.
>    * lib/Xm/LTXpm.c: Backport previous security fixes to lesstif1
>      (CAN-2004-0914, CAN-2005-0605).  Thanks to Kimmo Jukarainen for
>      doing the bulk of the work!  Closes: #294099.
>    * lib/Xm-2.1/RCUtils.c: Apply upstream fix for label positioning in
>      menus.  Closes: #279402.

Had a look for testing propigation purposes.

This seems broken:

-           sprintf(buf, "compress > \"%s\"", filename);
-           if (!(mdata->stream.file = popen(buf, "w")))
+           snprintf(buf, sizeof(buf), "compress > \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "w")))

And this:

-           sprintf(buf, "gzip -q > \"%s\"", filename);
-           if (!(mdata->stream.file = popen(buf, "w")))
+           snprintf(buf, sizeof(buf), "gzip -q > \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "w")))

Because s_popen does not support redirection and so on since it avoids
accessing the shell:

+** This is a secure but NOT 100% compatible replacement for popen()
+** Note:        - don't use pclose() use fclose() for closing the returned
+**                filedesc.!!!
+**
+** Known Bugs:  - unable to use i/o-redirection like > or <

I didn't check to see if NO_ZPIPE gets defined; if it does the blocks of
code above won't be built in. And yes, I noticed the same code already
exists in Xm-2.1/Xpm.c, also in #ifndef NO_ZPIPE.


Another problem with this s_popen is that it doesn't interpolate quotes
the way the shell does, so calls to s_popen (or popen with the above
#define) like these all won't work:

+               snprintf(buf, sizeof(buf), "uncompress -c \"%s\"", compressfile);
+               if (!(mdata->stream.file = s_popen(buf, "r"))) {

+                   snprintf(buf, sizeof(buf), "gunzip -c \"%s\"", compressfile);
+                   if (!(mdata->stream.file = s_popen(buf, "r"))) {

+           snprintf(buf, sizeof(buf), "uncompress -c \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "r")))

+           snprintf(buf, sizeof(buf), "gunzip -qc \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "r")))

+           snprintf(buf, sizeof(buf), "gzip -q > \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "w")))

+           snprintf(buf, sizeof(buf), "compress > \"%s\"", filename);
+           if (!(mdata->stream.file = s_popen(buf, "w")))

For example, this last one ends up calling 
execvp("compress", "compress", ">", "\"%s\"", filename).


This looks iffy:

+#ifndef NO_ZPIPE
+/*     FILE *s_popen(char *cmd, const char *type); */
+#else
+#      define s_popen popen
+#endif

I couldn't find any unpatches occurances of popen() in LTXpm.c. If there
were some and this is defined, it will break them because:
   1. Like the comment above says, s_popen() calls must be paired with
      fclose(), not pclose().
   2. They'd be subject to the shell interpolation problems described
      above.


I think this is broken; data_size is used undefined AFAICS:

@@ -7027,6 +7289,7 @@
 static void
 CreateExtensions(
     char **dataptr,
+    unsigned int data_size,
     unsigned int offset,
     _LtXpmExtension *ext,
     unsigned int num,
@@ -7039,12 +7302,12 @@
     dataptr++;
     a = 0;
     for (x = 0; x < num; x++, ext++) {
-       sprintf(*dataptr, "XPMEXT %s", ext->name);
+       snprintf(*dataptr, data_size, "XPMEXT %s", ext->name);

A similar problem is in the patch to CreatePixels and WritePixels2 and
WriteExtensions2.

-- 
see shy jo

Attachment: signature.asc
Description: Digital signature


Reply to: