{"thread":{"id":"30586","subject":"[PATCH] pretty: avoid buffer overflow in format_person_part","startedAt":"2012-05-22T05:45:08Z","lastAt":"2012-05-22T20:53:57Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"191876","messageId":"20120522054508.GA10576@sigill.intra.peff.net","threadId":"30586","inReplyTo":null,"subject":"[PATCH] pretty: avoid buffer overflow in format_person_part","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-22T05:45:08Z","receivedAt":"2012-05-22T05:45:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse the name and email from a commit to\npretty-print them, we usually can just put the result\ndirectly into our strbuf result. However, if we are going to\nuse the mailmap, then we must first copy them into a\nNUL-terminated buffer to feed to the mailmap machinery.\n\nWe did so by using strlcpy into a static buffer, but we used\nit wrong. We fed it the length of the substring we wanted to\ncopy, but never checked that that length was less than the\nsize of the destination buffer.\n\nThe simplest fix is to just use snprintf to copy the\nsubstring properly while still respecting the destination\nbuffer's size. It might seem like replacing the static\nbuffer with a strbuf would help, but we need to feed a\nstatic buffer to the mailmap machinery anyway, so there's\nnot much benefit to handling arbitrary sizes.\n\nA more ideal solution would be for mailmap to grow an\ninterface that:\n\n  1. Takes a pointer and length combination, instead of\n     assuming a NUL-terminated string.\n\n  2. Returns a pointer to the mailmap's allocated string,\n     rather than copying it into the buffer.\n\nThen we could avoid the need for an extra buffer entirely.\nHowever, doing this would involve a lot of refactoring of\nmailmap and of string_list (which mailmap uses to store the\nmap itself). For now, let's do the simplest thing to fix the\nbug.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nCurrent git will not generate commits with names long enough to overflow\nhere. With my recent series, that is no longer the case. Of course, that\nis not really relevant; a malicious attacker is free to craft a bogus\ncommit directly with hash-object.\n\n pretty.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 02a0a2b..986e114 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -547,8 +547,10 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \tmail_end = s.mail_end;\n \n \tif (part == 'N' || part == 'E') { /* mailmap lookup */\n-\t\tstrlcpy(person_name, name_start, name_end - name_start + 1);\n-\t\tstrlcpy(person_mail, mail_start, mail_end - mail_start + 1);\n+\t\tsnprintf(person_name, sizeof(person_name), \"%.*s\",\n+\t\t\t (int)(name_end - name_start), name_start);\n+\t\tsnprintf(person_mail, sizeof(person_mail), \"%.*s\",\n+\t\t\t (int)(mail_end - mail_start), mail_start);\n \t\tmailmap_name(person_mail, sizeof(person_mail), person_name, sizeof(person_name));\n \t\tname_start = person_name;\n \t\tname_end = name_start + strlen(person_name);\n-- \n1.7.9.7.33.gc430a50\n"},{"id":"191949","messageId":"7vy5ok2aze.fsf@alter.siamese.dyndns.org","threadId":"30586","inReplyTo":"20120522054508.GA10576@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: avoid buffer overflow in format_person_part","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-22T20:53:57Z","receivedAt":"2012-05-22T20:53:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> A more ideal solution would be for mailmap to grow an\n> interface that:\n>\n>   1. Takes a pointer and length combination, instead of\n>      assuming a NUL-terminated string.\n>\n>   2. Returns a pointer to the mailmap's allocated string,\n>      rather than copying it into the buffer.\n\nI was looking at René's patch to refs codepath that converted some \"we\nallocate because callee wants to have NUL-terminated strings\" functions to\ntake counted strings (or whatever we want to call it---perhaps \"string\nslices\"?), and it appears that we may benefit if more callers that do not\nhave to own strings that comes to them used them instead of NUL-terminated\nstrings as their interface.  At some deep point near the leaf of the\ncallchain, they may need to be converted in order to call external API\n(read: POSIX) that takes NUL-terminated strings, but internal callchain\nlike the mailmap one, if done carefully, should let us avoid extra\nallocations.\n"}]}