threads / patch / 18796

patchFix buffer overflow in config parser

Subject: [PATCH] Fix buffer overflow in config parser

## tl;dr

5 messages between Apr 8, 2009 and Apr 10, 2009. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Thomas Jarosch· Apr 8, 2009, 22:13 UTC · lore

Hello together,

attached is a small patch to fix a buffer overflow in config.c. Patch is against git master's HEAD.

I didn't send this one inline as I wanted to preserve the 1024+ byte long line.

Best regards, Thomas Jarosch

Fix buffer overflow in config parser.

Segfaulting config looks like this:
---------------------------------------
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
    foobar = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaxxxbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa123 4
---------------------------------------

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

Show changes to config.c +1 −1

diff --git a/config.c b/config.c
index b76fe4c..a9c67e8 100644
--- a/config.c
+++ b/config.c
@@ -72,7 +72,7 @@ static char *parse_value(void)
 			}
 		}
 		if (space) {
-			if (len)
+			if (len && len < sizeof(value)-1)
 				value[len++] = ' ';
 			space = 0;
 		}

Markus Heidelberg· Apr 8, 2009, 22:58 UTC · re: Thomas Jarosch · lore

Re: [PATCH] Fix buffer overflow in config parser

Thomas Jarosch, 09.04.2009:

Show 7 quoted lines

> Hello together,
> 
> attached is a small patch to fix a buffer overflow in config.c.
> Patch is against git master's HEAD.
> 
> I didn't send this one inline as I wanted to
> preserve the 1024+ byte long line.

You could send the patch inline and attach the example config.

Show 13 quoted lines

> diff --git a/config.c b/config.c
> index b76fe4c..a9c67e8 100644
> --- a/config.c
> +++ b/config.c
> @@ -72,7 +72,7 @@ static char *parse_value(void)
>                         }
>                 }
>                 if (space) {
> -                       if (len)
> +                       if (len && len < sizeof(value)-1)
>                                 value[len++] = ' ';
>                         space = 0;
>                 }

At the beginning of the for loop, there is already an overflow guard.

		if (len >= sizeof(value))
			return NULL;

It would probably be better to fix it at this place.

		if (len >= sizeof(value)-1)
			return NULL;

Markus

Markus Heidelberg· Apr 8, 2009, 23:15 UTC · re: Markus Heidelberg · lore

Re: [PATCH] Fix buffer overflow in config parser

Markus Heidelberg, 09.04.2009:

Show 23 quoted lines

> Thomas Jarosch, 09.04.2009:
> > Hello together,
> > 
> > attached is a small patch to fix a buffer overflow in config.c.
> > Patch is against git master's HEAD.
> > 
> > I didn't send this one inline as I wanted to
> > preserve the 1024+ byte long line.
> 
> You could send the patch inline and attach the example config.
> 
> > diff --git a/config.c b/config.c
> > index b76fe4c..a9c67e8 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -72,7 +72,7 @@ static char *parse_value(void)
> >                         }
> >                 }
> >                 if (space) {
> > -                       if (len)
> > +                       if (len && len < sizeof(value)-1)
> >                                 value[len++] = ' ';
> >                         space = 0;

Eh, or maybe better add a "continue;" here, so that only one char per loop is read.

> >                 }

Thomas Jarosch· Apr 9, 2009, 07:59 UTC · re: Markus Heidelberg · lore

Re: [PATCH] Fix buffer overflow in config parser

On Thursday, 9. April 2009 01:15:17 Markus Heidelberg wrote:

Show 15 quoted lines

> > > diff --git a/config.c b/config.c
> > > index b76fe4c..a9c67e8 100644
> > > --- a/config.c
> > > +++ b/config.c
> > > @@ -72,7 +72,7 @@ static char *parse_value(void)
> > >                         }
> > >                 }
> > >                 if (space) {
> > > -                       if (len)
> > > +                       if (len && len < sizeof(value)-1)
> > >                                 value[len++] = ' ';
> > >                         space = 0;
>
> Eh, or maybe better add a "continue;" here, so that only one char per
> loop is read.

Thanks for the review.

If I understand the intention of the complete code correctly, the idea was to read in 1+ spaces and put -one- space in the buffer as soon as the first non-space character is encountered (if not inside quotes).

Adding a "continue" statement would eat up the first non-space character.

I guess it's ok to modify the first size check or keep to problem local and check the size before putting the space in the buffer. Guess that's up to the maintainer which method he prefers.

Cheers, Thomas

Markus Heidelberg· Apr 10, 2009, 16:10 UTC · re: Thomas Jarosch · lore

Re: [PATCH] Fix buffer overflow in config parser

Thomas Jarosch, 09.04.2009:

Show 24 quoted lines

> On Thursday, 9. April 2009 01:15:17 Markus Heidelberg wrote:
> > > > diff --git a/config.c b/config.c
> > > > index b76fe4c..a9c67e8 100644
> > > > --- a/config.c
> > > > +++ b/config.c
> > > > @@ -72,7 +72,7 @@ static char *parse_value(void)
> > > >                         }
> > > >                 }
> > > >                 if (space) {
> > > > -                       if (len)
> > > > +                       if (len && len < sizeof(value)-1)
> > > >                                 value[len++] = ' ';
> > > >                         space = 0;
> >
> > Eh, or maybe better add a "continue;" here, so that only one char per
> > loop is read.
> 
> Thanks for the review.
> 
> If I understand the intention of the complete code correctly, the idea was
> to read in 1+ spaces and put -one- space in the buffer as soon as the first
> non-space character is encountered (if not inside quotes).
> 
> Adding a "continue" statement would eat up the first non-space character.

Yes, you are right, I judged to quickly.

> I guess it's ok to modify the first size check or keep to problem local and 
> check the size before putting the space in the buffer.

Keeping the problem local would mean to add this check to the end of the "for" loop before "value[len++] = c;" where the overflow actually happens. OTOH this overflow only occurs within a whitespace area, so from this POV it is local at this place.

I think the cleanest solution would be to decrease the existing check at the top by 1, even if then the maximum length will be decreased. But there is no hint in the docs about it anyway.

> Guess that's up to
> the maintainer which method he prefers.

Take what you think is the best solution and convince him :)

Markus