{"thread":{"id":"18872","subject":"[PATCH v2] Fix buffer overflow in config parser","startedAt":"2009-04-14T21:28:35Z","lastAt":"2009-04-17T13:16:54Z","messageCount":10,"participants":["Thomas Jarosch","Johannes Schindelin","Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"111325","messageId":"49E50003.2040907@intra2net.com","threadId":"18872","inReplyTo":null,"subject":"[PATCH v2] Fix buffer overflow in config parser","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-04-14T21:28:35Z","receivedAt":"2009-04-14T21:28:35Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"When interpreting a config value, the config parser reads in 1+ space\ncharacter(s) and puts -one- space character in the buffer as soon as\nthe first non-space character is encountered (if not inside quotes).\n\nUnfortunately the buffer size check lacks the extra space character\nwhich gets inserted at the next non-space character, resulting in\na crash with a specially crafted config entry.\n\nSigned-off-by: Thomas Jarosch \n---\n config.c | 2 +-\n t/t1303-wacky-config.sh | 9 ++++++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b76fe4c..2d70398 100644\n--- a/config.c\n+++ b/config.c\n@@ -51,7 +51,7 @@ static char *parse_value(void)\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (len >= sizeof(value))\n+\t\tif (len >= sizeof(value) - 1)\n \t\t\treturn NULL;\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 1983076..a7d8d25 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -10,7 +10,7 @@ setup() {\n \n check() {\n \techo \"$2\" >expected\n-\tgit config --get \"$1\" >actual\n+\tgit config --get \"$1\" >actual 2>&1\n \ttest_cmp actual expected\n }\n \n@@ -40,4 +40,11 @@ test_expect_success 'make sure git config escapes section names properly' '\n \tcheck \"$SECTION\" bar\n '\n \n+LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n+test_expect_success 'do not crash on special long config line' '\n+\tsetup &&\n+\tgit config section.key \"$LONG_VALUE\" &&\n+\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+'\n+\n test_done\n-- \n1.6.1.3\n"},{"id":"111326","messageId":"alpine.DEB.1.00.0904142340350.10279@pacific.mpi-cbg.de","threadId":"18872","inReplyTo":"49E50003.2040907@intra2net.com","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-14T21:41:14Z","receivedAt":"2009-04-14T21:41:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Apr 2009, Thomas Jarosch wrote:\n\n> t/t1303-wacky-config.sh | 9 ++++++++-\n\nI like the name!\n\n> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n\nBut should it not be guarded against NO_PERL?\n\nCiao,\nDscho\n"},{"id":"111328","messageId":"49E50480.5060005@intra2net.com","threadId":"18872","inReplyTo":"alpine.DEB.1.00.0904142340350.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-04-14T21:47:44Z","receivedAt":"2009-04-14T21:47:44Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"Johannes Schindelin wrote:\n>> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n> \n> But should it not be guarded against NO_PERL?\n\nHmm, lots of other tests like \"t4200-rerere.sh\" use perl\nand it don't see any special guard around the perl usage.\n\nIf it's needed, just add it while applying :-)\n\nThomas\n"},{"id":"111331","messageId":"7v3aca3lpl.fsf@gitster.siamese.dyndns.org","threadId":"18872","inReplyTo":"alpine.DEB.1.00.0904142340350.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-14T22:38:14Z","receivedAt":"2009-04-14T22:38:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Tue, 14 Apr 2009, Thomas Jarosch wrote:\n>\n>> t/t1303-wacky-config.sh | 9 ++++++++-\n>\n> I like the name!\n>\n>> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n>\n> But should it not be guarded against NO_PERL?\n\nThe right question to ask is a rhetorical \"do we need perl to do this?\"\n"},{"id":"111339","messageId":"49E5888D.2090607@viscovery.net","threadId":"18872","inReplyTo":"7v3aca3lpl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-04-15T07:11:09Z","receivedAt":"2009-04-15T07:11:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Schindelin writes:\n> \n>> Hi,\n>>\n>> On Tue, 14 Apr 2009, Thomas Jarosch wrote:\n>>\n>>> t/t1303-wacky-config.sh | 9 ++++++++-\n>> I like the name!\n>>\n>>> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n>> But should it not be guarded against NO_PERL?\n> \n> The right question to ask is a rhetorical \"do we need perl to do this?\"\n\nLONG_VALUE=$(printf \"x%0.1021dx a\", 7)\n\n-- Hannes\n"},{"id":"111340","messageId":"49E58F38.5060103@viscovery.net","threadId":"18872","inReplyTo":"49E5888D.2090607@viscovery.net","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-04-15T07:39:36Z","receivedAt":"2009-04-15T07:39:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n> Junio C Hamano schrieb:\n>> Johannes Schindelin writes:\n>>\n>>> Hi,\n>>>\n>>> On Tue, 14 Apr 2009, Thomas Jarosch wrote:\n>>>\n>>>> t/t1303-wacky-config.sh | 9 ++++++++-\n>>> I like the name!\n>>>\n>>>> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n>>> But should it not be guarded against NO_PERL?\n>> The right question to ask is a rhetorical \"do we need perl to do this?\"\n> \n> LONG_VALUE=$(printf \"x%0.1021dx a\", 7)\n\nOops! Make this\n\nLONG_VALUE=$(printf \"x%01021dx a\" 7)\n\n-- Hannes\n"},{"id":"111341","messageId":"20090415075035.GA23332@coredump.intra.peff.net","threadId":"18872","inReplyTo":"49E50480.5060005@intra2net.com","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-15T07:50:35Z","receivedAt":"2009-04-15T07:50:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2009 at 11:47:44PM +0200, Thomas Jarosch wrote:\n\n> Johannes Schindelin wrote:\n> >> +LONG_VALUE=`perl -e 'print \"x\" x 1023,\" a\"'`\n> > \n> > But should it not be guarded against NO_PERL?\n> \n> Hmm, lots of other tests like \"t4200-rerere.sh\" use perl\n> and it don't see any special guard around the perl usage.\n\nRight. There are really two types of perl usage in the tests:\n\n 1. Testing git programs which use perl.\n\n 2. Tests which happen to require perl as part of the testing.\n\nRight now, only instances of (1) are marked with NO_PERL. This means\nthat you can build with NO_PERL, test and install the result, and then\nuninstall perl (or make a binary package for perl-less people), and have\nit work fine. But you can't currently run the full test suite without\nany perl.\n\nYour usage falls into (2), none of which are marked (and if they are to\nbe marked, then all of them should be, since there is otherwise no\npoint).\n\n-Peff\n"},{"id":"111342","messageId":"20090415075352.GB23332@coredump.intra.peff.net","threadId":"18872","inReplyTo":"20090415075035.GA23332@coredump.intra.peff.net","subject":"Re: [PATCH v2] Fix buffer overflow in config parser","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-15T07:53:52Z","receivedAt":"2009-04-15T07:53:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2009 at 03:50:35AM -0400, Jeff King wrote:\n\n> 2. Tests which happen to require perl as part of the testing.\n> [...]\n> Your usage falls into (2), none of which are marked (and if they are to\n> be marked, then all of them should be, since there is otherwise no\n> point).\n\nI meant to add: it is still nice to avoid the use of perl in the test\nscripts if we can (and I think we can in this instance, as other\nresponses have noted). If a patch were made to skip tests which use\nperl, then the fewer tests we have to skip, the better.\n\n-Peff\n"},{"id":"111483","messageId":"200904171405.48269.thomas.jarosch@intra2net.com","threadId":"18872","inReplyTo":"49E58F38.5060103@viscovery.net","subject":"[PATCH v3] Fix buffer overflow in config parser","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-04-17T12:05:11Z","receivedAt":"2009-04-17T12:05:11Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"When interpreting a config value, the config parser reads in 1+ space\ncharacter(s) and puts -one- space character in the buffer as soon as\nthe first non-space character is encountered (if not inside quotes).\n\nUnfortunately the buffer size check lacks the extra space character\nwhich gets inserted at the next non-space character, resulting in\na crash with a specially crafted config entry.\n\nThe unit test now uses Java to compile a platform independent\n.NET framework to output the test string in C# :o) Read:\nThanks to Johannes Sixt for the correct printf call\nwhich replaces the perl invocation.\n\nSigned-off-by: Thomas Jarosch \n---\n config.c | 2 +-\n t/t1303-wacky-config.sh | 9 ++++++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b76fe4c..2d70398 100644\n--- a/config.c\n+++ b/config.c\n@@ -51,7 +51,7 @@ static char *parse_value(void)\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (len >= sizeof(value))\n+\t\tif (len >= sizeof(value) - 1)\n \t\t\treturn NULL;\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 1983076..a7d8d25 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -10,7 +10,7 @@ setup() {\n \n check() {\n \techo \"$2\" >expected\n-\tgit config --get \"$1\" >actual\n+\tgit config --get \"$1\" >actual 2>&1\n \ttest_cmp actual expected\n }\n \n@@ -40,4 +40,11 @@ test_expect_success 'make sure git config escapes section names properly' '\n \tcheck \"$SECTION\" bar\n '\n \n+LONG_VALUE=$(printf \"x%01021dx a\" 7)\n+test_expect_success 'do not crash on special long config line' '\n+\tsetup &&\n+\tgit config section.key \"$LONG_VALUE\" &&\n+\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+'\n+\n test_done\n-- \n1.6.1.3\n"},{"id":"111487","messageId":"alpine.DEB.1.00.0904171516400.6675@intel-tinevez-2-302","threadId":"18872","inReplyTo":"200904171405.48269.thomas.jarosch@intra2net.com","subject":"Re: [PATCH v3] Fix buffer overflow in config parser","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-17T13:16:54Z","receivedAt":"2009-04-17T13:16:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 17 Apr 2009, Thomas Jarosch wrote:\n\n> When interpreting a config value, the config parser reads in 1+ space\n> character(s) and puts -one- space character in the buffer as soon as\n> the first non-space character is encountered (if not inside quotes).\n> \n> Unfortunately the buffer size check lacks the extra space character\n> which gets inserted at the next non-space character, resulting in\n> a crash with a specially crafted config entry.\n> \n> The unit test now uses Java to compile a platform independent\n> .NET framework to output the test string in C# :o) Read:\n> Thanks to Johannes Sixt for the correct printf call\n> which replaces the perl invocation.\n\nLOL!\n\nThanks,\nDscho\n"}]}