{"thread":{"id":"34364","subject":"[PATCH] lockfile: fix buffer overflow in path handling","startedAt":"2013-07-06T19:48:52Z","lastAt":"2013-07-07T17:29:19Z","messageCount":4,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"222676","messageId":"1373140132-12351-1-git-send-email-mhagger@alum.mit.edu","threadId":"34364","inReplyTo":null,"subject":"[PATCH] lockfile: fix buffer overflow in path handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-06T19:48:52Z","receivedAt":"2013-07-06T19:48:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The path of the file to be locked is held in lock_file::filename,\nwhich is a fixed-length buffer of length PATH_MAX.  This buffer is\nalso (temporarily) used to hold the path of the lock file, which is\nthe path of the file being locked plus \".lock\".  Because of this, the\npath of the file being locked must be less than (PATH_MAX - 5)\ncharacters long (5 chars are needed for \".lock\" and one character for\nthe NUL terminator).\n\nOn entry into lock_file(), the path length was only verified to be\nless than PATH_MAX characters, not less than (PATH_MAX - 5)\ncharacters.\n\nWhen and if resolve_symlink() is called, then that function is\ncorrectly told to treat the buffer as (PATH_MAX - 5) characters long.\nThis part is correct.  However:\n\n* If LOCK_NODEREF was specified, then resolve_symlink() is never\n  called.\n\n* If resolve_symlink() is called but the path is not a symlink, then\n  the length check is never applied.\n\nSo it is possible for a path with length (PATH_MAX - 5 <= len <\nPATH_MAX) to make it through the checks.  When \".lock\" is strcat()ted\nto such a path, the lock_file::filename buffer is overflowed.\n\nFix the problem by adding a check when entering lock_file() that the\noriginal path is less than (PATH_MAX - 5) characters.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nThis patch applies to maint.  I discovered it when working on\nsomething else.  From a brief check of history, it looks like this\ncode has always been vulnerable to buffer overflows in one way or\nanother.\n\nI haven't tried to evaluate whether this problem could be exploited\nremotely.  Even if so, the ramifications are likely to be limited to\ndenial-of-service, because the data that are written past the end of\nthe buffer are not under the control of the user; they would be one of\nthe strings \"lock\\0\", \"ock\\0\", \"ck\\0\", \"k\\0\", or \"\\0\", depending on\nthe length of the path to be locked.\n\nI'm thinking of rewriting this code to use strbufs to prevent similar\nproblems in the future, but that is a more extensive change that\nwouldn't be appropriate for maint.\n\n lockfile.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c6fb77b..3ac49cb 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -124,13 +124,13 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\tif (strlen(path) >= sizeof(lk->filename))\n-\t\treturn -1;\n-\tstrcpy(lk->filename, path);\n \t/*\n \t * subtract 5 from size to make sure there's room for adding\n \t * \".lock\" for the lock file name\n \t */\n+\tif (strlen(path) >= sizeof(lk->filename)-5)\n+\t\treturn -1;\n+\tstrcpy(lk->filename, path);\n \tif (!(flags & LOCK_NODEREF))\n \t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n \tstrcat(lk->filename, \".lock\");\n-- \n1.8.3.2\n"},{"id":"222698","messageId":"20130707041236.GB30898@sigill.intra.peff.net","threadId":"34364","inReplyTo":"1373140132-12351-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] lockfile: fix buffer overflow in path handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-07T04:12:36Z","receivedAt":"2013-07-07T04:12:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 06, 2013 at 09:48:52PM +0200, Michael Haggerty wrote:\n\n> When and if resolve_symlink() is called, then that function is\n> correctly told to treat the buffer as (PATH_MAX - 5) characters long.\n> This part is correct.  However:\n> \n> * If LOCK_NODEREF was specified, then resolve_symlink() is never\n>   called.\n> \n> * If resolve_symlink() is called but the path is not a symlink, then\n>   the length check is never applied.\n> \n> So it is possible for a path with length (PATH_MAX - 5 <= len <\n> PATH_MAX) to make it through the checks.  When \".lock\" is strcat()ted\n> to such a path, the lock_file::filename buffer is overflowed.\n\nThanks for posting this. I independently discovered this about a month\nago while working on an unrelated series, and then let it languish\nunseen and forgotten at the base of that almost-done series.\n\nSo definitely a problem, and my patch looked almost identical to\nyours. The only difference is:\n\n>  static int lock_file(struct lock_file *lk, const char *path, int flags)\n>  {\n> -\tif (strlen(path) >= sizeof(lk->filename))\n> -\t\treturn -1;\n> -\tstrcpy(lk->filename, path);\n>  \t/*\n>  \t * subtract 5 from size to make sure there's room for adding\n>  \t * \".lock\" for the lock file name\n>  \t */\n> +\tif (strlen(path) >= sizeof(lk->filename)-5)\n> +\t\treturn -1;\n> +\tstrcpy(lk->filename, path);\n>  \tif (!(flags & LOCK_NODEREF))\n>  \t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n\nIt might be worth consolidating the magic \"-5\" into a constant near the\ncomment, like this:\n\ndiff --git a/lockfile.c b/lockfile.c\nindex c6fb77b..2aeb2bb 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -124,15 +124,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\tif (strlen(path) >= sizeof(lk->filename))\n-\t\treturn -1;\n-\tstrcpy(lk->filename, path);\n \t/*\n \t * subtract 5 from size to make sure there's room for adding\n \t * \".lock\" for the lock file name\n \t */\n+\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n+\tif (strlen(path) >= max_path_len)\n+\t\treturn -1;\n+\tstrcpy(lk->filename, path);\n \tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n+\t\tresolve_symlink(lk->filename, max_path_len);\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n\nBut either way, the fix looks good to me.\n\n-Peff\n"},{"id":"222738","messageId":"51D94225.1010803@alum.mit.edu","threadId":"34364","inReplyTo":"20130707041236.GB30898@sigill.intra.peff.net","subject":"Re: [PATCH] lockfile: fix buffer overflow in path handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-07T10:25:41Z","receivedAt":"2013-07-07T10:25:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/07/2013 06:12 AM, Jeff King wrote:\n> On Sat, Jul 06, 2013 at 09:48:52PM +0200, Michael Haggerty wrote:\n> \n>> When and if resolve_symlink() is called, then that function is\n>> correctly told to treat the buffer as (PATH_MAX - 5) characters long.\n>> This part is correct.  However:\n>>\n>> * If LOCK_NODEREF was specified, then resolve_symlink() is never\n>>   called.\n>>\n>> * If resolve_symlink() is called but the path is not a symlink, then\n>>   the length check is never applied.\n>>\n>> So it is possible for a path with length (PATH_MAX - 5 <= len <\n>> PATH_MAX) to make it through the checks.  When \".lock\" is strcat()ted\n>> to such a path, the lock_file::filename buffer is overflowed.\n> \n> Thanks for posting this. I independently discovered this about a month\n> ago while working on an unrelated series, and then let it languish\n> unseen and forgotten at the base of that almost-done series.\n> \n> So definitely a problem, and my patch looked almost identical to\n> yours. The only difference is:\n> \n>>  static int lock_file(struct lock_file *lk, const char *path, int flags)\n>>  {\n>> -\tif (strlen(path) >= sizeof(lk->filename))\n>> -\t\treturn -1;\n>> -\tstrcpy(lk->filename, path);\n>>  \t/*\n>>  \t * subtract 5 from size to make sure there's room for adding\n>>  \t * \".lock\" for the lock file name\n>>  \t */\n>> +\tif (strlen(path) >= sizeof(lk->filename)-5)\n>> +\t\treturn -1;\n>> +\tstrcpy(lk->filename, path);\n>>  \tif (!(flags & LOCK_NODEREF))\n>>  \t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n> \n> It might be worth consolidating the magic \"-5\" into a constant near the\n> comment, like this:\n> \n> diff --git a/lockfile.c b/lockfile.c\n> index c6fb77b..2aeb2bb 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -124,15 +124,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>  \n>  static int lock_file(struct lock_file *lk, const char *path, int flags)\n>  {\n> -\tif (strlen(path) >= sizeof(lk->filename))\n> -\t\treturn -1;\n> -\tstrcpy(lk->filename, path);\n>  \t/*\n>  \t * subtract 5 from size to make sure there's room for adding\n>  \t * \".lock\" for the lock file name\n>  \t */\n> +\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n> +\tif (strlen(path) >= max_path_len)\n> +\t\treturn -1;\n> +\tstrcpy(lk->filename, path);\n>  \tif (!(flags & LOCK_NODEREF))\n> -\t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n> +\t\tresolve_symlink(lk->filename, max_path_len);\n>  \tstrcat(lk->filename, \".lock\");\n>  \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n>  \tif (0 <= lk->fd) {\n> \n> But either way, the fix looks good to me.\n\nYes, the constant is an improvement and Peff's version is also fine with me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222751","messageId":"7v7gh25le8.fsf@alter.siamese.dyndns.org","threadId":"34364","inReplyTo":"51D94225.1010803@alum.mit.edu","subject":"Re: [PATCH] lockfile: fix buffer overflow in path handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T17:29:19Z","receivedAt":"2013-07-07T17:29:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> But either way, the fix looks good to me.\n>\n> Yes, the constant is an improvement and Peff's version is also fine with me.\n\nOK, will squash in.  Thanks both.\n"}]}