From owner-freebsd-bugs Wed Feb 5 01:06:38 1997 Return-Path: Received: (from root@localhost) by freefall.freebsd.org (8.8.5/8.8.5) id BAA25201 for bugs-outgoing; Wed, 5 Feb 1997 01:06:38 -0800 (PST) Received: from spitfire.ecsel.psu.edu (qmailr@spitfire.ecsel.psu.edu [146.186.218.51]) by freefall.freebsd.org (8.8.5/8.8.5) with SMTP id BAA25185 for ; Wed, 5 Feb 1997 01:06:32 -0800 (PST) Received: (qmail 8212 invoked by uid 1000); 5 Feb 1997 08:59:47 -0000 Message-ID: <19970205085947.8211.qmail@spitfire.ecsel.psu.edu> To: Joerg Wunsch cc: freebsd-bugs@freefall.freebsd.org, ache@freebsd.org Subject: Re: misc/2654 In-reply-to: Your message of "Tue, 04 Feb 1997 14:17:13 PST." <199702042217.OAA16975@freefall.freebsd.org> Date: Wed, 05 Feb 1997 03:59:47 -0500 From: Dan Cross Sender: owner-bugs@freebsd.org X-Loop: FreeBSD.org Precedence: bulk > Already fixed in -current by Andrey in a less blatant way. No, I'm not smoking my hair, as it turns out. I'm quite positive that the bug still exists (though less obviously) in -current. Here are some excerpts from setlocale.c as obtained from ftp.freebsd.org, /pub/FreeBSD/FreeBSD-current/src/lib/locale a few hours ago (run through pr -tn for reference...). (btw- yes, I need to set up CVSup and haven't had the time. :-( Maybe this weekend, weather and time permitting.. [please don't ask about the weather part. :-)]) Take the case of a program which calls setlocale(LC_ALL, ""). The code in setlocale does it's thing until we reach this point: 175 if (category) 176 return (loadlocale(category)); 177 178 for (i = 1; i < _LC_LAST; ++i) { 179 (void)strcpy(saved_categories[i], current_categories[i]); 180 if (loadlocale(i) == NULL) { 181 for (j = 1; j < i; j++) { 182 (void)strcpy(new_categories[j], 183 saved_categories[j]); 184 /* XXX can fail too */ 185 (void)loadlocale(j); 186 } 187 return (NULL); 188 } 189 } 190 return (currentlocale()); Since LC_ALL == 0, the category test fails and we proceed to the for loop. Imagine that PATH_LOCALE is set to a really long string with the standard cracker stack frame encoded into it, etc. The first call to loadlocale(i) hit's this code: 235 if (!_PathLocale) { 236 if ( !(ret = getenv("PATH_LOCALE")) 237 || getuid() != geteuid() 238 || getgid() != getegid() 239 ) 240 _PathLocale = _PATH_LOCALE; 241 else if ( strlen(ret) + 45 > PATH_MAX 242 || !(_PathLocale = strdup(ret)) 243 ) 244 return (NULL); 245 } Which then notices that _PathLocale is NULL, and does it's thing. Assume that we are running this program a ``couple of level's deep'' inside of a setuid program, so the effective and real UID and GID match up. Okay, at this point, loadlocale(i) returns, passing NULL back to setlocale(3). Note also the call to strdup(3) inside the if statement, on the right of the logical Or. Since the first part of the test fails, strdup(3) is called on ret and _PathLocale is set to out nasty string (assuming we have enough free memory for strdup(3) to be successful. :-) Setlocale(3) then continues to process the loop for the LC_ALL case, inside the if statement testing the return value from loadloacale(), and calling loadlocale() for the other categories (as seen on line 184, the return value from loadlocale() is ignored in this inner loop, with an XXX comment saying, ``This can fail too...''). The intention here is to set up the locale's for the other categories, and then return NULL as an indication to the routine calling setlocale(3) that ``hey, at least one category failed to load''. Anyway, more calls are made to loadlocale(), but this time, _PathLocale is SET, as a result of placing strdup(3) into the if statement on line 242. Thus, on subsequent invocations of load- locale(), the entire block of range checking code for _PathLocale is ignored, and the individual load functions for the various categories call strcpy(3) and friends without having completed the range checking which they THINK they have, and our stack frame encoded in our extra- long PATH_LOCALE environment variable is happily written onto our real stack, causing much havoc. While this might be an odd combination of events to cause this problem, it *is* a problem, and should be addressed. The obvious fix for this, is to move the strdup(3) call out of the if statement on line 242, and onto a line by itself, AFTER the length of ret has been checked. Here's a patch: *** setlocale.c 1997/02/05 08:38:18 1.1 --- setlocale.c 1997/02/05 08:39:02 *************** *** 238,247 **** || getgid() != getegid() ) _PathLocale = _PATH_LOCALE; ! else if ( strlen(ret) + 45 > PATH_MAX ! || !(_PathLocale = strdup(ret)) ! ) ! return (NULL); } if (strcmp(new, old) == 0) --- 238,247 ---- || getgid() != getegid() ) _PathLocale = _PATH_LOCALE; ! else if (strlen(ret) + 45 > PATH_MAX) ! return(NULL); ! else if ((_PathLocale = strdup(ret)) == NULL) ! return(NULL); } if (strcmp(new, old) == 0) I believe that the truth tables should work out to be the same. :-) That's my take on it, at least. If I'm wrong (and I hope I am...), I'd love to hear about it, but I'd rather make a fool out of myself in the sake of security than keep my mouth shut for fear of being laughed at. :-) btw- it looks as though some the entire locale code could use a looking at and some cosmetic changes. If anyone likes, I could do some of this at home and then send-pr my diff's so that someone could review and either commit them to the tree, or send them back to me saying, ``these are rubbish, please do them CORRECTLY.'' :-) > Probably needs to be fixed in 2.1.x, but i'll leave it to > Joe to merge the fix from -current. Okay, cool. Thanks for getting back to me on this... - Dan C.