* [PATCH v2] Fix buffer overflow in config parser
@ 2009-04-14 21:28 Thomas Jarosch
2009-04-14 21:41 ` Johannes Schindelin
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Jarosch @ 2009-04-14 21:28 UTC (permalink / raw)
To: gitster; +Cc: git, markus.heidelberg
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-14 21:28 [PATCH v2] Fix buffer overflow in config parser Thomas Jarosch
@ 2009-04-14 21:41 ` Johannes Schindelin
2009-04-14 21:47 ` Thomas Jarosch
2009-04-14 22:38 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Johannes Schindelin @ 2009-04-14 21:41 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: gitster, git, markus.heidelberg
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-14 21:41 ` Johannes Schindelin
@ 2009-04-14 21:47 ` Thomas Jarosch
2009-04-15 7:50 ` Jeff King
2009-04-14 22:38 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Jarosch @ 2009-04-14 21:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git, markus.heidelberg
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-14 21:47 ` Thomas Jarosch
@ 2009-04-15 7:50 ` Jeff King
2009-04-15 7:53 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2009-04-15 7:50 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: Johannes Schindelin, gitster, git, markus.heidelberg
On Tue, Apr 14, 2009 at 11:47:44PM +0200, Thomas Jarosch wrote:
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-15 7:50 ` Jeff King
@ 2009-04-15 7:53 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2009-04-15 7:53 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: Johannes Schindelin, gitster, git, markus.heidelberg
On Wed, Apr 15, 2009 at 03:50:35AM -0400, Jeff King wrote:
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-14 21:41 ` Johannes Schindelin
2009-04-14 21:47 ` Thomas Jarosch
@ 2009-04-14 22:38 ` Junio C Hamano
2009-04-15 7:11 ` Johannes Sixt
1 sibling, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2009-04-14 22:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Thomas Jarosch, gitster, git, markus.heidelberg
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?"
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-14 22:38 ` Junio C Hamano
@ 2009-04-15 7:11 ` Johannes Sixt
2009-04-15 7:39 ` Johannes Sixt
0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2009-04-15 7:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Thomas Jarosch, git, markus.heidelberg
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)
-- Hannes
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Fix buffer overflow in config parser
2009-04-15 7:11 ` Johannes Sixt
@ 2009-04-15 7:39 ` Johannes Sixt
2009-04-17 12:05 ` [PATCH v3] " Thomas Jarosch
0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2009-04-15 7:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, Thomas Jarosch, git,
markus.heidelberg
Johannes Sixt schrieb:
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] Fix buffer overflow in config parser
2009-04-15 7:39 ` Johannes Sixt
@ 2009-04-17 12:05 ` Thomas Jarosch
2009-04-17 13:16 ` Johannes Schindelin
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Jarosch @ 2009-04-17 12:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Johannes Schindelin, Johannes Sixt,
markus.heidelberg, Jeff King
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] Fix buffer overflow in config parser
2009-04-17 12:05 ` [PATCH v3] " Thomas Jarosch
@ 2009-04-17 13:16 ` Johannes Schindelin
0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2009-04-17 13:16 UTC (permalink / raw)
To: Thomas Jarosch
Cc: git, Junio C Hamano, Johannes Sixt, markus.heidelberg, Jeff King
Hi,
On Fri, 17 Apr 2009, Thomas Jarosch wrote:
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-04-17 13:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-14 21:28 [PATCH v2] Fix buffer overflow in config parser Thomas Jarosch
2009-04-14 21:41 ` Johannes Schindelin
2009-04-14 21:47 ` Thomas Jarosch
2009-04-15 7:50 ` Jeff King
2009-04-15 7:53 ` Jeff King
2009-04-14 22:38 ` Junio C Hamano
2009-04-15 7:11 ` Johannes Sixt
2009-04-15 7:39 ` Johannes Sixt
2009-04-17 12:05 ` [PATCH v3] " Thomas Jarosch
2009-04-17 13:16 ` Johannes Schindelin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).