Bug 1920

fd.org was not reachable when the patch has become public. I've not had time
to take care of this before. 
I'll commit the patch shortly. 
Roland, can I commit to the 6.8 branch directly or do I need to request approval
1st? 

Please keep Bug #1924 in mind. 

Matthieu Herrb wrote: 
> Roland, can I commit to the 6.8 branch directly or do I need to request 
> approval 1st?

It may be better to request approval first -  we may want to wait anyway until
bug 1924 ("XPM security fixes break writing XPM files with absolute path names")
has been resolved and then take both patches in one step to ensure that the
"stable" branch still has a working libXpm... :)

Additionally we have to wait with the commits to the stable branch until the
xorg-commit messages work again (curently they only work for some people and not
everyone, for example my and your commits are not being listed... ;-(( ) that
everyone can monitor what's being commited to it (that's the reason why I didn't
commit the already approved patches yet).

Created attachment 1406 [details] [review]
This is the patch for 6.8.1

Created attachment 1469 [details] [review]
remove string parsing from decompressor call

Can I suggest avoid using s_popen from the previous patch?
It breaks writing, and introduces a bug in case if compressor
program could not be started (the started child can go wild).
It also does not check return code of dup2, allocates and
fragments a lot of memory, does not handle waitpid interrupt
by a signal (i.e. SIGWINCH).
The proposed patch restricts the created pipe to only one program
with only one argument, as it is always the case in libXpm. It
also does not pollute application's namespace with overly generic
names (the introduced symbol is xpmPipeThrough).

Created attachment 1470 [details] [review]
remove string parsing from compressor call

This is the same for writing code. Patches are against unmodified 6.8.1 code.

Created attachment 1471 [details] [review]
simplify image name generation

The last patch makes the function of the code in XpmWriteFileFromXpmImage
a lot more obvious.

All patches are tested with the code below and share/Xm/pixmaps/crab.xpm
(gzipped/compressed too).

#include <stdio.h>
#include <string.h>
#include <X11/xpm.h>

int main(int argc, char* argv[])
{
    XpmImage img;
    XpmInfo info;
    memset(&img, 0, sizeof(img));
    memset(&info, 0, sizeof(info));
    int err = XpmReadFileToXpmImage(argv[1], &img, &info);
    if ( err )
	printf("err=%d\n", err);
    else
    {
	printf("w=%u, h=%u, cpp=%u, cols=%u, vmask=%08lx, hotspot=%u,%u\n",
	       img.width,img.height,img.cpp,img.ncolors,info.valuemask,
	       info.valuemask & XpmHotspot ? info.x_hotspot: 0,
	       info.valuemask & XpmHotspot ? info.y_hotspot: 0);
    }
    if ( argv[2] )
    {
	err = XpmWriteFileFromXpmImage(argv[2], &img, &info);
	if ( err )
	    printf("err=%d\n", err);
    }
    return 0;
}

sorry for accidentally obsoleting Thomas' patch.

I've no problem with replacing my patches as long as the result is at least
equally secure. ;-)

concerning patch: https://bugs.freedesktop.org/attachment.cgi?id=1470


remark 1:
+	    strcpy(compressfile, filename);
+	    ext = strcpy(compressfile, ".Z");

The second strcpy() was intended to be a strcat(), right?


remakr 2:
+	    fd = open(filename, O_RDONLY);
+	    if ( fd < 0 )
+	    {
+		ext = strcpy(compressfile, ".gz");
+		fd = open(filename, O_RDONLY);
+		if ( fd < 0 )
+		{

I am a bit confused (maybe too much coffee ;) here. Shouldn't it be
open(compressfile)? (and again the strcpy vs. strcat thing)

remark 3:
+	    ext = strcpy(compressfile, ".Z");
[...]
+		ext = strcpy(compressfile, ".gz");

Assuming the strcat() remark above is correct, the result will become
"<filename>.Z.gz"... 


Can you send me (or attach) the file plus your changes please.
I like to review the whole file. Just looking at the patch may
result in wrong assumptions and parsing errors.
Thank you.

Can't believe it! It must be wrong patch, because I actually tested
the code. Thanks a lot for actually looking at it!

(In reply to comment #9)
> remark 1:
> +	    strcpy(compressfile, filename);
> +	    ext = strcpy(compressfile, ".Z");
> 
> The second strcpy() was intended to be a strcat(), right?

no, it had to be "strcpy(compressfile + len, ext = ".Z");" :)
ext is used later.

> remakr 2:
> +	    fd = open(filename, O_RDONLY);

compressfile

> +	    if ( fd < 0 )
> +	    {
> +		ext = strcpy(compressfile, ".gz");

same here: strcpy(compressfile + len, ext = ".gz").

> +		fd = open(filename, O_RDONLY);

compressfile

> +		if ( fd < 0 )
> +		{
> 
> I am a bit confused (maybe too much coffee ;) here. Shouldn't it be
> open(compressfile)? (and again the strcpy vs. strcat thing)

No-no, that me again. I should really look at what I send.

> remark 3:
> +	    ext = strcpy(compressfile, ".Z");
> [...]
> +		ext = strcpy(compressfile, ".gz");
> 
> Assuming the strcat() remark above is correct, the result will become
> "<filename>.Z.gz"... 
> 
> 
> Can you send me (or attach) the file plus your changes please.
> I like to review the whole file. Just looking at the patch may
> result in wrong assumptions and parsing errors.
> Thank you.

Of course.

Created attachment 1489 [details]
RdFToI.c, the really patched

I really shouldn't post patches after eight...

Just curious: Wouldn't it be a much better idea to use zlib directly as proposed
in bug 2009 ? That may reduce most of the pain of calling external applications
and may give a good perf. boost, too... :)

Created attachment 1490 [details] [review]
remove string parsing from decompressor call

fixed the patch. All thanks go to Thomas.

(In reply to comment #12)
> Just curious: Wouldn't it be a much better idea to use zlib directly as proposed
> in bug 2009 ? That may reduce most of the pain of calling external applications
> and may give a good perf. boost, too... :)

It also means an effort of rewriting all places were stdio is used,
like calls to fprintf to gzprintf, and it does not help at all if
want LZW (.Z) support: zlib also doesn't do it.

I'd suggest replace stdio with block based operations and select simple
read/write operations depending on file encoding.
I almost started doing it, but ... I mean, there is a lot of modern,
fast and very good written image libraries around. Does it make sense
to spend any time optimizing this one?

(In reply to comment #14)
> (In reply to comment #12)
> > Just curious: Wouldn't it be a much better idea to use zlib directly as proposed
> > in bug 2009 ? That may reduce most of the pain of calling external applications
> > and may give a good perf. boost, too... :)
> 
> It also means an effort of rewriting all places were stdio is used,
> like calls to fprintf to gzprintf, and it does not help at all if
> want LZW (.Z) support: zlib also doesn't do it.

Erm... did you read my comment in bug 2009 ? At least *.Z decompresson works
with gunzip...

> I'd suggest replace stdio with block based operations and select simple
> read/write operations depending on file encoding.

Ugh. You're kidding, right ?

> I almost started doing it, but ... I mean, there is a lot of modern,
> fast and very good written image libraries around.

Which of them really has Xpm support (e.g. AFAIK Mozilla's libpr0n doesn't have
Xpm support...) ?

> Does it make sense
> to spend any time optimizing this one?

"Yes" ... as it would simply end the neverending story of this bug and all the
regressions caused by it (OKOK, "... likely end ..." may be better here). And it
would make libXpm a better citizen in the unix world where the usage of |fork()|
may cause trouble for multi-threaded applications.

The file looks ok.

The remaining problem is a setuid application that is linked against libxpm,
very unlikely I think...


	    mdata->stream.file = xpmPipeThrough(fd, "uncompress", "-c", "r");
	}
	else if ( ext && !strcmp(ext, ".gz") )
	{
	    mdata->type = XPMPIPE;
	    mdata->stream.file = xpmPipeThrough(fd, "gunzip", "-qc", "r");

Without a cleaned up PATH variable (or by using full pathnames for the compress
tools) a local user can set PATH to ./ and let the setuid app run his binaries.

This was also mention by Alan Coopersmith from Sun.com.

I argued that it lies in the responsibility of the application to clean up the
environment and that we used it to stay compatible to popen(3),
Maybe we should re.think about this.

Another issue are filenames starting with '-' or '--'.

thomas@bragg:~> touch -- "--fast"
thomas@bragg:~> gzip "--fast"
gzip: compressed data not written to a terminal. Use -f to force compression.
For help, type: gzip -h
thomas@bragg:~> gzip -- "--fast"
thomas@bragg:~> l -- "--fast.gz"
-rw-r-----    1 thomas   suse           27 2004-12-08 11:34 --fast.gz
thomas@bragg:~> gunzip -- "--fast.gz"
thomas@bragg:~> l -- "--fast"
-rw-r-----    1 thomas   suse            0 2004-12-08 11:34 --fast
thomas@bragg:~>

The last argument right before the filename should be a "--".

I forgot (or maybe I had a reason for it, can't remember) this in my patch too.

Switching to zlib would remove all these funny issues. 

(In reply to comment #13)
> Created an attachment (id=1490) [edit]
> remove string parsing from decompressor call
> 
> fixed the patch. All thanks go to Thomas.

Alex, it would really have helped if you could have worked on a tree with
Thomas' patch applied. 
We now have 2 incompatibles patches to RdFtoI.c, which both contains chunks that
need to be applied. 

Created attachment 1514 [details] [review]
[FIXED_X11R68x] Merged patch

Here is a patch relative to 6.8.1 that merges Alex's diffs and Thomas other 
patches, as commited to the trunk.

(In reply to comment #18)
> Here is a patch relative to 6.8.1 that merges Alex's diffs and Thomas other 
> patches, as commited to the trunk.  

Thank you, Matthieu. I was working with 6.8.1 official, yes.

And I'm sorry for the horribly slow responses, was busy lately.
BTW, does anyone know how to force Bugzilla actually send mails
when a bug changes?

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Just curious: Wouldn't it be a much better idea to use zlib directly
> > > as proposed in bug 2009 ? That may reduce most of the pain of calling
> > > external applications and may give a good perf. boost, too... :)
> > 
> > It also means an effort of rewriting all places were stdio is used,
> > like calls to fprintf to gzprintf, and it does not help at all if
> > want LZW (.Z) support: zlib also doesn't do it.
> 
> Erm... did you read my comment in bug 2009 ? At least *.Z decompresson works
> with gunzip...

did you read my answer on your comment? How do you compress?

> > I'd suggest replace stdio with block based operations and select simple
> > read/write operations depending on file encoding.
> 
> Ugh. You're kidding, right ?

no. It'd be simpler, compacter and faster.

> > I almost started doing it, but ... I mean, there is a lot of modern,
> > fast and very good written image libraries around.
>
> Which of them really has Xpm support (e.g. AFAIK Mozilla's libpr0n
> doesn't have Xpm support...) ?

I dunno. It wasn't the point. The point was support of the fairly old
image format.

> > Does it make sense
> > to spend any time optimizing this one?
> 
> "Yes" ... as it would simply end the neverending story of this bug and
> all the regressions caused by it (OKOK, "... likely end ..." may be
> better here). And it would make libXpm a better citizen in the unix
> world where the usage of |fork()| may cause trouble for multi-threaded
> applications.

yes, and that is very good reason. Also SIGCHLD and SIGPIPE caused by the
current code are real problems.

So of course you're right, it should use zlib and in-library implementation
of LZW. Does anyone know an unpatented LZW packer?

(In reply to comment #16)
> Without a cleaned up PATH variable (or by using full pathnames for the
> compress tools) a local user can set PATH to ./ and let the setuid app
> run his binaries.

I see no way to fix this (except hardcoded path or own compressors).
 
> Another issue are filenames starting with '-' or '--'.
...
> The last argument right before the filename should be a "--".

1. The file name is never given to the programs, files are open directly
2. Not all systems have GNU getopt and not always a program with name
compress support "--" in the argument line (mine does not).

yes, I recognized it yesterday. "--" isn't needed for your implementation.

Comment on attachment 1514 [details] [review]
[FIXED_X11R68x] Merged patch

Approval for X11R6.8.x stable branch granted in the 2004-12-13
release-wranglers phone call.
Please don't commit yourself, I'll do that later myself...

Comment on attachment 1514 [details] [review]
[FIXED_X11R68x] Merged patch

Patch checked-in into X11R6.8.x stable branch:

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.365.2.104; previous revision: 1.365.2.103
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
/cvs/xorg/xc/extras/Xpm/lib/Attrib.c,v	<--  Attrib.c
new revision: 1.1.1.1.6.2; previous revision: 1.1.1.1.6.1
/cvs/xorg/xc/extras/Xpm/lib/CrBufFrI.c,v  <--  CrBufFrI.c
new revision: 1.2.4.1; previous revision: 1.2
/cvs/xorg/xc/extras/Xpm/lib/CrDatFrI.c,v  <--  CrDatFrI.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/RdFToBuf.c,v  <--  RdFToBuf.c
new revision: 1.1.1.1.6.1; previous revision: 1.1.1.1
/cvs/xorg/xc/extras/Xpm/lib/RdFToI.c,v	<--  RdFToI.c
new revision: 1.2.4.1; previous revision: 1.2
/cvs/xorg/xc/extras/Xpm/lib/WrFFrBuf.c,v  <--  WrFFrBuf.c
new revision: 1.1.1.1.6.1; previous revision: 1.1.1.1
/cvs/xorg/xc/extras/Xpm/lib/WrFFrI.c,v	<--  WrFFrI.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/XpmI.h,v  <--  XpmI.h
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/create.c,v	<--  create.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/data.c,v  <--  data.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/hashtab.c,v  <--  hashtab.c
new revision: 1.1.1.1.6.2; previous revision: 1.1.1.1.6.1
/cvs/xorg/xc/extras/Xpm/lib/misc.c,v  <--  misc.c
new revision: 1.1.1.1.6.1; previous revision: 1.1.1.1
/cvs/xorg/xc/extras/Xpm/lib/parse.c,v  <--  parse.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
/cvs/xorg/xc/extras/Xpm/lib/scan.c,v  <--  scan.c
new revision: 1.2.4.2; previous revision: 1.2.4.1
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
/cvs/xorg/xc/lib/Xpm/Imakefile,v  <--  Imakefile
new revision: 1.2.4.2; previous revision: 1.2.4.1
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
Mailing the commit message to xorg-commit@lists.freedesktop.org...

Having just looked at the 6.8.2 release, there's a couple of issues with the 
patch.  In a few places the code does:

unsigned int i;

for (i = nbytes; --i >=0;)
   *dst++ = *src++;

The compiler obviously says that i is unsigned, so i is never negative, and so -
-i>=0 will wrap i to UINT_MAX, so is always true.

The two places are create.c PutImagePixels and scan.c GetImagePixels.

oops, that is indeed evil. :(

(In reply to comment #26)
> oops, that is indeed evil. :(

Yeah... ;-(

(In reply to comment #25)
> Having just looked at the 6.8.2 release, there's a couple of issues with the 
> patch.  In a few places the code does:
> 
> unsigned int i;
> 
> for (i = nbytes; --i >=0;)
>    *dst++ = *src++;
> 
> The compiler obviously says that i is unsigned, so i is never negative, and so -
> -i>=0 will wrap i to UINT_MAX, so is always true.

Can you make a patch for that and get it reviewed by Thomas Biege, please ?

Created attachment 1903 [details] [review]
Proposed fix for this issue

This patch looks enough to me. In all cases the initial value for i is a signed
int.

- the signed 'i' seems not to trigger problems elsewhere
- Is "image->bitmap_unit" user-controled? If so we may encounter buffer
overflows again at this for-loops
 

(In reply to comment #30)

> - Is "image->bitmap_unit" user-controled? If so we may encounter buffer
> overflows again at this for-loops

image is of type XImage.  It's used only used when converting in-memory XImages
to XPM. So yes it's user-controlled, but one need a way to build an XImage with
a bad value in bitmap_unit. This is pretty unlikely, but it's worth fixing anyways. 

Created attachment 1908 [details] [review]
New patch, filtering negative image->bitmap_unit

Created attachment 1909 [details] [review]
New patch, filtering negative image->bitmap_unit

(previous version was wrong)

Well I tried to find the definition of struct XImage before adding my comment
here but wasn't successful. :)
All version I found did not contain the bitmap_unit element... but I think it's
a signed integer, right?
Maybe I was blinded by rush. What is the name of the header file containing XImage?

As I mentioned before along with my first review. This is just the tip of the
iceberg... unfortunately. There are more values from untrusted sources (image
files) which are used carelessly in loops and asignments.

Noone to blame here but this old and dusty code should be rewritten in another
language. :)

(In reply to comment #34)
> Well I tried to find the definition of struct XImage before adding my comment
> here but wasn't successful. :)
> All version I found did not contain the bitmap_unit element... but I think it's
> a signed integer, right?

Yes. 

> Maybe I was blinded by rush. What is the name of the header file containing
XImage?
> 
It's Xlib.h (in /usr/X11R6/include/X11/). 

> As I mentioned before along with my first review. This is just the tip of the
> iceberg... unfortunately. There are more values from untrusted sources (image
> files) which are used carelessly in loops and asignments.
 
> Noone to blame here but this old and dusty code should be rewritten in another
> language. :)

At least it should be rewritten with security in mind. I'm not sure the correct
language for this kind of low-level things exists. 

Attachement #1909 commited to HEAD.

Will this be the official patch?

Is this bug already public (CVS, mailing list, here)?

(In reply to comment #37)
> Will this be the official patch?

Yes, unless someone finds something wrong with it. 
> 
> Is this bug already public (CVS, mailing list, here)?

Yes. This bugzilla is public and so is the xorg-commit@freedesktop.org list. 

For further reference, CAN-2005-0605 has been assigned to this last issue. 

mass update: the fix for this bug was applied to both HEAD and the stable 6.8 branch, but the bug was 
never closed.  closing now.