{"thread":{"id":"11443","subject":"input validation in receive-pack","startedAt":"2008-01-01T21:34:51Z","lastAt":"2008-01-02T19:14:40Z","messageCount":8,"participants":["Martin Koegler","Junio C Hamano","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"64306","messageId":"20080101213451.GA26772@auto.tuwien.ac.at","threadId":"11443","inReplyTo":null,"subject":"input validation in receive-pack","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-01T21:34:51Z","receivedAt":"2008-01-01T21:34:51Z","isPatch":false,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"In the function \"static const char *update(struct command *cmd)\" in\nreceive-pack.c:\n\n|        if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&\n|            !is_null_sha1(old_sha1) &&\n|            !prefixcmp(name, \"refs/heads/\")) {\n|                struct commit *old_commit, *new_commit;\n|                struct commit_list *bases, *ent;\n|\n|                old_commit = (struct commit *)parse_object(old_sha1);\n|                new_commit = (struct commit *)parse_object(new_sha1);\n|                bases = get_merge_bases(old_commit, new_commit, 1);\n|                for (ent = bases; ent; ent = ent->next)\n|                        if (!hashcmp(old_sha1, ent->item->object.sha1))\n|                                break;\n|                free_commit_list(bases);\n|                if (!ent) {\n|                        error(\"denying non-fast forward %s\"\n|                              \" (you should pull first)\", name);\n|                        return \"non-fast forward\";\n|                }\n|        }\n\nAs far as I understand the code, it assumes, that sha1 values provided\nby the client really point to a commit. Shouldn't there be a check for\nthe object type?\n\nSome lines above:\n|        if (!prefixcmp(name, \"refs/\") && check_ref_format(name + 5)) {\n|                error(\"refusing to create funny ref '%s' remotely\", name);\n|                return \"funny refname\";\n|        }\n\nIs this code really correct? All refnames starting with \"refs/\" may\nonly contain allowed characters, while all other may contain any\ncharacters (except \\0 and \\n)?\n\nFor the updating code path, lock_any_ref_for_update calls\ncheck_ref_format, so the error will happen latter. For the delete code\npath, the refname seems not to be checked.\n\nIn the update code path, the check is done in refs.c:\n| struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n| {\n|         if (check_ref_format(ref) == -1)\n|                 return NULL;\n|         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n| }\n\ncheck_ref_format may also return -2 (less than two name levels) and -3\n(* at the end), which are ignored. Is it really intended, that\nreceive-pack can create such refs.\n\nmfg Martin Kögler\n"},{"id":"64310","messageId":"7vzlvp3oya.fsf@gitster.siamese.dyndns.org","threadId":"11443","inReplyTo":"20080101213451.GA26772@auto.tuwien.ac.at","subject":"Re: input validation in receive-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-02T03:07:25Z","receivedAt":"2008-01-02T03:07:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n\n> In the function \"static const char *update(struct command *cmd)\" in\n> receive-pack.c:\n>\n> |        if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&\n> |            !is_null_sha1(old_sha1) &&\n> |            !prefixcmp(name, \"refs/heads/\")) {\n> |                struct commit *old_commit, *new_commit;\n> |                struct commit_list *bases, *ent;\n> |\n> |                old_commit = (struct commit *)parse_object(old_sha1);\n> |                new_commit = (struct commit *)parse_object(new_sha1);\n> |                bases = get_merge_bases(old_commit, new_commit, 1);\n> |                for (ent = bases; ent; ent = ent->next)\n> |                        if (!hashcmp(old_sha1, ent->item->object.sha1))\n> |                                break;\n> |                free_commit_list(bases);\n> |                if (!ent) {\n> |                        error(\"denying non-fast forward %s\"\n> |                              \" (you should pull first)\", name);\n> |                        return \"non-fast forward\";\n> |                }\n> |        }\n>\n> As far as I understand the code, it assumes, that sha1 values provided\n> by the client really point to a commit. Shouldn't there be a check for\n> the object type?\n\nYes, 11031d7e9f34f6a20ff4a4bd4fa3e5e3c0024a57 seems to have been\na bit sloppy.  The codepath to delete a ref may need to be lax\n(see 28391a80a94d2b59d1d21f8264fe5dab91d77249) but there is no\nexcuse not to be strict when updating.\n\n> Some lines above:\n> |        if (!prefixcmp(name, \"refs/\") && check_ref_format(name + 5)) {\n> |                error(\"refusing to create funny ref '%s' remotely\", name);\n> |                return \"funny refname\";\n> |        }\n>\n> Is this code really correct?\n\nInteresting.  Things have been this way forever, I think.  I do\nnot offhand see any reason not to refuse refs outside refs/, so\nyou can try \n\n\tif (prefixcmp(name, \"refs/\") || check_ref_format(name +\t5))\n\nand see what happens.  Some people may however want to push to\nHEAD (that is \".git/HEAD\" which is outside \".git/refs\"), though.\n\n> In the update code path, the check is done in refs.c:\n> | struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n> | {\n> |         if (check_ref_format(ref) == -1)\n> |                 return NULL;\n> |         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n> | }\n>\n> check_ref_format may also return -2 (less than two name levels) and -3\n> (* at the end), which are ignored. Is it really intended, that\n> receive-pack can create such refs.\n\nMisconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that\nchanged check_ref_format() without looking at what its callers\nare checking, I think.\n"},{"id":"64314","messageId":"alpine.LNX.1.00.0801012353580.13593@iabervon.org","threadId":"11443","inReplyTo":"7vzlvp3oya.fsf@gitster.siamese.dyndns.org","subject":"Re: input validation in receive-pack","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-02T05:01:20Z","receivedAt":"2008-01-02T05:01:20Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 1 Jan 2008, Junio C Hamano wrote:\n\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> \n> > In the update code path, the check is done in refs.c:\n> > | struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n> > | {\n> > |         if (check_ref_format(ref) == -1)\n> > |                 return NULL;\n> > |         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n> > | }\n> >\n> > check_ref_format may also return -2 (less than two name levels) and -3\n> > (* at the end), which are ignored. Is it really intended, that\n> > receive-pack can create such refs.\n> \n> Misconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that\n> changed check_ref_format() without looking at what its callers\n> are checking, I think.\n\nWhen I got to it, it was already accepting -2. It clearly shouldn't accept \n-3 (and I don't know why I missed it; I was probably misinterpreting the \noriginal logic there.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"64316","messageId":"7vhchw3hkz.fsf@gitster.siamese.dyndns.org","threadId":"11443","inReplyTo":"alpine.LNX.1.00.0801012353580.13593@iabervon.org","subject":"Re: input validation in receive-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-02T05:46:36Z","receivedAt":"2008-01-02T05:46:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Tue, 1 Jan 2008, Junio C Hamano wrote:\n>\n>> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n>> \n>> > In the update code path, the check is done in refs.c:\n>> > | struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n>> > | {\n>> > |         if (check_ref_format(ref) == -1)\n>> > |                 return NULL;\n>> > |         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n>> > | }\n>> >\n>> > check_ref_format may also return -2 (less than two name levels) and -3\n>> > (* at the end), which are ignored. Is it really intended, that\n>> > receive-pack can create such refs.\n>> \n>> Misconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that\n>> changed check_ref_format() without looking at what its callers\n>> are checking, I think.\n>\n> When I got to it, it was already accepting -2. It clearly shouldn't accept \n> -3 (and I don't know why I missed it; I was probably misinterpreting the \n> original logic there.\n\nYou are right.\n\nbuiltin-commit.c uses it to lock \"HEAD\", and update_ref() calls\nit to update any ref so we cannot reject -2.  However, I do not\nthink allowing wildcard is useful for any caller.\n\n-- >8 --\nlock_any_ref_for_update(): reject wildcard return from check_ref_format\n\nRecent check_ref_format() returns -3 as well as -1 (general\nerror) and -2 (less than two levels).  The caller was explicitly\nchecking for -1, to allow \"HEAD\" but still disallow bogus refs.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n refs.c |   27 ++++++++++++++++++---------\n refs.h |    5 ++++-\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 759924d..7484a46 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -613,32 +613,37 @@ int check_ref_format(const char *ref)\n \t\twhile ((ch = *cp++) == '/')\n \t\t\t; /* tolerate duplicated slashes */\n \t\tif (!ch)\n-\t\t\treturn -1; /* should not end with slashes */\n+\t\t\t/* should not end with slashes */\n+\t\t\treturn CHECK_REF_FORMAT_ERROR;\n \n \t\t/* we are at the beginning of the path component */\n \t\tif (ch == '.')\n-\t\t\treturn -1;\n+\t\t\treturn CHECK_REF_FORMAT_ERROR;\n \t\tbad_type = bad_ref_char(ch);\n \t\tif (bad_type) {\n-\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n+\t\t\treturn (bad_type == 2 && !*cp)\n+\t\t\t\t? CHECK_REF_FORMAT_WILDCARD\n+\t\t\t\t: CHECK_REF_FORMAT_ERROR;\n \t\t}\n \n \t\t/* scan the rest of the path component */\n \t\twhile ((ch = *cp++) != 0) {\n \t\t\tbad_type = bad_ref_char(ch);\n \t\t\tif (bad_type) {\n-\t\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n+\t\t\t\treturn (bad_type == 2 && !*cp)\n+\t\t\t\t\t? CHECK_REF_FORMAT_WILDCARD\n+\t\t\t\t\t: CHECK_REF_FORMAT_ERROR;\n \t\t\t}\n \t\t\tif (ch == '/')\n \t\t\t\tbreak;\n \t\t\tif (ch == '.' && *cp == '.')\n-\t\t\t\treturn -1;\n+\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n \t\t}\n \t\tlevel++;\n \t\tif (!ch) {\n \t\t\tif (level < 2)\n-\t\t\t\treturn -2; /* at least of form \"heads/blah\" */\n-\t\t\treturn 0;\n+\t\t\t\treturn CHECK_REF_FORMAT_ONELEVEL;\n+\t\t\treturn CHECK_REF_FORMAT_OK;\n \t\t}\n \t}\n }\n@@ -816,9 +821,13 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n \n struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n {\n-\tif (check_ref_format(ref) == -1)\n+\tswitch (check_ref_format(ref)) {\n+\tcase CHECK_REF_FORMAT_ERROR:\n+\tcase CHECK_REF_FORMAT_WILDCARD:\n \t\treturn NULL;\n-\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n+\tdefault:\n+\t\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n+\t}\n }\n \n static struct lock_file packlock;\ndiff --git a/refs.h b/refs.h\nindex 9dc8aa0..9cd16f8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -52,7 +52,10 @@ int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);\n  */\n extern int for_each_reflog(each_ref_fn, void *);\n \n-/** Returns 0 if target has the right format for a ref. **/\n+#define CHECK_REF_FORMAT_OK 0\n+#define CHECK_REF_FORMAT_ERROR (-1)\n+#define CHECK_REF_FORMAT_ONELEVEL (-2)\n+#define CHECK_REF_FORMAT_WILDCARD (-3)\n extern int check_ref_format(const char *target);\n \n /** rename ref, return 0 on success **/\n"},{"id":"64321","messageId":"20080102075152.GA24401@auto.tuwien.ac.at","threadId":"11443","inReplyTo":"7vzlvp3oya.fsf@gitster.siamese.dyndns.org","subject":"Re: input validation in receive-pack","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-02T07:51:52Z","receivedAt":"2008-01-02T07:51:52Z","isPatch":false,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Tue, Jan 01, 2008 at 07:07:25PM -0800, Junio C Hamano wrote:\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > Some lines above:\n> > |        if (!prefixcmp(name, \"refs/\") && check_ref_format(name + 5)) {\n> > |                error(\"refusing to create funny ref '%s' remotely\", name);\n> > |                return \"funny refname\";\n> > |        }\n> >\n> > Is this code really correct?\n> \n> Interesting.  Things have been this way forever, I think.  I do\n> not offhand see any reason not to refuse refs outside refs/, so\n> you can try \n> \n> \tif (prefixcmp(name, \"refs/\") || check_ref_format(name +\t5))\n> \n> and see what happens. \n\nI tried this and it passed the test suite.\n\n>  Some people may however want to push to\n> HEAD (that is \".git/HEAD\" which is outside \".git/refs\"), though.\n\nIf pushing to HEAD is allowed, it bypasses the fast-forward check.\n\nAs minimum,\nif (check_ref_format(name))\nshould be safer, as it rejects totally invalid refnames (especially ../[...]).\n\nAre there more refs outside \"refs/\", which somebody would want to push to?\nIf not, the following patch could work:\n\nif (!strcmp(name, \"HEAD\") && \n   (prefixcmp(name, \"refs/\") || check_ref_format(name+5))) {\n  [..error..]\n}\n\nif (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&\n    !is_null_sha1(old_sha1) &&\n    (!prefixcmp(name, \"refs/heads/\")||!strcmp(name, \"HEAD\")) {\n[...]\n}\n\nmfg Martin Kögler\n"},{"id":"64322","messageId":"7v63yc3bcm.fsf@gitster.siamese.dyndns.org","threadId":"11443","inReplyTo":"20080102075152.GA24401@auto.tuwien.ac.at","subject":"Re: input validation in receive-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-02T08:01:13Z","receivedAt":"2008-01-02T08:01:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n\n> Are there more refs outside \"refs/\", which somebody would want to push to?\n\nAs anything outside refs/ and not HEAD are subject to gc, I do\nnot think it should even be allowed.\n\nBut this is, as everybody should be aware, very late in 1.5.4\ncycle, so I'd rather avoid bahviour change to tighten things\nbefore post-1.5.4 opens.\n"},{"id":"64335","messageId":"alpine.LNX.1.00.0801021043220.13593@iabervon.org","threadId":"11443","inReplyTo":"7vhchw3hkz.fsf@gitster.siamese.dyndns.org","subject":"Re: input validation in receive-pack","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-02T15:53:56Z","receivedAt":"2008-01-02T15:53:56Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 1 Jan 2008, Junio C Hamano wrote:\n\n> @@ -816,9 +821,13 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n>  \n>  struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n>  {\n> -\tif (check_ref_format(ref) == -1)\n> +\tswitch (check_ref_format(ref)) {\n> +\tcase CHECK_REF_FORMAT_ERROR:\n> +\tcase CHECK_REF_FORMAT_WILDCARD:\n>  \t\treturn NULL;\n> -\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n> +\tdefault:\n> +\t\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n\nIt might be wise to make \"default\" the return NULL case, and list the two \nokay cases explicitly, so it doesn't need to be changed if \ncheck_ref_format() someday gets additional \"okay for some purposes\" \nvalues.\n\nAside from that, it looks good, except that builtin-send-pack.c and \nfast-import.c should probably use the symbolic constants, too. (All other \ncallers only check whether the value is true or not).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"64345","messageId":"7vfxxgvy3j.fsf@gitster.siamese.dyndns.org","threadId":"11443","inReplyTo":"alpine.LNX.1.00.0801021043220.13593@iabervon.org","subject":"Re: input validation in receive-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-02T19:14:40Z","receivedAt":"2008-01-02T19:14:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> It might be wise to make \"default\" the return NULL case, and list the two \n> okay cases explicitly, so it doesn't need to be changed if \n> check_ref_format() someday gets additional \"okay for some purposes\" \n> values.\n\nSounds sensible.\n\n> Aside from that, it looks good, except that builtin-send-pack.c and \n> fast-import.c should probably use the symbolic constants, too. (All other \n> callers only check whether the value is true or not).\n\nAlso sensible.\n\n-- >8 --\nUpdate callers of check_ref_format()\n\nThis updates send-pack and fast-import to use symbolic constants\nfor checking the return values from check_ref_format(), and also\nfutureproof the logic in lock_any_ref_for_update() to explicitly\nname the case that is usually considered an error but is Ok for\nthis particular use.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-send-pack.c |   10 ++++++----\n fast-import.c       |    5 +++--\n refs.c              |    6 +++---\n 3 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 25ae1fe..8afb1d0 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -541,10 +541,12 @@ static void verify_remote_names(int nr_heads, const char **heads)\n \t\tremote = remote ? (remote + 1) : heads[i];\n \t\tswitch (check_ref_format(remote)) {\n \t\tcase 0: /* ok */\n-\t\tcase -2: /* ok but a single level -- that is fine for\n-\t\t\t  * a match pattern.\n-\t\t\t  */\n-\t\tcase -3: /* ok but ends with a pattern-match character */\n+\t\tcase CHECK_REF_FORMAT_ONELEVEL:\n+\t\t\t/* ok but a single level -- that is fine for\n+\t\t\t * a match pattern.\n+\t\t\t */\n+\t\tcase CHECK_REF_FORMAT_WILDCARD:\n+\t\t\t/* ok but ends with a pattern-match character */\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"remote part of refspec is not a valid name in %s\",\ndiff --git a/fast-import.c b/fast-import.c\nindex 4646c05..74597c9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -642,8 +642,9 @@ static struct branch *new_branch(const char *name)\n \tif (b)\n \t\tdie(\"Invalid attempt to create duplicate branch: %s\", name);\n \tswitch (check_ref_format(name)) {\n-\tcase  0: break; /* its valid */\n-\tcase -2: break; /* valid, but too few '/', allow anyway */\n+\tcase 0: break; /* its valid */\n+\tcase CHECK_REF_FORMAT_ONELEVEL:\n+\t\tbreak; /* valid, but too few '/', allow anyway */\n \tdefault:\n \t\tdie(\"Branch name doesn't conform to GIT standards: %s\", name);\n \t}\ndiff --git a/refs.c b/refs.c\nindex 7484a46..58f6d17 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -822,10 +822,10 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n {\n \tswitch (check_ref_format(ref)) {\n-\tcase CHECK_REF_FORMAT_ERROR:\n-\tcase CHECK_REF_FORMAT_WILDCARD:\n-\t\treturn NULL;\n \tdefault:\n+\t\treturn NULL;\n+\tcase 0:\n+\tcase CHECK_REF_FORMAT_ONELEVEL:\n \t\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n \t}\n }\n"}]}