[PATCH] Use strncpy to protect from buffer overruns.

[PATCH] Use strncpy to protect from buffer overruns.

[Steven Michalske]

is_git_directory() uses strcpy with pointer arithmitic, protect it from
overflowing.  Even though we currently protect higher up when we have the
environment variable path passed in, we should protect the calls here.

Signed-off-by: Steven Michalske <smichalske@gmail.com>
---
 setup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 7e04602..0080299 100644
--- a/setup.c
+++ b/setup.c
@@ -170,22 +170,24 @@ static int is_git_directory(const char *suspect)
 	char path[PATH_MAX];
 	size_t len = strlen(suspect);
 
-	strcpy(path, suspect);
+	path[sizeof(path) - 1] = '\0';
+
+	strncpy(path, suspect, sizeof(path) - 1);
 	if (getenv(DB_ENVIRONMENT)) {
 		if (access(getenv(DB_ENVIRONMENT), X_OK))
 			return 0;
 	}
 	else {
-		strcpy(path + len, "/objects");
+		strncpy(path + len, "/objects", sizeof(path) - len - 1);
 		if (access(path, X_OK))
 			return 0;
 	}
 
-	strcpy(path + len, "/refs");
+	strncpy(path + len, "/refs", sizeof(path) - len - 1);
 	if (access(path, X_OK))
 		return 0;
 
-	strcpy(path + len, "/HEAD");
+	strncpy(path + len, "/HEAD", sizeof(path) - len - 1);
 	if (validate_headref(path))
 		return 0;
 
-- 
1.7.0.3

Re: [PATCH] Use strncpy to protect from buffer overruns.

[Alex Riesen]

On Wed, Jun 9, 2010 at 12:22, Steven Michalske <smichalske@gmail.com> wrote:
> is_git_directory() uses strcpy with pointer arithmitic, protect it from
> overflowing.  Even though we currently protect higher up when we have the
> environment variable path passed in, we should protect the calls here.

Why? The function is static.

> -       strcpy(path, suspect);
> +       path[sizeof(path) - 1] = '\0';
> +
> +       strncpy(path, suspect, sizeof(path) - 1);

And we have strlcpy for such things.

Re: [PATCH] Use strncpy to protect from buffer overruns.

[Steven Michalske]

On Jun 9, 2010, at 5:44 AM, Alex Riesen wrote:

> On Wed, Jun 9, 2010 at 12:22, Steven Michalske <smichalske@gmail.com> wrote:
>> is_git_directory() uses strcpy with pointer arithmitic, protect it from
>> overflowing.  Even though we currently protect higher up when we have the
>> environment variable path passed in, we should protect the calls here.
> 
> Why? The function is static.
> 
The code might be locally constrained.

I always assume that a bit of code can be overwritten from other portions of code.

A small vulnerability is discovered that lets an attacker remove the length check or edit the pointer in the function call, but could not squeeze in the full shell code snippet.  But the now edited function here lets you put in arbitrarily long code.

>> -       strcpy(path, suspect);
>> +       path[sizeof(path) - 1] = '\0';
>> +
>> +       strncpy(path, suspect, sizeof(path) - 1);
> 
> And we have strlcpy for such things.

It is not portable.

Re: [PATCH] Use strncpy to protect from buffer overruns.

[Alex Riesen]

On Wed, Jun 9, 2010 at 20:25, Steven Michalske <smichalske@gmail.com> wrote:
>> On Wed, Jun 9, 2010 at 12:22, Steven Michalske <smichalske@gmail.com> wrote:
>>> is_git_directory() uses strcpy with pointer arithmitic, protect it from
>>> overflowing.  Even though we currently protect higher up when we have the
>>> environment variable path passed in, we should protect the calls here.
>>
>> Why? The function is static.
>>
> The code might be locally constrained.
>
> I always assume that a bit of code can be overwritten from other portions of code.
>
> A small vulnerability is discovered that lets an attacker remove the length check
> or edit the pointer in the function call, but could not squeeze in the full shell code
> snippet.  But the now edited function here lets you put in arbitrarily long code.

Eh?

>>> -       strcpy(path, suspect);
>>> +       path[sizeof(path) - 1] = '\0';
>>> +
>>> +       strncpy(path, suspect, sizeof(path) - 1);
>>
>> And we have strlcpy for such things.
>
> It is not portable.

Git has its own copy of the function:

  $ git ls-files *strlcpy.c

  $

Re: [PATCH] Use strncpy to protect from buffer overruns.

[Steven Michalske]

On Jun 9, 2010, at 12:31 PM, Alex Riesen wrote:

> On Wed, Jun 9, 2010 at 20:25, Steven Michalske <smichalske@gmail.com> wrote:
>>> On Wed, Jun 9, 2010 at 12:22, Steven Michalske <smichalske@gmail.com> wrote:
>>>> is_git_directory() uses strcpy with pointer arithmitic, protect it from
>>>> overflowing.  Even though we currently protect higher up when we have the
>>>> environment variable path passed in, we should protect the calls here.
>>> 
>>> Why? The function is static.
>>> 
>> The code might be locally constrained.
>> 
>> I always assume that a bit of code can be overwritten from other portions of code.
>> 
>> A small vulnerability is discovered that lets an attacker remove the length check
>> or edit the pointer in the function call, but could not squeeze in the full shell code
>> snippet.  But the now edited function here lets you put in arbitrarily long code.
> 
> Eh?
> 
Basically the protection is not robust against malicious code.  It's armored with leather, not the modern full body armor.

>>>> -       strcpy(path, suspect);
>>>> +       path[sizeof(path) - 1] = '\0';
>>>> +
>>>> +       strncpy(path, suspect, sizeof(path) - 1);
>>> 
>>> And we have strlcpy for such things.
>> 
>> It is not portable.
> 
> Git has its own copy of the function:
> 
>  $ git ls-files *strlcpy.c
> 
>  $

Good to know, I could refactor with this.