threads / patch / 18872

v2Fix buffer overflow in config parser

Subject: [PATCH v2] Fix buffer overflow in config parser

## tl;dr

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

replies: 9people: 5as markdown or json

Thomas Jarosch· Apr 14, 2009, 21:28 UTC · lore

When interpreting a config value, the config parser reads in 1+ space character(s) and puts -one- space character in the buffer as soon as the first non-space character is encountered (if not inside quotes).

Unfortunately the buffer size check lacks the extra space character which gets inserted at the next non-space character, resulting in a crash with a specially crafted config entry.

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
---
 config.c                |    2 +-
 t/t1303-wacky-config.sh |    9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)
Show changes to 2 files +9 −2

config.c, t/t1303-wacky-config.sh

diff --git a/config.c b/config.c
index b76fe4c..2d70398 100644
--- a/config.c
+++ b/config.c
@@ -51,7 +51,7 @@ static char *parse_value(void)
 
 	for (;;) {
 		int c = get_next_char();
-		if (len >= sizeof(value))
+		if (len >= sizeof(value) - 1)
 			return NULL;
 		if (c == '\n') {
 			if (quote)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 1983076..a7d8d25 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -10,7 +10,7 @@ setup() {
 
 check() {
 	echo "$2" >expected
-	git config --get "$1" >actual
+	git config --get "$1" >actual 2>&1
 	test_cmp actual expected
 }
 
@@ -40,4 +40,11 @@ test_expect_success 'make sure git config escapes section names properly' '
 	check "$SECTION" bar
 '
 
+LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
+test_expect_success 'do not crash on special long config line' '
+	setup &&
+	git config section.key "$LONG_VALUE" &&
+	check section.key "fatal: bad config file line 2 in .git/config"
+'
+
 test_done

-- 
1.6.1.3
Johannes Schindelin· Apr 14, 2009, 21:41 UTC · re: Thomas Jarosch · lore

Re: [PATCH v2] Fix buffer overflow in config parser

Hi,
On Tue, 14 Apr 2009, Thomas Jarosch wrote:
>  t/t1303-wacky-config.sh |    9 ++++++++-
I like the name!
> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
But should it not be guarded against NO_PERL?

Ciao, Dscho

Thomas Jarosch· Apr 14, 2009, 21:47 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] Fix buffer overflow in config parser

Johannes Schindelin wrote:
>> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
> 
> But should it not be guarded against NO_PERL?

Hmm, lots of other tests like "t4200-rerere.sh" use perl and it don't see any special guard around the perl usage.

If it's needed, just add it while applying :-)
Thomas
Jeff King· Apr 15, 2009, 07:50 UTC · re: Thomas Jarosch · lore

Re: [PATCH v2] Fix buffer overflow in config parser

On Tue, Apr 14, 2009 at 11:47:44PM +0200, Thomas Jarosch wrote:
Show 7 quoted lines
> Johannes Schindelin wrote:
> >> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
> > 
> > But should it not be guarded against NO_PERL?
> 
> Hmm, lots of other tests like "t4200-rerere.sh" use perl
> and it don't see any special guard around the perl usage.
Right.  There are really two types of perl usage in the tests:
  1. Testing git programs which use perl.
  2. Tests which happen to require perl as part of the testing.

Right now, only instances of (1) are marked with NO_PERL. This means that you can build with NO_PERL, test and install the result, and then uninstall perl (or make a binary package for perl-less people), and have it work fine. But you can't currently run the full test suite without any perl.

Your usage falls into (2), none of which are marked (and if they are to be marked, then all of them should be, since there is otherwise no point).

-Peff
Jeff King· Apr 15, 2009, 07:53 UTC · re: Jeff King · lore

Re: [PATCH v2] Fix buffer overflow in config parser

On Wed, Apr 15, 2009 at 03:50:35AM -0400, Jeff King wrote:
Show 5 quoted lines
>   2. Tests which happen to require perl as part of the testing.
> [...]
> Your usage falls into (2), none of which are marked (and if they are to
> be marked, then all of them should be, since there is otherwise no
> point).

I meant to add: it is still nice to avoid the use of perl in the test scripts if we can (and I think we can in this instance, as other responses have noted). If a patch were made to skip tests which use perl, then the fewer tests we have to skip, the better.

-Peff
Junio C Hamano· Apr 14, 2009, 22:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] Fix buffer overflow in config parser

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 11 quoted lines
> Hi,
>
> On Tue, 14 Apr 2009, Thomas Jarosch wrote:
>
>>  t/t1303-wacky-config.sh |    9 ++++++++-
>
> I like the name!
>
>> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
>
> But should it not be guarded against NO_PERL?
The right question to ask is a rhetorical "do we need perl to do this?"
Johannes Sixt· Apr 15, 2009, 07:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Fix buffer overflow in config parser

Junio C Hamano schrieb:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> Hi,
>>
>> On Tue, 14 Apr 2009, Thomas Jarosch wrote:
>>
>>>  t/t1303-wacky-config.sh |    9 ++++++++-
>> I like the name!
>>
>>> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
>> But should it not be guarded against NO_PERL?
> 
> The right question to ask is a rhetorical "do we need perl to do this?"
LONG_VALUE=$(printf "x%0.1021dx a", 7)
-- Hannes
Johannes Sixt· Apr 15, 2009, 07:39 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] Fix buffer overflow in config parser

Johannes Sixt schrieb:
Show 15 quoted lines
> Junio C Hamano schrieb:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> Hi,
>>>
>>> On Tue, 14 Apr 2009, Thomas Jarosch wrote:
>>>
>>>>  t/t1303-wacky-config.sh |    9 ++++++++-
>>> I like the name!
>>>
>>>> +LONG_VALUE=`perl -e 'print "x" x 1023," a"'`
>>> But should it not be guarded against NO_PERL?
>> The right question to ask is a rhetorical "do we need perl to do this?"
> 
> LONG_VALUE=$(printf "x%0.1021dx a", 7)
Oops! Make this
LONG_VALUE=$(printf "x%01021dx a" 7)
-- Hannes
Thomas Jarosch· Apr 17, 2009, 12:05 UTC · re: Johannes Sixt · lore

[PATCH v3] Fix buffer overflow in config parser

When interpreting a config value, the config parser reads in 1+ space character(s) and puts -one- space character in the buffer as soon as the first non-space character is encountered (if not inside quotes).

Unfortunately the buffer size check lacks the extra space character which gets inserted at the next non-space character, resulting in a crash with a specially crafted config entry.

The unit test now uses Java to compile a platform independent .NET framework to output the test string in C# :o) Read: Thanks to Johannes Sixt for the correct printf call which replaces the perl invocation.

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
---
 config.c                |    2 +-
 t/t1303-wacky-config.sh |    9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)
Show changes to 2 files +9 −2

config.c, t/t1303-wacky-config.sh

diff --git a/config.c b/config.c
index b76fe4c..2d70398 100644
--- a/config.c
+++ b/config.c
@@ -51,7 +51,7 @@ static char *parse_value(void)
 
 	for (;;) {
 		int c = get_next_char();
-		if (len >= sizeof(value))
+		if (len >= sizeof(value) - 1)
 			return NULL;
 		if (c == '\n') {
 			if (quote)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 1983076..a7d8d25 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -10,7 +10,7 @@ setup() {
 
 check() {
 	echo "$2" >expected
-	git config --get "$1" >actual
+	git config --get "$1" >actual 2>&1
 	test_cmp actual expected
 }
 
@@ -40,4 +40,11 @@ test_expect_success 'make sure git config escapes section names properly' '
 	check "$SECTION" bar
 '
 
+LONG_VALUE=$(printf "x%01021dx a" 7)
+test_expect_success 'do not crash on special long config line' '
+	setup &&
+	git config section.key "$LONG_VALUE" &&
+	check section.key "fatal: bad config file line 2 in .git/config"
+'
+
 test_done

-- 
1.6.1.3
Johannes Schindelin· Apr 17, 2009, 13:16 UTC · re: Thomas Jarosch · lore

Re: [PATCH v3] Fix buffer overflow in config parser

Hi,
On Fri, 17 Apr 2009, Thomas Jarosch wrote:
Show 12 quoted lines
> When interpreting a config value, the config parser reads in 1+ space
> character(s) and puts -one- space character in the buffer as soon as
> the first non-space character is encountered (if not inside quotes).
> 
> Unfortunately the buffer size check lacks the extra space character
> which gets inserted at the next non-space character, resulting in
> a crash with a specially crafted config entry.
> 
> The unit test now uses Java to compile a platform independent
> .NET framework to output the test string in C# :o) Read:
> Thanks to Johannes Sixt for the correct printf call
> which replaces the perl invocation.
LOL!

Thanks, Dscho

← back to recent threads