Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 21 Feb 2013 11:50:27 -0500
From: Rich Felker <dalias@...ifal.cx>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] write strcasestr

On Thu, Feb 21, 2013 at 10:40:38AM -0500, Strake wrote:
> >From 6a228eaf8bc2b3830c56bd2c8caa2ffa44245531 Mon Sep 17 00:00:00 2001
> From: Strake <strake888@...il.com>
> Date: Thu, 21 Feb 2013 10:41:09 -0500
> Subject: [PATCH] write strcasestr
> 
> ---
>  src/string/strcasestr.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/src/string/strcasestr.c b/src/string/strcasestr.c
> index f1cb0e8..c67c606 100644
> --- a/src/string/strcasestr.c
> +++ b/src/string/strcasestr.c
> @@ -1,7 +1,35 @@
> +#include <stdlib.h>
>  #include <string.h>
> +#include <wchar.h>
> +#include <stdio.h>
> 
> -char *strcasestr(const char *h, const char *n)
> -{
> -	//FIXME!
> -	return strstr(h, n);
> +static void mbstolower (char *s) {
> +	mbstate_t mbst;
> +	wchar_t wx;
> +	
> +	while (*s) {
> +		memset (&mbst, 0, sizeof (mbstate_t));
> +		if (mbrtowc (&wx, s, MB_CUR_MAX, &mbst) < 0) break;

The return value of mbrtowc is unsigned so it can never be < 0.

> +		wx = towlower (wx);
> +		memset (&mbst, 0, sizeof (mbstate_t));
> +		s += wcrtomb (s, wx, &mbst);
> +	}
> +}
> +
> +char *strcasestr (const char *s, const char *t) {
> +	char *x, *y, *z;
> +	
> +	x = strdup (s); if (!x) return 0;
> +	y = strdup (t); if (!y) return 0;

This function does not have a way to report failure, so it cannot
fail; therefore, allocating storage is not an option. Returning zero
means the needle is not present in the haystack, not that the
implementation ran out of resources to look for it.

For a concrete example of where false negatives would be a security
problem, see Busybox's use of the function.

> +	z = strstr (x, y);
> +	if (z) s += z - x; else s = 0;

This logic is also incorrect. It assumes the length of the lowercase
characters is the same as the length of the corresponding uppercase
ones; this is in general false. Actually, this means your algorithm
has another serious security flaw: it may overrun the allocated
buffer when performing the case conversion.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.