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@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html