[PATCH] fix buffer overflow in util_run_program()

[PATCH] fix buffer overflow in util_run_program()

[Florian Zumbiehl]

Hi,

...

Reading your high-quality code starts getting a bit boring, so I guess that
I won't waste any more of your valuable time for now, except for finishing
the threads I started.

Florian

diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index 64203a8..c309945 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const char *command, char **envp,
 	pid_t pid;
 	char arg[UTIL_PATH_SIZE];
 	char program[UTIL_PATH_SIZE];
-	char *argv[(sizeof(arg) / 2) + 1];
+	char *argv[sizeof(arg) + 1];
 	int devnull;
 	int i;
 	int err = 0;

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

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Friday 04 of September 2009 23:54:14 Florian Zumbiehl wrote:
> Hi,
> 
> ...
> 
> Reading your high-quality code starts getting a bit boring, so I
>  guess that I won't waste any more of your valuable time for now,
>  except for finishing the threads I started.
> 
> Florian
> 
> diff --git a/libudev/libudev-util-private.c
>  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> --- a/libudev/libudev-util-private.c
> +++ b/libudev/libudev-util-private.c
> @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
>  char *command, char **envp, pid_t pid;
>  	char arg[UTIL_PATH_SIZE];
>  	char program[UTIL_PATH_SIZE];
> -	char *argv[(sizeof(arg) / 2) + 1];
> +	char *argv[sizeof(arg) + 1];

Could you give example when this overflows?

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

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

[Florian Zumbiehl]

Hi,

> > diff --git a/libudev/libudev-util-private.c
> >  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > --- a/libudev/libudev-util-private.c
> > +++ b/libudev/libudev-util-private.c
> > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> >  char *command, char **envp, pid_t pid;
> >  	char arg[UTIL_PATH_SIZE];
> >  	char program[UTIL_PATH_SIZE];
> > -	char *argv[(sizeof(arg) / 2) + 1];
> > +	char *argv[sizeof(arg) + 1];
> 
> Could you give example when this overflows?

UTIL_PATH_SIZE-1 spaces.

Florian

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

[Andrey Borzenkov]

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

On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
> Hi,
> 
> > > diff --git a/libudev/libudev-util-private.c
> > >  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > > --- a/libudev/libudev-util-private.c
> > > +++ b/libudev/libudev-util-private.c
> > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> > >  char *command, char **envp, pid_t pid;
> > >  	char arg[UTIL_PATH_SIZE];
> > >  	char program[UTIL_PATH_SIZE];
> > > -	char *argv[(sizeof(arg) / 2) + 1];
> > > +	char *argv[sizeof(arg) + 1];
> >
> > Could you give example when this overflows?
> 
> UTIL_PATH_SIZE-1 spaces.
> 

Please try to understand what code you are fixing does.

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

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

[Florian Zumbiehl]

Hi,

> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
> > Hi,
> > 
> > > > diff --git a/libudev/libudev-util-private.c
> > > >  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > > > --- a/libudev/libudev-util-private.c
> > > > +++ b/libudev/libudev-util-private.c
> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> > > >  char *command, char **envp, pid_t pid;
> > > >  	char arg[UTIL_PATH_SIZE];
> > > >  	char program[UTIL_PATH_SIZE];
> > > > -	char *argv[(sizeof(arg) / 2) + 1];
> > > > +	char *argv[sizeof(arg) + 1];
> > >
> > > Could you give example when this overflows?
> > 
> > UTIL_PATH_SIZE-1 spaces.
> > 
> 
> Please try to understand what code you are fixing does.

No, of course not.

Now, what am I missing? I obviously do not understand much of how udev
works, but if the code of this function is not somewhat pointless, then
how would there not be a potential buffer overflow?

Florian

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

[Alan Jenkins]

On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
>> > Hi,
>> >
>> > > > diff --git a/libudev/libudev-util-private.c
>> > > >  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
>> > > > --- a/libudev/libudev-util-private.c
>> > > > +++ b/libudev/libudev-util-private.c
>> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
>> > > >  char *command, char **envp, pid_t pid;
>> > > >  	char arg[UTIL_PATH_SIZE];
>> > > >  	char program[UTIL_PATH_SIZE];
>> > > > -	char *argv[(sizeof(arg) / 2) + 1];
>> > > > +	char *argv[sizeof(arg) + 1];
>> > >
>> > > Could you give example when this overflows?
>> >
>> > UTIL_PATH_SIZE-1 spaces.
>> >
>>
>> Please try to understand what code you are fixing does.
>
> No, of course not.
>
> Now, what am I missing? I obviously do not understand much of how udev
> works, but if the code of this function is not somewhat pointless, then
> how would there not be a potential buffer overflow?
>
> Florian

Running "ls  -l" (two spaces) should be equivalent to "ls -l" (one
space).  arg filled with spaces should be more or less equivalent to
arg = "".  If it's not - then that's the real bug.

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

[Florian Zumbiehl]

Hi,

> > Now, what am I missing? I obviously do not understand much of how udev
> > works, but if the code of this function is not somewhat pointless, then
> > how would there not be a potential buffer overflow?
> >
> > Florian
> 
> Running "ls  -l" (two spaces) should be equivalent to "ls -l" (one
> space).  arg filled with spaces should be more or less equivalent to
> arg = "".  If it's not - then that's the real bug.

well, I don't want to get into fixing semantic bugs, as there generally
doesn't seem to be much of a hint as to what the intended semantics are -
except that you wonder how the code's semantics could actually be
intentional. So I would suggest fixing the buffer overflow for now, until
someone feels like taking care of the semantic bug.

Florian

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

[Alan Jenkins]

On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> > Now, what am I missing? I obviously do not understand much of how udev
>> > works, but if the code of this function is not somewhat pointless, then
>> > how would there not be a potential buffer overflow?
>> >
>> > Florian
>>
>> Running "ls  -l" (two spaces) should be equivalent to "ls -l" (one
>> space).  arg filled with spaces should be more or less equivalent to
>> arg = "".  If it's not - then that's the real bug.
>
> well, I don't want to get into fixing semantic bugs, as there generally
> doesn't seem to be much of a hint as to what the intended semantics are -
> except that you wonder how the code's semantics could actually be
> intentional. So I would suggest fixing the buffer overflow for now, until
> someone feels like taking care of the semantic bug.

My point was that I don't see any such semantic bug; I can't see where
the overflow would come from.

As far as I can see, the code uses strsep() which will correctly
interpret a string of spaces as containing no tokens - and return
NULL.

If I'm right, there's a different semantic bug - the use of strsep()
to find a closing quote, which will fail for strings like

' a '' b '

Regards
Alan

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

[Andrey Borzenkov]

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

On Saturday 05 of September 2009 22:41:27 Alan Jenkins wrote:
> On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> > Hi,
> >
> >> > Now, what am I missing? I obviously do not understand much of
> >> > how udev works, but if the code of this function is not somewhat
> >> > pointless, then how would there not be a potential buffer
> >> > overflow?
> >> >
> >> > Florian
> >>
> >> Running "ls  -l" (two spaces) should be equivalent to "ls -l" (one
> >> space).  arg filled with spaces should be more or less equivalent
> >> to arg = "".  If it's not - then that's the real bug.
> >
> > well, I don't want to get into fixing semantic bugs, as there
> > generally doesn't seem to be much of a hint as to what the intended
> > semantics are - except that you wonder how the code's semantics
> > could actually be intentional. So I would suggest fixing the buffer
> > overflow for now, until someone feels like taking care of the
> > semantic bug.
> 
> My point was that I don't see any such semantic bug; I can't see
>  where the overflow would come from.
> 
> As far as I can see, the code uses strsep() which will correctly
> interpret a string of spaces as containing no tokens - and return
> NULL.
> 
> If I'm right, there's a different semantic bug - the use of strsep()
> to find a closing quote, which will fail for strings like
> 
> ' a '' b '
> 

If this is assumed to be two arguments ' a ' and ' b ', this function 
works correctly. What is really not possible, is to quote the quote.

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

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

[Florian Zumbiehl]

Hi,

> > well, I don't want to get into fixing semantic bugs, as there generally
> > doesn't seem to be much of a hint as to what the intended semantics are -
> > except that you wonder how the code's semantics could actually be
> > intentional. So I would suggest fixing the buffer overflow for now, until
> > someone feels like taking care of the semantic bug.
> 
> My point was that I don't see any such semantic bug; I can't see where
> the overflow would come from.
> 
> As far as I can see, the code uses strsep() which will correctly
> interpret a string of spaces as containing no tokens - and return
> NULL.
> 
> If I'm right, there's a different semantic bug - the use of strsep()
> to find a closing quote, which will fail for strings like
> 
> ' a '' b '

| $ cat foo.c
| 
| #include <string.h>
| #include <stdio.h>
| 
| int main(){
| 	char s[]="   ",*p=s;
| 	while(p)printf("|%s|\n",strsep(&p," "));
| 	return 0;
| }
| 
| $ gcc -o foo foo.c
| $ ./foo
| ||
| ||
| ||
| ||
| $ 

actually, I don't really know for sure what the intended semantics of
strsep() are, but assuming that the glibc implementation is not
majorly broken, I would argue that there is a buffer overflow in
that code ;-)

Florian

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

[Kay Sievers]

On Sat, Sep 5, 2009 at 12:50, Alan Jenkins
<sourcejedi.lkml@googlemail.com> wrote:
> On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
>>> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
>>> > > > diff --git a/libudev/libudev-util-private.c
>>> > > >  b/libudev/libudev-util-private.c index 64203a8..c309945 100644
>>> > > > --- a/libudev/libudev-util-private.c
>>> > > > +++ b/libudev/libudev-util-private.c
>>> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
>>> > > >  char *command, char **envp, pid_t pid;
>>> > > >        char arg[UTIL_PATH_SIZE];
>>> > > >        char program[UTIL_PATH_SIZE];
>>> > > > -      char *argv[(sizeof(arg) / 2) + 1];
>>> > > > +      char *argv[sizeof(arg) + 1];
>>> > >
>>> > > Could you give example when this overflows?
>>> >
>>> > UTIL_PATH_SIZE-1 spaces.
>>> >
>>>
>>> Please try to understand what code you are fixing does.
>>
>> No, of course not.
>>
>> Now, what am I missing? I obviously do not understand much of how udev
>> works, but if the code of this function is not somewhat pointless, then
>> how would there not be a potential buffer overflow?
>
> Running "ls  -l" (two spaces) should be equivalent to "ls -l" (one
> space).  arg filled with spaces should be more or less equivalent to
> arg = "".  If it's not - then that's the real bug.

Changed it to skip multiple consecutive spaces.

Thanks,
Kay

[PATCH] fix buffer overflow in util_run_program(), #2

[Florian Zumbiehl]

Hi,

I'm not sure how likely it is for UTIL_PATH_SIZE to have an odd value
(maybe it has right now? :-), but I guess making this universally correct
doesn't hurt ...

Florian

diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index fb64c13..e0670db 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -251,7 +251,7 @@ int util_run_program(struct udev *udev, const char *command, char **envp,
 	pid_t pid;
 	char arg[UTIL_PATH_SIZE];
 	char program[UTIL_PATH_SIZE];
-	char *argv[(sizeof(arg) / 2) + 1];
+	char *argv[((sizeof(arg) + 1) / 2) + 1];
 	int devnull;
 	int i;
 	int err = 0;

Re: [PATCH] fix buffer overflow in util_run_program(), #2

[Kay Sievers]

On Mon, Sep 7, 2009 at 20:18, Florian Zumbiehl<florz@florz.de> wrote:
> I'm not sure how likely it is for UTIL_PATH_SIZE to have an odd value
> (maybe it has right now? :-), but I guess making this universally correct
> doesn't hurt ...

Applied.

Thanks,
Kay