[PATCH/RFC 4/9] add sanity length check to format_person_part
- From
Will Palmer <wmpalmer@gmail.com>
- Date
- Mar 28, 2011, 23:17 UTC
- Message-ID
- <1301354251-23380-5-git-send-email-wmpalmer@gmail.com>
- In-Reply-To
- <1301354251-23380-1-git-send-email-wmpalmer@gmail.com>
previously we were relying on the length check from ident.c, ie: relying on having been given a good object to parse in the first place. If the object should have triggered the "Impossibly long personal identifier" check at commit-time, for example, it would have resulted in an overflow here.
This is another one which, due to the input constraints, would not have been a real-world problem for regular usage. I was able to cause an overflow by viewing the log of a commit with an impossibly-long Author, created via hash-object. This is admittedly a pretty far-fetched scenario, though it could potentially be considered a security issue.
In any case, the lack of it rubbed me the wrong way when I was refactoring this section, and it is trivial to add the sanity check.
Signed-off-by: Will Palmer <wmpalmer@gmail.com> --- pretty.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/pretty.c b/pretty.c index d5a724f..8a288f0 100644 --- a/pretty.c +++ b/pretty.c @@ -449,6 +449,7 @@ static size_t format_person_part(struct strbuf *sb, char part, unsigned long date = 0; char *ep; const char *name_start, *name_end, *mail_start, *mail_end, *msg_end = msg+len; + size_t name_len, mail_len; char person_name[1024]; char person_mail[1024]; @@ -469,29 +470,36 @@ static size_t format_person_part(struct strbuf *sb, char part, name_end = msg+end; while (name_end > name_start && isspace(*(name_end-1))) name_end--; + name_len = name_end-name_start; + if (name_len >= sizeof(person_name)) + goto skip; mail_start = msg+end+1; mail_end = mail_start; while (mail_end < msg_end && *mail_end != '>') mail_end++; + mail_len = mail_end-mail_start; + if (mail_len >= sizeof(person_mail)) + goto skip; if (mail_end == msg_end) goto skip; end = mail_end-msg; if (part == 'N' || part == 'E') { /* mailmap lookup */ - strlcpy(person_name, name_start, name_end-name_start+1); - strlcpy(person_mail, mail_start, mail_end-mail_start+1); + /* copy up to, and including, the end delimiter */ + strlcpy(person_name, name_start, name_len+1); + strlcpy(person_mail, mail_start, mail_len+1); mailmap_name(person_mail, sizeof(person_mail), person_name, sizeof(person_name)); name_start = person_name; - name_end = name_start + strlen(person_name); + name_len = strlen(person_name); mail_start = person_mail; - mail_end = mail_start + strlen(person_mail); + mail_len = strlen(person_mail); } if (part == 'n' || part == 'N') { /* name */ - strbuf_add(sb, name_start, name_end-name_start); + strbuf_add(sb, name_start, name_len); return placeholder_len; } if (part == 'e' || part == 'E') { /* email */ - strbuf_add(sb, mail_start, mail_end-mail_start); + strbuf_add(sb, mail_start, mail_len); return placeholder_len; }
-- 1.7.4.2