threads / patch / 24049

patchUse strncpy to protect from buffer overruns.

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

## tl;dr

5 messages between Jun 9, 2010 and Jun 9, 2010. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Steven Michalske· Jun 9, 2010, 10:22 UTC · lore

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(-)
Show changes to setup.c +6 −4
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
Alex Riesen· Jun 9, 2010, 12:44 UTC · re: Steven Michalske · lore

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

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.
Steven Michalske· Jun 9, 2010, 18:25 UTC · re: Alex Riesen · lore

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

On Jun 9, 2010, at 5:44 AM, Alex Riesen wrote:
Show 7 quoted lines
> 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.
Show 6 quoted lines
>> -       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.
Alex Riesen· Jun 9, 2010, 19:31 UTC · re: Steven Michalske · lore

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

On Wed, Jun 9, 2010 at 20:25, Steven Michalske <smichalske@gmail.com> wrote:
Show 14 quoted lines
>> 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?
Show 8 quoted lines
>>> -       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
  $
Steven Michalske· Jun 9, 2010, 20:42 UTC · re: Alex Riesen · lore

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

On Jun 9, 2010, at 12:31 PM, Alex Riesen wrote:
Show 18 quoted lines
> 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.
Show 14 quoted lines
>>>> -       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.

← back to recent threads