Bug 1920 - XPM Security patch still missing in CVS head
XPM Security patch still missing in CVS head
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Lib/Xpm
git
x86 (IA32) Linux (All)
: high normal
Assigned To: Matthieu Herrb
:
Depends on: 1924
Blocks:
  Show dependency treegraph
 
Reported: 2004-11-25 08:16 UTC by Stefan Dirsch
Modified: 2005-04-02 20:36 UTC (History)
7 users (show)

See Also:


Attachments
This is the patch for 6.8.1 (54.11 KB, patch)
2004-11-26 15:08 UTC, Matthieu Herrb
no flags Details | Splinter Review
remove string parsing from decompressor call (5.26 KB, patch)
2004-12-04 14:59 UTC, Alex Riesen
no flags Details | Splinter Review
remove string parsing from compressor call (2.46 KB, patch)
2004-12-04 15:00 UTC, Alex Riesen
no flags Details | Splinter Review
simplify image name generation (1.87 KB, patch)
2004-12-04 15:06 UTC, Alex Riesen
no flags Details | Splinter Review
RdFToI.c, the really patched (6.70 KB, text/plain)
2004-12-07 13:24 UTC, Alex Riesen
no flags Details
remove string parsing from decompressor call (5.28 KB, patch)
2004-12-07 13:32 UTC, Alex Riesen
no flags Details | Splinter Review
[FIXED_X11R68x] Merged patch (52.23 KB, patch)
2004-12-11 08:26 UTC, Matthieu Herrb
roland.mainz: 6.8‑branch+
Details | Splinter Review
Proposed fix for this issue (1.10 KB, patch)
2005-02-14 13:41 UTC, Matthieu Herrb
no flags Details | Splinter Review
New patch, filtering negative image->bitmap_unit (1.57 KB, patch)
2005-02-15 13:36 UTC, Matthieu Herrb
no flags Details | Splinter Review
New patch, filtering negative image->bitmap_unit (1.38 KB, patch)
2005-02-15 13:41 UTC, Matthieu Herrb
matthieu.herrb: 6.8‑branch?
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Dirsch 2004-11-25 08:16:32 UTC
Looks like the XPM Security patch is still missing in CVS head.

  http://www.x.org/pub/X11R6.8.1/patches/xorg-681-CAN-2004-0914.patch

It should also be included for 6.8.2 release.
Comment 1 Matthieu Herrb 2004-11-25 13:15:53 UTC
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? 
Comment 2 Stefan Dirsch 2004-11-26 03:29:15 UTC
Please keep Bug #1924 in mind. 
Comment 3 Roland Mainz 2004-11-26 03:37:35 UTC
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).
Comment 4 Matthieu Herrb 2004-11-26 15:08:40 UTC
Created attachment 1406 [details] [review]
This is the patch for 6.8.1
Comment 5 Alex Riesen 2004-12-04 14:59:03 UTC
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).
Comment 6 Alex Riesen 2004-12-04 15:00:31 UTC
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.
Comment 7 Alex Riesen 2004-12-04 15:06:19 UTC
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;
}
Comment 8 Alex Riesen 2004-12-04 15:15:39 UTC
sorry for accidentally obsoleting Thomas' patch.
Comment 9 Thomas Biege 2004-12-07 02:42:39 UTC
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.
Comment 10 Alex Riesen 2004-12-07 13:22:00 UTC
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.
Comment 11 Alex Riesen 2004-12-07 13:24:26 UTC
Created attachment 1489 [details]
RdFToI.c, the really patched

I really shouldn't post patches after eight...
Comment 12 Roland Mainz 2004-12-07 13:31:18 UTC
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... :)
Comment 13 Alex Riesen 2004-12-07 13:32:18 UTC
Created attachment 1490 [details] [review]
remove string parsing from decompressor call

fixed the patch. All thanks go to Thomas.
Comment 14 Alex Riesen 2004-12-07 13:45:01 UTC
(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?
Comment 15 Roland Mainz 2004-12-07 13:53:53 UTC
(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.
Comment 16 Thomas Biege 2004-12-08 02:41:46 UTC
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. 
Comment 17 Matthieu Herrb 2004-12-11 06:25:35 UTC
(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. 
Comment 18 Matthieu Herrb 2004-12-11 08:26:12 UTC
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.
Comment 19 Alex Riesen 2004-12-13 14:38:13 UTC
(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?
Comment 20 Alex Riesen 2004-12-13 14:48:55 UTC
(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?
Comment 21 Alex Riesen 2004-12-13 15:00:25 UTC
(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).
Comment 22 Thomas Biege 2004-12-14 02:16:27 UTC
yes, I recognized it yesterday. "--" isn't needed for your implementation.
Comment 23 Roland Mainz 2004-12-14 11:54:41 UTC
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 24 Roland Mainz 2004-12-16 17:10:00 UTC
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...
Comment 25 Chris Gilbert 2005-02-14 05:51:57 UTC
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.
Comment 26 Thomas Biege 2005-02-14 06:23:40 UTC
oops, that is indeed evil. :(
Comment 27 Roland Mainz 2005-02-14 06:36:38 UTC
(In reply to comment #26)
> oops, that is indeed evil. :(

Yeah... ;-(

Comment 28 Roland Mainz 2005-02-14 06:38:23 UTC
(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 ?
Comment 29 Matthieu Herrb 2005-02-14 13:41:21 UTC
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.
Comment 30 Thomas Biege 2005-02-15 08:48:34 UTC
- 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
 
Comment 31 Matthieu Herrb 2005-02-15 13:23:59 UTC
(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. 
Comment 32 Matthieu Herrb 2005-02-15 13:36:21 UTC
Created attachment 1908 [details] [review]
New patch, filtering negative image->bitmap_unit
Comment 33 Matthieu Herrb 2005-02-15 13:41:44 UTC
Created attachment 1909 [details] [review]
New patch, filtering negative image->bitmap_unit

(previous version was wrong)
Comment 34 Thomas Biege 2005-02-16 01:17:39 UTC
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. :)
Comment 35 Matthieu Herrb 2005-02-16 02:25:06 UTC
(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. 
Comment 36 Matthieu Herrb 2005-02-18 14:55:57 UTC
Attachement #1909 commited to HEAD.
Comment 37 Thomas Biege 2005-03-01 00:04:19 UTC
Will this be the official patch?

Is this bug already public (CVS, mailing list, here)?
Comment 38 Matthieu Herrb 2005-03-01 00:52:17 UTC
(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. 
Comment 39 Matthieu Herrb 2005-03-02 14:11:09 UTC
For further reference, CAN-2005-0605 has been assigned to this last issue. 
Comment 40 ajax at nwnk dot net 2005-04-03 14:36:31 UTC
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.