{"thread":{"id":"11500","subject":"[PATCH] parse_tag_buffer: don't parse invalid tags","startedAt":"2008-01-06T19:03:10Z","lastAt":"2008-01-07T07:40:09Z","messageCount":4,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"64598","messageId":"11996461912682-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11500","inReplyTo":null,"subject":"[PATCH] parse_tag_buffer: don't parse invalid tags","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-06T19:03:10Z","receivedAt":"2008-01-06T19:03:10Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"The current tag parsing code can access memory outside the tag buffer,\nif \\n are missing. This patch prevent this behaviour.\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n tag.c |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex f62bcdd..fa22ae6 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -39,6 +39,7 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \tunsigned char sha1[20];\n \tconst char *type_line, *tag_line, *sig_line;\n \tchar type[20];\n+\tconst char* start = data;\n \n         if (item->object.parsed)\n                 return 0;\n@@ -53,11 +54,11 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \tif (memcmp(\"\\ntype \", type_line-1, 6))\n \t\treturn -1;\n \n-\ttag_line = strchr(type_line, '\\n');\n+\ttag_line = memchr(type_line, '\\n', size - (type_line - start));\n \tif (!tag_line || memcmp(\"tag \", ++tag_line, 4))\n \t\treturn -1;\n \n-\tsig_line = strchr(tag_line, '\\n');\n+\tsig_line = memchr(tag_line, '\\n', size - (tag_line - start));\n \tif (!sig_line)\n \t\treturn -1;\n \tsig_line++;\n-- \n1.4.4.4\n"},{"id":"64599","messageId":"11996461913672-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11500","inReplyTo":"11996461912682-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-06T19:03:11Z","receivedAt":"2008-01-06T19:03:11Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n commit.c |   28 +++++++++++++++++++++-------\n 1 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex f074811..ffa0894 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -48,19 +48,33 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \treturn check_commit(obj, sha1, 0);\n }\n \n-static unsigned long parse_commit_date(const char *buf)\n+static unsigned long parse_commit_date(const char *buf, const char* tail)\n {\n \tunsigned long date;\n+\tchar datebuf[20];\n+\tunsigned long len;\n \n+\tif (buf + 6 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"author\", 6))\n \t\treturn 0;\n-\twhile (*buf++ != '\\n')\n+\twhile (buf < tail && *buf++ != '\\n')\n \t\t/* nada */;\n+\tif (buf + 9 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"committer\", 9))\n \t\treturn 0;\n-\twhile (*buf++ != '>')\n+\twhile (buf < tail && *buf++ != '>')\n \t\t/* nada */;\n-\tdate = strtoul(buf, NULL, 10);\n+\tif (buf >= tail)\n+\t\treturn 0;\n+\t\n+\tlen = tail - buf;\n+\tif (len > sizeof(datebuf) - 1)\n+\t  len = sizeof(datebuf) - 1;\n+\tmemcpy(datebuf, buf, len);\n+\tdatebuf[len] = 0;\n+\tdate = strtoul(datebuf, NULL, 10);\n \tif (date == ULONG_MAX)\n \t\tdate = 0;\n \treturn date;\n@@ -236,9 +250,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\treturn 0;\n \titem->object.parsed = 1;\n \ttail += size;\n-\tif (tail <= bufptr + 5 || memcmp(bufptr, \"tree \", 5))\n+\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n-\tif (tail <= bufptr + 45 || get_sha1_hex(bufptr + 5, parent) < 0)\n+\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t     sha1_to_hex(item->object.sha1));\n \titem->tree = lookup_tree(parent);\n@@ -275,7 +289,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\t\tn_refs++;\n \t\t}\n \t}\n-\titem->date = parse_commit_date(bufptr);\n+\titem->date = parse_commit_date(bufptr, tail);\n \n \tif (track_object_refs) {\n \t\tunsigned i = 0;\n-- \n1.4.4.4\n"},{"id":"64608","messageId":"7vbq7y4ns6.fsf@gitster.siamese.dyndns.org","threadId":"11500","inReplyTo":"11996461913672-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-06T22:00:57Z","receivedAt":"2008-01-06T22:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n> ---\n>  commit.c |   28 +++++++++++++++++++++-------\n>  1 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index f074811..ffa0894 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -48,19 +48,33 @@ struct commit *lookup_commit(const unsigned char *sha1)\n>  \treturn check_commit(obj, sha1, 0);\n>  }\n>  \n> -static unsigned long parse_commit_date(const char *buf)\n> +static unsigned long parse_commit_date(const char *buf, const char* tail)\n\nShould be \"const char *tail\" in our codebase.\n\n>  {\n>  \tunsigned long date;\n> +\tchar datebuf[20];\n> +\tunsigned long len;\n>  \n> +\tif (buf + 6 >= tail)\n> +\t\treturn 0;\n>  \tif (memcmp(buf, \"author\", 6))\n>  \t\treturn 0;\n\nEven though buf, which is a result from read_sha1_file(), is\nalways terminated with an extra NUL (outside its object size),\nif a bogus commit object ends with \"author\" (and without the\nauthor information) this part will pass, and ...\n\n> -\twhile (*buf++ != '\\n')\n> +\twhile (buf < tail && *buf++ != '\\n')\n>  \t\t/* nada */;\n> +\tif (buf + 9 >= tail)\n> +\t\treturn 0;\n\n... you catch that here.  That seems like a good change.\n\n>  \tif (memcmp(buf, \"committer\", 9))\n>  \t\treturn 0;\n> -\twhile (*buf++ != '>')\n> +\twhile (buf < tail && *buf++ != '>')\n>  \t\t/* nada */;\n> -\tdate = strtoul(buf, NULL, 10);\n> +\tif (buf >= tail)\n> +\t\treturn 0;\n\nLikewise here.\n\n> +\tlen = tail - buf;\n> +\tif (len > sizeof(datebuf) - 1)\n> +\t  len = sizeof(datebuf) - 1;\n\nBroken indentation.\n\n> +\tmemcpy(datebuf, buf, len);\n> +\tdatebuf[len] = 0;\n> +\tdate = strtoul(datebuf, NULL, 10);\n\nHowever, as long as buf at this point hasn't go beyond tail,\nwhich you already checked, I think we can rely on strtoul()\nstopping at the NUL at the end of buffer (that is one beyond\ntail), without this extra memcpy().  Am I mistaken?\n\n> @@ -236,9 +250,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n>  \t\treturn 0;\n>  \titem->object.parsed = 1;\n>  \ttail += size;\n> -\tif (tail <= bufptr + 5 || memcmp(bufptr, \"tree \", 5))\n> +\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n>  \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n> -\tif (tail <= bufptr + 45 || get_sha1_hex(bufptr + 5, parent) < 0)\n> +\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n>  \t\treturn error(\"bad tree pointer in commit %s\",\n>  \t\t\t     sha1_to_hex(item->object.sha1));\n>  \titem->tree = lookup_tree(parent);\n\nThis hunk is logically a no-op but I like your version better.\nIt also makes sure tree object name is terminated with a LF.\n\n> @@ -275,7 +289,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n>  \t\t\tn_refs++;\n>  \t\t}\n>  \t}\n> -\titem->date = parse_commit_date(bufptr);\n> +\titem->date = parse_commit_date(bufptr, tail);\n>  \n>  \tif (track_object_refs) {\n>  \t\tunsigned i = 0;\n> -- \n> 1.4.4.4\n\nWhen already somewhat deep in the rc cycle, looking at a patch\nfrom somebody who uses 1.4.4.4 makes me look at the patch a bit\nmore carefully than usual ;-)\n"},{"id":"64629","messageId":"20080107074009.GA32710@auto.tuwien.ac.at","threadId":"11500","inReplyTo":"7vbq7y4ns6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-07T07:40:09Z","receivedAt":"2008-01-07T07:40:09Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Jan 06, 2008 at 02:00:57PM -0800, Junio C Hamano wrote:\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n> > diff --git a/commit.c b/commit.c\n> > index f074811..ffa0894 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -48,19 +48,33 @@ struct commit *lookup_commit(const unsigned char *sha1)\n> >  \treturn check_commit(obj, sha1, 0);\n> >  }\n> >  \n> > -static unsigned long parse_commit_date(const char *buf)\n> > +static unsigned long parse_commit_date(const char *buf, const char* tail)\n> \n> Should be \"const char *tail\" in our codebase.\n\nWill fix.\n\n> >  {\n> >  \tunsigned long date;\n> > +\tchar datebuf[20];\n> > +\tunsigned long len;\n> >  \n> > +\tif (buf + 6 >= tail)\n> > +\t\treturn 0;\n> >  \tif (memcmp(buf, \"author\", 6))\n> >  \t\treturn 0;\n> \n> Even though buf, which is a result from read_sha1_file(), is\n> always terminated with an extra NUL (outside its object size),\n> if a bogus commit object ends with \"author\" (and without the\n> author information) this part will pass, and ...\n\nfsck_commit (builtin-fsck.c) does quite redundant checks (except that\nit checks for author too). Should I make parse_commit fail, if there\nis no author and commiter line? This way, we could remove \n|        if (memcmp(buffer, \"tree \", 5))\n|                return objerror(&commit->object, \"invalid format - expected 'tree' line\");\n|        if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n|                return objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n|        buffer += 46;\n|        while (!memcmp(buffer, \"parent \", 7)) {\n|                if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n|                        return objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n|                buffer += 48;\n|        }\n|        if (memcmp(buffer, \"author \", 7))\n|                return objerror(&commit->object, \"invalid format - expected 'author' line\");\nfrom fsck_commit.\n\n\n> > -\twhile (*buf++ != '\\n')\n> > +\twhile (buf < tail && *buf++ != '\\n')\n> >  \t\t/* nada */;\n> > +\tif (buf + 9 >= tail)\n> > +\t\treturn 0;\n> \n> ... you catch that here.  That seems like a good change.\n> \n> >  \tif (memcmp(buf, \"committer\", 9))\n> >  \t\treturn 0;\n> > -\twhile (*buf++ != '>')\n> > +\twhile (buf < tail && *buf++ != '>')\n> >  \t\t/* nada */;\n> > -\tdate = strtoul(buf, NULL, 10);\n> > +\tif (buf >= tail)\n> > +\t\treturn 0;\n> \n> Likewise here.\n> \n> > +\tlen = tail - buf;\n> > +\tif (len > sizeof(datebuf) - 1)\n> > +\t  len = sizeof(datebuf) - 1;\n> \n> Broken indentation.\n\nWill fix.\n\n> > +\tmemcpy(datebuf, buf, len);\n> > +\tdatebuf[len] = 0;\n> > +\tdate = strtoul(datebuf, NULL, 10);\n> \n> However, as long as buf at this point hasn't go beyond tail,\n> which you already checked, I think we can rely on strtoul()\n> stopping at the NUL at the end of buffer (that is one beyond\n> tail), without this extra memcpy().  Am I mistaken?\n\nNo.\n\nparse_commit_buffer is only called from parse_commit (safe, as it uses\na buffer by read_sha1_file) and parse_object_buffer (safe, as it is\ncalled by parse_object (safe as it uses a buffer by read_sha1_file)\nand get_obj (in builtin-for-each-ref.c, safe as it uses a buffer by\nread_sha1_file)).\n\n> > @@ -275,7 +289,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n> >  \t\t\tn_refs++;\n> >  \t\t}\n> >  \t}\n> > -\titem->date = parse_commit_date(bufptr);\n> > +\titem->date = parse_commit_date(bufptr, tail);\n> >  \n> >  \tif (track_object_refs) {\n> >  \t\tunsigned i = 0;\n> > -- \n> > 1.4.4.4\n> \n> When already somewhat deep in the rc cycle, looking at a patch\n> from somebody who uses 1.4.4.4 makes me look at the patch a bit\n> more carefully than usual ;-)\n\nstg is much simpler to use for such patches. My distribution [Linus\nalready called it insafe for it's git version] ships stg 0.11, which\nis sufficient for this. I simply use stg with the shipped git 1.4.4.4\nto avoid any compatibiltiy problems.\n\nmfg Martin Kögler\nPS:\nThe function of get_obj in builtin-for-each-ref.c looks quite similar to\nparse_object.\n"}]}