Date: Wed, 05 Feb 1997 03:59:47 -0500 From: Dan Cross <tenser@spitfire.ecsel.psu.edu> To: Joerg Wunsch <joerg@freefall.freebsd.org> Cc: freebsd-bugs@freefall.freebsd.org, ache@freebsd.org Subject: Re: misc/2654 Message-ID: <19970205085947.8211.qmail@spitfire.ecsel.psu.edu> In-Reply-To: Your message of "Tue, 04 Feb 1997 14:17:13 PST." <199702042217.OAA16975@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19970205085947.8211.qmail>