{"thread":{"id":"18796","subject":"[PATCH] Fix buffer overflow in config parser","startedAt":"2009-04-08T22:13:52Z","lastAt":"2009-04-10T16:10:54Z","messageCount":5,"participants":["Thomas Jarosch","Markus Heidelberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110877","messageId":"49DD21A0.3040505@intra2net.com","threadId":"18796","inReplyTo":null,"subject":"[PATCH] Fix buffer overflow in config parser","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-04-08T22:13:52Z","receivedAt":"2009-04-08T22:13:52Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"Hello together,\n\nattached is a small patch to fix a buffer overflow in config.c.\nPatch is against git master's HEAD.\n\nI didn't send this one inline as I wanted to\npreserve the 1024+ byte long line.\n\nBest regards,\nThomas Jarosch\n\n\n\nFix buffer overflow in config parser.\n\nSegfaulting config looks like this:\n---------------------------------------\n[core]\n repositoryformatversion = 0\n filemode = true\n bare = false\n logallrefupdates = true\n foobar = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaxxxbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa123 4\n---------------------------------------\n\nSigned-off-by: Thomas Jarosch \n\ndiff --git a/config.c b/config.c\nindex b76fe4c..a9c67e8 100644\n--- a/config.c\n+++ b/config.c\n@@ -72,7 +72,7 @@ static char *parse_value(void)\n \t\t\t}\n \t\t}\n \t\tif (space) {\n-\t\t\tif (len)\n+\t\t\tif (len && len < sizeof(value)-1)\n \t\t\t\tvalue[len++] = ' ';\n \t\t\tspace = 0;\n \t\t}\n\n"},{"id":"110878","messageId":"200904090058.42751.markus.heidelberg@web.de","threadId":"18796","inReplyTo":"49DD21A0.3040505@intra2net.com","subject":"Re: [PATCH] Fix buffer overflow in config parser","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-08T22:58:42Z","receivedAt":"2009-04-08T22:58:42Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Thomas Jarosch, 09.04.2009:\n> Hello together,\n> \n> attached is a small patch to fix a buffer overflow in config.c.\n> Patch is against git master's HEAD.\n> \n> I didn't send this one inline as I wanted to\n> preserve the 1024+ byte long line.\n\nYou could send the patch inline and attach the example config.\n\n> diff --git a/config.c b/config.c\n> index b76fe4c..a9c67e8 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -72,7 +72,7 @@ static char *parse_value(void)\n> }\n> }\n> if (space) {\n> - if (len)\n> + if (len && len < sizeof(value)-1)\n> value[len++] = ' ';\n> space = 0;\n> }\n\nAt the beginning of the for loop, there is already an overflow guard.\n\n\t\tif (len >= sizeof(value))\n\t\t\treturn NULL;\n\nIt would probably be better to fix it at this place.\n\n\t\tif (len >= sizeof(value)-1)\n\t\t\treturn NULL;\n\nMarkus\n"},{"id":"110880","messageId":"200904090115.17609.markus.heidelberg@web.de","threadId":"18796","inReplyTo":"200904090058.42751.markus.heidelberg@web.de","subject":"Re: [PATCH] Fix buffer overflow in config parser","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-08T23:15:17Z","receivedAt":"2009-04-08T23:15:17Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Markus Heidelberg, 09.04.2009:\n> Thomas Jarosch, 09.04.2009:\n> > Hello together,\n> > \n> > attached is a small patch to fix a buffer overflow in config.c.\n> > Patch is against git master's HEAD.\n> > \n> > I didn't send this one inline as I wanted to\n> > preserve the 1024+ byte long line.\n> \n> You could send the patch inline and attach the example config.\n> \n> > diff --git a/config.c b/config.c\n> > index b76fe4c..a9c67e8 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -72,7 +72,7 @@ static char *parse_value(void)\n> > }\n> > }\n> > if (space) {\n> > - if (len)\n> > + if (len && len < sizeof(value)-1)\n> > value[len++] = ' ';\n> > space = 0;\n\nEh, or maybe better add a \"continue;\" here, so that only one char per\nloop is read.\n\n> > }\n"},{"id":"110894","messageId":"200904090959.28646.thomas.jarosch@intra2net.com","threadId":"18796","inReplyTo":"200904090115.17609.markus.heidelberg@web.de","subject":"Re: [PATCH] Fix buffer overflow in config parser","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-04-09T07:59:28Z","receivedAt":"2009-04-09T07:59:28Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"On Thursday, 9. April 2009 01:15:17 Markus Heidelberg wrote:\n> > > diff --git a/config.c b/config.c\n> > > index b76fe4c..a9c67e8 100644\n> > > --- a/config.c\n> > > +++ b/config.c\n> > > @@ -72,7 +72,7 @@ static char *parse_value(void)\n> > > }\n> > > }\n> > > if (space) {\n> > > - if (len)\n> > > + if (len && len < sizeof(value)-1)\n> > > value[len++] = ' ';\n> > > space = 0;\n>\n> Eh, or maybe better add a \"continue;\" here, so that only one char per\n> loop is read.\n\nThanks for the review.\n\nIf I understand the intention of the complete code correctly, the idea was\nto read in 1+ spaces and put -one- space in the buffer as soon as the first\nnon-space character is encountered (if not inside quotes).\n\nAdding a \"continue\" statement would eat up the first non-space character.\n\nI guess it's ok to modify the first size check or keep to problem local and \ncheck the size before putting the space in the buffer. Guess that's up to\nthe maintainer which method he prefers.\n\nCheers,\nThomas\n"},{"id":"111012","messageId":"200904101810.54774.markus.heidelberg@web.de","threadId":"18796","inReplyTo":"200904090959.28646.thomas.jarosch@intra2net.com","subject":"Re: [PATCH] Fix buffer overflow in config parser","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-10T16:10:54Z","receivedAt":"2009-04-10T16:10:54Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Thomas Jarosch, 09.04.2009:\n> On Thursday, 9. April 2009 01:15:17 Markus Heidelberg wrote:\n> > > > diff --git a/config.c b/config.c\n> > > > index b76fe4c..a9c67e8 100644\n> > > > --- a/config.c\n> > > > +++ b/config.c\n> > > > @@ -72,7 +72,7 @@ static char *parse_value(void)\n> > > > }\n> > > > }\n> > > > if (space) {\n> > > > - if (len)\n> > > > + if (len && len < sizeof(value)-1)\n> > > > value[len++] = ' ';\n> > > > space = 0;\n> >\n> > Eh, or maybe better add a \"continue;\" here, so that only one char per\n> > loop is read.\n> \n> Thanks for the review.\n> \n> If I understand the intention of the complete code correctly, the idea was\n> to read in 1+ spaces and put -one- space in the buffer as soon as the first\n> non-space character is encountered (if not inside quotes).\n> \n> Adding a \"continue\" statement would eat up the first non-space character.\n\nYes, you are right, I judged to quickly.\n\n> I guess it's ok to modify the first size check or keep to problem local and \n> check the size before putting the space in the buffer.\n\nKeeping the problem local would mean to add this check to the end of the\n\"for\" loop before \"value[len++] = c;\" where the overflow actually\nhappens.\nOTOH this overflow only occurs within a whitespace area, so from this\nPOV it is local at this place.\n\nI think the cleanest solution would be to decrease the existing check at\nthe top by 1, even if then the maximum length will be decreased. But\nthere is no hint in the docs about it anyway.\n\n> Guess that's up to\n> the maintainer which method he prefers.\n\nTake what you think is the best solution and convince him :)\n\nMarkus\n"}]}