http://forums.gentoo.org/viewtopic.php?t=224301&start=0 I think the code excerpt clear. The answer of the author is, that he doesn't see a problem, because "there will be no path longer than 4096 chars"...
Carlo could you please provide a short English summary so we all can understand the issue?
well, from what I understand of the issue: void check_program (char * szProgram, long lSprache) { struct stat tmpStat; char filename[255]; long bGefunden = 0; printf ("Test %s", szProgram); // Wenn kein Pfad gesetzt, dann im gesamten Pfad suchen if (!strstr (szProgram, "/")) { char szPfad[4096]; <-- here int i; char szVerzeichnis[1024]; strcpy (szPfad, getenv ("PATH")); <-- and here strcpy (szVerzeichnis, ""); for (i=0; i<strlen(szPfad); i++) { if (szPfad[i] == ':') ... so... if the PATH variable is greater than 4096 characters, buffer overflow occurs, program dies, wham bam thank you mam. I think the recommendation was to use strncopy so that excess data gets truncuated. As Carlos stated, the author of the package believes that path will never be greater than 4096. I highly doubt that however and am testing it now.
Exactly. The unpleasant part is the ignorant answer of the author of lxdvdrip. Do we really want software in Portage, if it is known, that the author is so uneducated about security issues? Chris: I'm still Carlo w/o s :)
Carlo: er.. woops :P oh yah, this is definately in issue.. First off: I made a neat little program I'll attach shortly that puts out a 5000 times. It's called test_exploit. Now then.. I go ahead and set my PATH to the regular PATH variable and the test exploit: root@secures /usr/portage/distfiles/lxdvdrip # export PATH="/sbin:/bin:/usr/sbin:/usr/bin:`/home/chris/test_exploit`" results will be pasted for that (don't want to spam you people ;) and now we run lxdvdrip: root@secures /usr/portage/distfiles/lxdvdrip # lxdvdrip -dl=/dev/hdc and: Test dvdauthorSegmentation Fault kiss it bye bye! Path can never be greater than 4096 eh ;).
Created attachment 39918 [details] test_exploit.c
Created attachment 39919 [details] sample_data.txt
There we go.. patched that nice and good. See what a simple little usage of the right function does :P : Test dvdauthor: OK Test streamdvd: Program not found i=ignore, else cancel: lxdvdrip is canceled! root@secures /usr/portage/distfiles/lxdvdrip # ok so.. I vote no glsa because it's ~ on all KEYWORDED archs since it was created. version 1.20_pre1-r1 is the safe version in portage.
hi, just for your interest: ... char szBefehl2[4096]; ... strcpy (szBefehl, getenv ("HOME")); strcat (szBefehl, "/.lxdvdrip.conf"); fConf=fopen(szBefehl, "r"); about lines 1052ff. the same problem with $HOME; also tested as exploitable... # export HOME=`perl -e 'printf "A" x 9500'` && ./lxdvdrip Segmentation fault btw: i'm sure, that there are other flaws here. someone would need to read through ~5200 lies of code... *uhhh* regards rootshell
Ah, thanks didn't catch that :). Fixed in portage. I scaned for any more getenv() type functions.. that was it. I may check this later on if I have time (lucky to get a break enough to fix this :)
hi again, sorry for interrupting again... ;-) i just had a look at the patch: it may be better to ensure, the dest string is (will always be) NUL terminated; to do so, the following is necessary: --- lxdvdrip.c_old 2004-09-19 14:49:07.013638160 +0900 +++ lxdvdrip.c 2004-09-19 15:21:42.472363248 +0900 @@ -333,7 +333,7 @@ char szPfad[4096]; int i; char szVerzeichnis[1024]; - strcpy (szPfad, getenv ("PATH")); + strncpy (szPfad, getenv ("PATH"), 4096); + szPath[sizeof(szPath)-1] = '\0'; strcpy (szVerzeichnis, ""); for (i=0; i<strlen(szPfad); i++) { @@ -1033,7 +1033,7 @@ // Auslesen der Parameter aus einer Datei // Zuerst lokal versuchen - strcpy (szBefehl, getenv ("HOME")); + strncpy (szBefehl, getenv ("HOME"), 4096); + szBefehl[sizeof(szBefehl)-1] = '\0'; strcat (szBefehl, "/.lxdvdrip.conf"); fConf=fopen(szBefehl, "r"); if (!fConf) these changes ensure, that the buffers always correctly end with \0, mo matter how long the source buffer is... best regards rootshell
oh sorry, typo: <<< + szPath[sizeof(szPath)-1] = '\0'; >>> + szPfad[sizeof(szPfad)-1] = '\0'; regards rootshell
Thx for the swift reaction Chris. This is unstable so no GLSA will be issued. Please close this bug when it's fixed.
wooo, woops. I was looking at strcpy's description when I did the patching (which.. adds the null at the end, but strncpy doesn't). This is why I shouldn't patch things until my cereal kicks in ;). In cvs fixed, closing the bug. Feel free to re-open if you notice any other lack of intelligent coding there-of. Thanks again for pointing the good stuff out :).
