{"thread":{"id":"24049","subject":"[PATCH] Use strncpy to protect from buffer overruns.","startedAt":"2010-06-09T10:22:01Z","lastAt":"2010-06-09T20:42:31Z","messageCount":5,"participants":["Steven Michalske","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"143321","messageId":"1276078921-25429-1-git-send-email-smichalske@gmail.com","threadId":"24049","inReplyTo":null,"subject":"[PATCH] Use strncpy to protect from buffer overruns.","fromName":"Steven Michalske","fromEmail":"smichalske@gmail.com","sentAt":"2010-06-09T10:22:01Z","receivedAt":"2010-06-09T10:22:01Z","isPatch":true,"sender":{"key":"smichalske@gmail.com","avatar":"https://gravatar.com/avatar/721f27456adc9ac84f3bb235f021a70015abb9e09222ae8622fc5579c6a203c1?d=mp&s=160"},"body":"is_git_directory() uses strcpy with pointer arithmitic, protect it from\noverflowing. Even though we currently protect higher up when we have the\nenvironment variable path passed in, we should protect the calls here.\n\nSigned-off-by: Steven Michalske \n---\n setup.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 7e04602..0080299 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -170,22 +170,24 @@ static int is_git_directory(const char *suspect)\n \tchar path[PATH_MAX];\n \tsize_t len = strlen(suspect);\n \n-\tstrcpy(path, suspect);\n+\tpath[sizeof(path) - 1] = '\\0';\n+\n+\tstrncpy(path, suspect, sizeof(path) - 1);\n \tif (getenv(DB_ENVIRONMENT)) {\n \t\tif (access(getenv(DB_ENVIRONMENT), X_OK))\n \t\t\treturn 0;\n \t}\n \telse {\n-\t\tstrcpy(path + len, \"/objects\");\n+\t\tstrncpy(path + len, \"/objects\", sizeof(path) - len - 1);\n \t\tif (access(path, X_OK))\n \t\t\treturn 0;\n \t}\n \n-\tstrcpy(path + len, \"/refs\");\n+\tstrncpy(path + len, \"/refs\", sizeof(path) - len - 1);\n \tif (access(path, X_OK))\n \t\treturn 0;\n \n-\tstrcpy(path + len, \"/HEAD\");\n+\tstrncpy(path + len, \"/HEAD\", sizeof(path) - len - 1);\n \tif (validate_headref(path))\n \t\treturn 0;\n \n-- \n1.7.0.3\n"},{"id":"143326","messageId":"AANLkTikID-cEqdDD8XEEYmClSYEkcxdAU-rjYA2LoRVe@mail.gmail.com","threadId":"24049","inReplyTo":"1276078921-25429-1-git-send-email-smichalske@gmail.com","subject":"Re: [PATCH] Use strncpy to protect from buffer overruns.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-09T12:44:51Z","receivedAt":"2010-06-09T12:44:51Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jun 9, 2010 at 12:22, Steven Michalske wrote:\n> is_git_directory() uses strcpy with pointer arithmitic, protect it from\n> overflowing.  Even though we currently protect higher up when we have the\n> environment variable path passed in, we should protect the calls here.\n\nWhy? The function is static.\n\n> -       strcpy(path, suspect);\n> +       path[sizeof(path) - 1] = '\\0';\n> +\n> +       strncpy(path, suspect, sizeof(path) - 1);\n\nAnd we have strlcpy for such things.\n"},{"id":"143365","messageId":"34152ED6-ACCC-467B-9076-1A742612AC75@gmail.com","threadId":"24049","inReplyTo":"AANLkTikID-cEqdDD8XEEYmClSYEkcxdAU-rjYA2LoRVe@mail.gmail.com","subject":"Re: [PATCH] Use strncpy to protect from buffer overruns.","fromName":"Steven Michalske","fromEmail":"smichalske@gmail.com","sentAt":"2010-06-09T18:25:44Z","receivedAt":"2010-06-09T18:25:44Z","isPatch":true,"sender":{"key":"smichalske@gmail.com","avatar":"https://gravatar.com/avatar/721f27456adc9ac84f3bb235f021a70015abb9e09222ae8622fc5579c6a203c1?d=mp&s=160"},"body":"\nOn Jun 9, 2010, at 5:44 AM, Alex Riesen wrote:\n\n> On Wed, Jun 9, 2010 at 12:22, Steven Michalske wrote:\n>> is_git_directory() uses strcpy with pointer arithmitic, protect it from\n>> overflowing. Even though we currently protect higher up when we have the\n>> environment variable path passed in, we should protect the calls here.\n> \n> Why? The function is static.\n> \nThe code might be locally constrained.\n\nI always assume that a bit of code can be overwritten from other portions of code.\n\nA 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.\n\n>> - strcpy(path, suspect);\n>> + path[sizeof(path) - 1] = '\\0';\n>> +\n>> + strncpy(path, suspect, sizeof(path) - 1);\n> \n> And we have strlcpy for such things.\n\nIt is not portable."},{"id":"143368","messageId":"AANLkTikQpJa9gzj1riKF79MflGAKv9IqJVXpICgHLXAG@mail.gmail.com","threadId":"24049","inReplyTo":"34152ED6-ACCC-467B-9076-1A742612AC75@gmail.com","subject":"Re: [PATCH] Use strncpy to protect from buffer overruns.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-09T19:31:09Z","receivedAt":"2010-06-09T19:31:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jun 9, 2010 at 20:25, Steven Michalske wrote:\n>> On Wed, Jun 9, 2010 at 12:22, Steven Michalske wrote:\n>>> is_git_directory() uses strcpy with pointer arithmitic, protect it from\n>>> overflowing.  Even though we currently protect higher up when we have the\n>>> environment variable path passed in, we should protect the calls here.\n>>\n>> Why? The function is static.\n>>\n> The code might be locally constrained.\n>\n> I always assume that a bit of code can be overwritten from other portions of code.\n>\n> A small vulnerability is discovered that lets an attacker remove the length check\n> or edit the pointer in the function call, but could not squeeze in the full shell code\n> snippet.  But the now edited function here lets you put in arbitrarily long code.\n\nEh?\n\n>>> -       strcpy(path, suspect);\n>>> +       path[sizeof(path) - 1] = '\\0';\n>>> +\n>>> +       strncpy(path, suspect, sizeof(path) - 1);\n>>\n>> And we have strlcpy for such things.\n>\n> It is not portable.\n\nGit has its own copy of the function:\n\n $ git ls-files *strlcpy.c\n\n $\n"},{"id":"143372","messageId":"1E40D9E3-5459-4D29-9D6D-A6528FF8407F@gmail.com","threadId":"24049","inReplyTo":"AANLkTikQpJa9gzj1riKF79MflGAKv9IqJVXpICgHLXAG@mail.gmail.com","subject":"Re: [PATCH] Use strncpy to protect from buffer overruns.","fromName":"Steven Michalske","fromEmail":"smichalske@gmail.com","sentAt":"2010-06-09T20:42:31Z","receivedAt":"2010-06-09T20:42:31Z","isPatch":true,"sender":{"key":"smichalske@gmail.com","avatar":"https://gravatar.com/avatar/721f27456adc9ac84f3bb235f021a70015abb9e09222ae8622fc5579c6a203c1?d=mp&s=160"},"body":"\nOn Jun 9, 2010, at 12:31 PM, Alex Riesen wrote:\n\n> On Wed, Jun 9, 2010 at 20:25, Steven Michalske wrote:\n>>> On Wed, Jun 9, 2010 at 12:22, Steven Michalske wrote:\n>>>> is_git_directory() uses strcpy with pointer arithmitic, protect it from\n>>>> overflowing. Even though we currently protect higher up when we have the\n>>>> environment variable path passed in, we should protect the calls here.\n>>> \n>>> Why? The function is static.\n>>> \n>> The code might be locally constrained.\n>> \n>> I always assume that a bit of code can be overwritten from other portions of code.\n>> \n>> A small vulnerability is discovered that lets an attacker remove the length check\n>> or edit the pointer in the function call, but could not squeeze in the full shell code\n>> snippet. But the now edited function here lets you put in arbitrarily long code.\n> \n> Eh?\n> \nBasically the protection is not robust against malicious code. It's armored with leather, not the modern full body armor.\n\n>>>> - strcpy(path, suspect);\n>>>> + path[sizeof(path) - 1] = '\\0';\n>>>> +\n>>>> + strncpy(path, suspect, sizeof(path) - 1);\n>>> \n>>> And we have strlcpy for such things.\n>> \n>> It is not portable.\n> \n> Git has its own copy of the function:\n> \n> $ git ls-files *strlcpy.c\n> \n> $\n\nGood to know, I could refactor with this."}]}