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.
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.