[PATCH] fix buffer overflow in udev_util_encode_string()

[PATCH] fix buffer overflow in udev_util_encode_string()

[Florian Zumbiehl]

Hi,

this is broken in such a strange way that I am not really sure whether
I hit the semantics expected by the callers - I hope you will be able to
figure it out? Untested, of course.

Florian

diff --git a/libudev/libudev-util.c b/libudev/libudev-util.c
index b07eabb..9a141db 100644
--- a/libudev/libudev-util.c
+++ b/libudev/libudev-util.c
@@ -448,28 +448,29 @@ int udev_util_encode_string(const char *str, char *str_enc, size_t len)
 {
 	size_t i, j;
 
-	if (str = NULL || str_enc = NULL || len = 0)
+	if (str = NULL || str_enc = NULL)
 		return -1;
 
-	str_enc[0] = '\0';
 	for (i = 0, j = 0; str[i] != '\0'; i++) {
 		int seqlen;
 
 		seqlen = utf8_encoded_valid_unichar(&str[i]);
 		if (seqlen > 1) {
+			if(len-j<seqlen)goto err;
 			memcpy(&str_enc[j], &str[i], seqlen);
 			j += seqlen;
 			i += (seqlen-1);
 		} else if (str[i] = '\\' || !is_whitelisted(str[i], NULL)) {
+			if(len-j<4)goto err;
 			sprintf(&str_enc[j], "\\x%02x", (unsigned char) str[i]);
 			j += 4;
 		} else {
+			if(len-j<1)goto err;
 			str_enc[j] = str[i];
 			j++;
 		}
-		if (j+3 >= len)
-			goto err;
 	}
+	if(len-j<1)goto err;
 	str_enc[j] = '\0';
 	return 0;
 err:

Re: [PATCH] fix buffer overflow in udev_util_encode_string()

[Kay Sievers]

On Mon, Aug 31, 2009 at 19:33, Florian Zumbiehl<florz@florz.de> wrote:
> this is broken in such a strange way that I am not really sure whether
> I hit the semantics expected by the callers - I hope you will be able to
> figure it out? Untested, of course.

Applied.

Thanks,
Kay

Re: [PATCH] fix buffer overflow in udev_util_encode_string()

[Karel Zak]

On Tue, Sep 01, 2009 at 12:56:51PM +0200, Kay Sievers wrote:
> On Mon, Aug 31, 2009 at 19:33, Florian Zumbiehl<florz@florz.de> wrote:
> > this is broken in such a strange way that I am not really sure whether
> > I hit the semantics expected by the callers - I hope you will be able to
> > figure it out? Untested, of course.
> 
> Applied.

 Applied also to libblkid from util-linux-ng.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>

[PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Florian Zumbiehl]

Hi,

untested, ...

Florian

diff --git a/libudev/libudev-util.c b/libudev/libudev-util.c
index 9a141db..a2aef84 100644
--- a/libudev/libudev-util.c
+++ b/libudev/libudev-util.c
@@ -355,7 +355,7 @@ int udev_util_replace_whitespace(const char *str, char *to, size_t len)
 	size_t i, j;
 
 	/* strip trailing whitespace */
-	len = strnlen(str, len);
+	len = strnlen(str, len-1);
 	while (len && isspace(str[len-1]))
 		len--;
 

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Andrey Borzenkov]

[-- Attachment #1: Type: Text/Plain, Size: 599 bytes --]

On Friday 04 of September 2009 23:34:45 Florian Zumbiehl wrote:
> Hi,
> 
> untested, ...
> 
> Florian
> 
> diff --git a/libudev/libudev-util.c b/libudev/libudev-util.c
> index 9a141db..a2aef84 100644
> --- a/libudev/libudev-util.c
> +++ b/libudev/libudev-util.c
> @@ -355,7 +355,7 @@ int udev_util_replace_whitespace(const char *str,
>  char *to, size_t len) size_t i, j;
> 
>  	/* strip trailing whitespace */
> -	len = strnlen(str, len);
> +	len = strnlen(str, len-1);
>  	while (len && isspace(str[len-1]))
>  		len--;
> 
wrong. Please try to understand what this loop does.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Florian Zumbiehl]

Hi,

> > diff --git a/libudev/libudev-util.c b/libudev/libudev-util.c
> > index 9a141db..a2aef84 100644
> > --- a/libudev/libudev-util.c
> > +++ b/libudev/libudev-util.c
> > @@ -355,7 +355,7 @@ int udev_util_replace_whitespace(const char *str,
> >  char *to, size_t len) size_t i, j;
> > 
> >  	/* strip trailing whitespace */
> > -	len = strnlen(str, len);
> > +	len = strnlen(str, len-1);
> >  	while (len && isspace(str[len-1]))
> >  		len--;
> > 
> wrong.

Because?

> Please try to understand what this loop does.

And then?

Florian

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Alan Jenkins]

On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> > diff --git a/libudev/libudev-util.c b/libudev/libudev-util.c
>> > index 9a141db..a2aef84 100644
>> > --- a/libudev/libudev-util.c
>> > +++ b/libudev/libudev-util.c
>> > @@ -355,7 +355,7 @@ int udev_util_replace_whitespace(const char *str,
>> >  char *to, size_t len) size_t i, j;
>> >
>> >  	/* strip trailing whitespace */
>> > -	len = strnlen(str, len);
>> > +	len = strnlen(str, len-1);
>> >  	while (len && isspace(str[len-1]))
>> >  		len--;
>> >
>> wrong.
>
> Because?

before

strnlen("a", 2) = 1
isspace("a"[1-1]) = 0
-> final value of len = 1.
"a" fits in a 2 byte buffer.

after

strnlen("a", 2-1) = 0

-> "a" will be truncated to "" even though it would fit in a two-byte buffer

>> Please try to understand what this loop does.
>
> And then?
>
> Florian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Florian Zumbiehl]

Hi,

[...]
> after
> 
> strnlen("a", 2-1) = 0

| $ cat foo.c
| 
| #include <string.h>
| #include <stdio.h>
| 
| int main(){
| 	printf("%u\n",strnlen("a",2-1));
| 	return 0;
| }
| 
| $ gcc -o foo foo.c
| $ ./foo
| 1
| $ 

Florian

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Alan Jenkins]

On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
> [...]
>> after
>>
>> strnlen("a", 2-1) = 0
>
> | $ cat foo.c
> |
> | #include <string.h>
> | #include <stdio.h>
> |
> | int main(){
> | 	printf("%u\n",strnlen("a",2-1));
> | 	return 0;
> | }
> |
> | $ gcc -o foo foo.c
> | $ ./foo
> | 1
> | $
>
> Florian

Indeed, excuse my brainfart.  Looking at the rest of the function I
agree it needs fixing,  Unless Andrey can correct us.

I'm not quite sure about this fix.  String functions normally do
something reasonable when a length of 0 is passed.  It looks like this
fixed version implements "length 0 is a special value meaning no
limit" :-) due to arithmetic underflow.

Oh - and reading code this closely usually is boring.  Especially when
you're scanning, and don't necessarily have much idea of the bigger
picture.  Auditting edge-case stability is even less exciting than
auditting security.  So thanks for lending a fresh set of eyeballs for
a while!

Alan

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Andrey Borzenkov]

[-- Attachment #1: Type: Text/Plain, Size: 1433 bytes --]

On Saturday 05 of September 2009 22:17:52 Alan Jenkins wrote:
> On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> > Hi,
> >
> > [...]
> >
> >> after
> >>
> >> strnlen("a", 2-1) = 0
> >>
> > | $ cat foo.c
> > |
> > | #include <string.h>
> > | #include <stdio.h>
> > |
> > | int main(){
> > | 	printf("%u\n",strnlen("a",2-1));
> > | 	return 0;
> > | }
> > |
> > | $ gcc -o foo foo.c
> > | $ ./foo
> > | 1
> > | $
> >
> > Florian
> 
> Indeed, excuse my brainfart.  Looking at the rest of the function I
> agree it needs fixing,  Unless Andrey can correct us.
> 

This change breaks udev_util_replace_whitespace().

#include <stdio.h>
#include <stddef.h>

main()
{
        int len = 3, i, j;
        char *str = "a b";
        char to[10];

        len = strnlen(str, len - 1);
        while (len && isspace(str[len-1]))
                len--;

        j = i = 0;
        while (i < len) {
                /* substitute multiple whitespace with a single '_' */
                if (isspace(str[i])) {
                        while (isspace(str[i]))
                                i++;
                        to[j++] = '_';
                        }
                to[j++] = str[i++];
        }
        to[j] = '\0';

        printf("'%s'\n", to);
}

{pts/1}% gcc -O0 foo.c
{pts/1}% ./a.out
'a'

instead of expected a_b

What is exact problem with original code? 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Florian Zumbiehl]

Hi,

> Indeed, excuse my brainfart.  Looking at the rest of the function I
> agree it needs fixing,  Unless Andrey can correct us.
> 
> I'm not quite sure about this fix.  String functions normally do
> something reasonable when a length of 0 is passed.  It looks like this
> fixed version implements "length 0 is a special value meaning no
> limit" :-) due to arithmetic underflow.

*g*

Well, on the one hand: You can not really do anything "sensible"
(except for an exit(), maybe) with a len of 0, assuming that len
is supposed to specify the amount of memory available at to,
and that the function is supposed to place a C string at to:
C strings have a minimum memory requirement of 1 byte. If some
code accidentally passes a len of 0, I would expect it to be
rather unlikely that it subsequently would handle correctly the
special case that there is no C string at to, and thus it would
sooner or later break in some rather nasty way anyhow.

On the other hand: My rather subjective impression is that it's
kindof a basic assumption in the udev code that memory objects or
strings won't have a size of 0. So, there at least were a few places
that looked somewhat suspicious to me, but I couldn't really tell
whether the assumptions made by the code would be met. Maybe somebody
with a deeper understanding of the code wants to have a look at that?
The same analogously applies for sizes close to the maximum of the
respective parameter types ...

Oh, and while we are at it: The udev code seems to be a bit undecided
as to whether memory allocation on the heap can fail.

> Oh - and reading code this closely usually is boring.  Especially when
> you're scanning, and don't necessarily have much idea of the bigger
> picture.  Auditting edge-case stability is even less exciting than
> auditting security.  So thanks for lending a fresh set of eyeballs for
> a while!

That part really was directed more at the people who thought that
flaming and referring me to google was the way to handle bug reports,
but thanks ;-)

Florian

Re: [PATCH] fix buffer overflow in udev_util_replace_whitespace()

[Florian Zumbiehl]

Hi,

> > Indeed, excuse my brainfart.  Looking at the rest of the function I
> > agree it needs fixing,  Unless Andrey can correct us.
> > 
> 
> This change breaks udev_util_replace_whitespace().
> 
> #include <stdio.h>
> #include <stddef.h>
> 
> main()
> {
>         int len = 3, i, j;
>         char *str = "a b";
>         char to[10];
> 
>         len = strnlen(str, len - 1);
>         while (len && isspace(str[len-1]))
>                 len--;
> 
>         j = i = 0;
>         while (i < len) {
>                 /* substitute multiple whitespace with a single '_' */
>                 if (isspace(str[i])) {
>                         while (isspace(str[i]))
>                                 i++;
>                         to[j++] = '_';
>                         }
>                 to[j++] = str[i++];
>         }
>         to[j] = '\0';
> 
>         printf("'%s'\n", to);
> }
> 
> {pts/1}% gcc -O0 foo.c
> {pts/1}% ./a.out
> 'a'
> 
> instead of expected a_b

well, I obviously don't know what the expected behaviour is, unless
the expexted behaviour is what the code says, which would imply
potentially undefined behaviour and as such seemed rather unlikely to
me to be the expected behaviour.

So, if you expect me to say anything with regard to the expected
behaviour, other than that it probably ought to not be undefined,
could you provide a description of what the expected behaviour is?

> What is exact problem with original code? 

Assuming that len is the amount of memory available at to, it could
cause undefined behaviour by the means of a buffer overflow with
appropriate input (the code does not necessarily limit itself to writing
to the first len bytes at to, even for cases with len!=0).

Florian