Date: Tue, 17 Aug 1999 12:26:09 -0700 (PDT) From: Archie Cobbs <archie@whistle.com> To: des@flood.ping.uio.no (Dag-Erling Smorgrav) Cc: igusarov@chat.ru (Igor Gousarov), freebsd-hackers@freebsd.org Subject: Re: Unsafe code in libc in 3.0-RELEASE FreeBSD i386 Message-ID: <199908171926.MAA04640@bubba.whistle.com> In-Reply-To: <xzpemhauyru.fsf@flood.ping.uio.no> from Dag-Erling Smorgrav at "Aug 11, 1999 05:23:33 pm"
next in thread | previous in thread | raw e-mail | index | archive | help
Dag-Erling Smorgrav writes:
> Archie Cobbs <archie@whistle.com> writes:
> > Igor Gousarov writes:
> > > The source file for setlocale function (/usr/src/lib/libc/locale/setlocale.c)
> > > contains the line which might put libc into infinite loop:
> > > [...]
> > Please file a PR to make sure that this doesn't "slip through
> > the cracks"...
>
> It seems to have slipped through the cracks. Good thing I had a
> process mark on this message. What do you think of the attached patch
> (against -CURRENT)?
>
> I think there's still a possibility of new_categories being overrun,
> since there's no bounds checking on i in the do ... while (*locale)
> loop. I suggest that a careful audit by somebody who knows this code
> (or at least knows what it's supposed to do).
Sorry for the late reply..
I think I understand what that do { } while loop is trying to do.
Basically, LC_ALL can either be a single locale, in which case all
categories should get that locale, or else several locales all
separated by slashes, in which case consecutive categories get
the respective locales.
I've re-written your patch and simplified it a bit. Let me know what
you think (ie, please review).
Thanks,
-Archie
___________________________________________________________________________
Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com
Index: setlocale.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/locale/setlocale.c,v
retrieving revision 1.23
diff -u -r1.23 setlocale.c
--- setlocale.c 1998/04/29 22:39:56 1.23
+++ setlocale.c 1999/08/17 19:23:31
@@ -105,8 +105,8 @@
int category;
const char *locale;
{
- int i, j, len;
- char *env, *r;
+ int i, j;
+ char *env;
if (category < LC_ALL || category >= _LC_LAST)
return (NULL);
@@ -150,32 +150,26 @@
(void)strncpy(new_categories[category], locale, ENCODING_LEN);
new_categories[category][ENCODING_LEN] = '\0';
} else {
- if ((r = strchr(locale, '/')) == NULL) {
- for (i = 1; i < _LC_LAST; ++i) {
- (void)strncpy(new_categories[i], locale, ENCODING_LEN);
- new_categories[i][ENCODING_LEN] = '\0';
- }
- } else {
- for (i = 1; r[1] == '/'; ++r);
- if (!r[1])
- return (NULL); /* Hmm, just slashes... */
- do {
- len = r - locale > ENCODING_LEN ? ENCODING_LEN : r - locale;
- (void)strncpy(new_categories[i], locale, len);
- new_categories[i][len] = '\0';
- i++;
- locale = r;
- while (*locale == '/')
- ++locale;
- while (*++r && *r != '/');
- } while (*locale);
- while (i < _LC_LAST)
- (void)strcpy(new_categories[i],
- new_categories[i-1]);
+ char *s, buf[(_LC_LAST - 1) * (ENCODING_LEN + 1)];
+
+ /* Parse out setting(s) separated by slashes */
+ buf[sizeof(buf) - 1] = '\0';
+ (void)strncpy(buf, locale, sizeof(buf));
+ if (buf[sizeof(buf) - 1] != '\0')
+ return (NULL); /* arg(s) too long */
+ for (i = 1, s = strtok(buf, "/");
+ i < _LC_LAST && s != NULL;
+ ++i, s = strtok(NULL, "/")) {
+ (void)strncpy(new_categories[i], s, ENCODING_LEN);
+ new_categories[i][ENCODING_LEN] = '\0';
}
+
+ /* Copy last setting for all remaining categories */
+ for (; i < _LC_LAST; i++)
+ (void)strcpy(new_categories[i], new_categories[i - 1]);
}
- if (category)
+ if (category != LC_ALL)
return (loadlocale(category));
for (i = 1; i < _LC_LAST; ++i) {
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199908171926.MAA04640>