# [PATCH] Use strncpy to protect from buffer overruns. 5 messages from 2010-06-09 to 2010-06-09. Participants: Steven Michalske, Alex Riesen. Thread: https://gitlist.dev/t/24049 ## Steven Michalske, 2010-06-09 10:22 Subject: [PATCH] Use strncpy to protect from buffer overruns. Message-ID: <1276078921-25429-1-git-send-email-smichalske@gmail.com> URL: https://gitlist.dev/e/1276078921-25429-1-git-send-email-smichalske%40gmail.com ``` 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 --- 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 ``` ## Alex Riesen, 2010-06-09 12:44 Subject: Re: [PATCH] Use strncpy to protect from buffer overruns. Message-ID: URL: https://gitlist.dev/e/AANLkTikID-cEqdDD8XEEYmClSYEkcxdAU-rjYA2LoRVe%40mail.gmail.com In-Reply-To: <1276078921-25429-1-git-send-email-smichalske@gmail.com> ``` On Wed, Jun 9, 2010 at 12:22, Steven Michalske 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, 2010-06-09 18:25 Subject: Re: [PATCH] Use strncpy to protect from buffer overruns. Message-ID: <34152ED6-ACCC-467B-9076-1A742612AC75@gmail.com> URL: https://gitlist.dev/e/34152ED6-ACCC-467B-9076-1A742612AC75%40gmail.com In-Reply-To: ``` On Jun 9, 2010, at 5:44 AM, Alex Riesen wrote: > On Wed, Jun 9, 2010 at 12:22, Steven Michalske 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. ``` ## Alex Riesen, 2010-06-09 19:31 Subject: Re: [PATCH] Use strncpy to protect from buffer overruns. Message-ID: URL: https://gitlist.dev/e/AANLkTikQpJa9gzj1riKF79MflGAKv9IqJVXpICgHLXAG%40mail.gmail.com In-Reply-To: <34152ED6-ACCC-467B-9076-1A742612AC75@gmail.com> ``` On Wed, Jun 9, 2010 at 20:25, Steven Michalske wrote: >> On Wed, Jun 9, 2010 at 12:22, Steven Michalske 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 $ ``` ## Steven Michalske, 2010-06-09 20:42 Subject: Re: [PATCH] Use strncpy to protect from buffer overruns. Message-ID: <1E40D9E3-5459-4D29-9D6D-A6528FF8407F@gmail.com> URL: https://gitlist.dev/e/1E40D9E3-5459-4D29-9D6D-A6528FF8407F%40gmail.com In-Reply-To: ``` On Jun 9, 2010, at 12:31 PM, Alex Riesen wrote: > On Wed, Jun 9, 2010 at 20:25, Steven Michalske wrote: >>> On Wed, Jun 9, 2010 at 12:22, Steven Michalske 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. ```