{"thread":{"id":"30354","subject":"[PATCH] create_ref_entry(): move check_refname_format() call to callers","startedAt":"2012-04-29T06:18:08Z","lastAt":"2012-05-02T15:38:04Z","messageCount":8,"participants":["mhagger@alum.mit.edu","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"190243","messageId":"1335680288-5128-1-git-send-email-mhagger@alum.mit.edu","threadId":"30354","inReplyTo":null,"subject":"[PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-04-29T06:18:08Z","receivedAt":"2012-04-29T06:18:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThis allows the caller to decide what flags to use for\ncheck_refname_format().\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nOn 04/27/2012 05:06 PM, Junio C Hamano wrote:\n> Jeff King<peff@peff.net>  writes:\n> \n>> I upgraded git on a machine recently, and it created problems for a repo\n>> with a bogus character in a ref name.  Older versions of git never\n>> complained about it. Newer ones, containing your dce4bab (\"add_ref():\n>> verify that the refname is formatted correctly\") do. That's fine; it's\n>> bogus and git _should_ complain about it.\n>>\n>> However, recovering from the situation is unnecessarily hard, ...\n>> ...\n>> I seem to recall discussing this format-tightening and trying to be sure\n>> that users were left with a way forward for fixing their repos. But I\n>> can't find the discussion, and I don't recall any conclusion we came to.\n> \n> I haven't dug the archive but I do recall pointing many issues out\n> around the theme \"be liberal in what you accept and strict in what you\n> produce\" on this topic, and loosening one or two showstoppers during the\n> review cycle, but obviously we did not catch all of them.\n> \n> Michael?\n\nI will work on providing more infrastructure for checking refnames at\nvarying levels of strictness, but I don't know enough about the code\npaths to be able to find the places where the strictness levels need\ntweaking.\n\nFor this to work, the various callers of check_refname_format() will\nhave to be able to influence the level of strictness that they want to\nenforce.  This patch is one trivial step in that direction.\n\n refs.c |   23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 09322fe..bc735e7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -172,15 +172,11 @@ struct ref_entry {\n };\n \n static struct ref_entry *create_ref_entry(const char *refname,\n-\t\t\t\t\t  const unsigned char *sha1, int flag,\n-\t\t\t\t\t  int check_name)\n+\t\t\t\t\t  const unsigned char *sha1, int flag)\n {\n \tint len;\n \tstruct ref_entry *ref;\n \n-\tif (check_name &&\n-\t    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n-\t\tdie(\"Reference has invalid format: '%s'\", refname);\n \tlen = strlen(refname) + 1;\n \tref = xmalloc(sizeof(struct ref_entry) + len);\n \thashcpy(ref->u.value.sha1, sha1);\n@@ -710,7 +706,11 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \n \t\trefname = parse_ref_line(refline, sha1);\n \t\tif (refname) {\n-\t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n+\t\t\tif (check_refname_format(refname,\n+\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL|\n+\t\t\t\t\t\t REFNAME_DOT_COMPONENT))\n+\t\t\t\tdie(\"Packed reference has invalid format: '%s'\", refname);\n+\t\t\tlast = create_ref_entry(refname, sha1, flag);\n \t\t\tadd_ref(dir, last);\n \t\t\tcontinue;\n \t\t}\n@@ -745,8 +745,11 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \n void add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n+\tif (check_refname_format(refname,\n+\t\t\t\t REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n+\t\tdie(\"Reference has invalid format: '%s'\", refname);\n \tadd_ref(get_packed_refs(get_ref_cache(NULL)),\n-\t\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n+\t\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED));\n }\n \n static void get_ref_dir(struct ref_cache *refs, const char *base,\n@@ -805,7 +808,11 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t}\n-\t\t\tadd_ref(dir, create_ref_entry(refname, sha1, flag, 1));\n+\t\t\tif (check_refname_format(refname,\n+\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL|\n+\t\t\t\t\t\t REFNAME_DOT_COMPONENT))\n+\t\t\t\tdie(\"Loose reference has invalid format: '%s'\", refname);\n+\t\t\tadd_ref(dir, create_ref_entry(refname, sha1, flag));\n \t\t}\n \t\tfree(refname);\n \t\tclosedir(d);\n-- \n1.7.10\n"},{"id":"190247","messageId":"20120429115831.GC24254@sigill.intra.peff.net","threadId":"30354","inReplyTo":"1335680288-5128-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-29T11:58:31Z","receivedAt":"2012-04-29T11:58:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 29, 2012 at 08:18:08AM +0200, mhagger@alum.mit.edu wrote:\n\n> I will work on providing more infrastructure for checking refnames at\n> varying levels of strictness, but I don't know enough about the code\n> paths to be able to find the places where the strictness levels need\n> tweaking.\n> \n> For this to work, the various callers of check_refname_format() will\n> have to be able to influence the level of strictness that they want to\n> enforce.  This patch is one trivial step in that direction.\n\nIt seems like the create_ref_entry code paths should _always_ just be\nissuing warnings, as they are about reading existing refs, no? The die()\nside should only come when we are writing refs.\n\nSo something like this patch:\n\ndiff --git a/refs.c b/refs.c\nindex a5802e1..3dba205 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -180,7 +180,7 @@ static struct ref_entry *create_ref_entry(const char *refname,\n \n \tif (check_name &&\n \t    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n-\t\tdie(\"Reference has invalid format: '%s'\", refname);\n+\t\twarning(\"Reference has invalid format: '%s'\", refname);\n \tlen = strlen(refname) + 1;\n \tref = xmalloc(sizeof(struct ref_entry) + len);\n \thashcpy(ref->u.value.sha1, sha1);\n@@ -926,7 +926,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \tif (flag)\n \t\t*flag = 0;\n \n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n+\tif (!reading && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n \t\treturn NULL;\n \n \tfor (;;) {\n\nwhich gives me the following behavior on the snippet I posted earlier:\n\n  $ git fsck\n  warning: Reference has invalid format: 'refs/tags/foo^?bar'\n\n  $ git rev-parse --verify \"$evil\"\n  711078d8c4c0d26b02afee9f385a64455fe4cccd\n\n  $ git for-each-ref\n  warning: Reference has invalid format: 'refs/tags/foo^?bar'\n  711078d8c4c0d26b02afee9f385a64455fe4cccd commit refs/heads/master\n  711078d8c4c0d26b02afee9f385a64455fe4cccd commit refs/tags/foobar\n\n  $ git tag -l\n  warning: Reference has invalid format: 'refs/tags/foo^?bar'\n  foo^?bar\n\n  $ git tag fixed \"$evil\"\n  warning: Reference has invalid format: 'refs/tags/foo^?bar'\n  $ git rev-parse fixed\n  711078d8c4c0d26b02afee9f385a64455fe4cccd\n  $ git tag -d \"$evil\"\n  Deleted tag 'foo^?bar' (was 711078d)\n\n-Peff\n"},{"id":"190279","messageId":"7v1un5iw3t.fsf@alter.siamese.dyndns.org","threadId":"30354","inReplyTo":"20120429115831.GC24254@sigill.intra.peff.net","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-30T06:15:18Z","receivedAt":"2012-04-30T06:15:18Z","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> On Sun, Apr 29, 2012 at 08:18:08AM +0200, mhagger@alum.mit.edu wrote:\n> ...\n> It seems like the create_ref_entry code paths should _always_ just be\n> issuing warnings, as they are about reading existing refs, no? The die()\n> side should only come when we are writing refs.\n\nThat matches my observation.  Michael, are we missing something?\n"},{"id":"190312","messageId":"4F9EBB6D.3090900@alum.mit.edu","threadId":"30354","inReplyTo":"20120429115831.GC24254@sigill.intra.peff.net","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-04-30T16:18:53Z","receivedAt":"2012-04-30T16:18:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2012 01:58 PM, Jeff King wrote:\n> On Sun, Apr 29, 2012 at 08:18:08AM +0200, mhagger@alum.mit.edu wrote:\n>\n>> I will work on providing more infrastructure for checking refnames at\n>> varying levels of strictness, but I don't know enough about the code\n>> paths to be able to find the places where the strictness levels need\n>> tweaking.\n>>\n>> For this to work, the various callers of check_refname_format() will\n>> have to be able to influence the level of strictness that they want to\n>> enforce.  This patch is one trivial step in that direction.\n>\n> It seems like the create_ref_entry code paths should _always_ just be\n> issuing warnings, as they are about reading existing refs, no? The die()\n> side should only come when we are writing refs.\n\nThere are not two but three classes of refnames:\n\n1. Fully legal refnames (according to the rules of check-ref-format).\n\n2. Refnames that might cause parsing trouble in some circumstances but \ncould otherwise be tolerated (with a warning) in the hope that the user \ncan delete them before they cause further confusion.  These include all \nillegal refnames that aren't covered by (3).\n\n3. Refnames that are so pathological that we don't want to let them into \nour code at all, under any circumstances.  This category, I think, \nincludes refnames that include \"/../\" (because they could cause our code \nto walk up the filesystem) or LF (because if written to a packed-refs \nfile they would make it unreadable) and perhaps \"//\" (because they would \nconfuse the loose reference code and the hierarchical reference cache \ncode).  And depending on how much you trust shell scripts to do quoting \ncorrectly, other patterns might also be risky.\n\nI would like to be able to distinguish between (2) and (3) before \ndeciding what to do in any specific cases.  References in category (2) \ncan probably be accepted (with a warning) in old data but we should not \nallow the user to create new ones.  References in category (3) are more \ncritical; I see three options for dealing with them: die(), emit a \nwarning then drop them on the floor, or emit a warning but handle them \nsomehow (for example, by URL-encoding them).\n\nTreating category (3) the same as category (2) was more or less the \nstatus quo before the changes, but I think it is dangerous, especially \nwhen dealing with references that might come from remote sources (do you \ndisagree?).\n\n\nRegarding create_ref_entry(), there are three callers:\n\n* read_packed_refs(): Only used to read references from local\n   packed-refs files.  Seems uncritical in terms of security, so for now\n   I suppose we could change this caller to emit a warning but use the\n   reference anyway.  (It would not be a good idea to emit a warning but\n   *not* use the reference, because the next time the packed-refs file\n   is rewritten the reference would be lost.)\n\n* get_ref_dir(): Only used to read loose references from the local\n   filesystem.  Seems uncritical in terms of security, so for now\n   I suppose we could change this caller to emit a warning but use the\n   reference anyway.  Alternately, we could emit a warning but not use\n   the reference; this would not result in any data loss because nobody\n   would have a reason to remove the loose reference file.\n\n* add_packed_ref(): Used by write_remote_refs() to insert references\n   from a cloned repository into the local packed-refs file.  Here I\n   think we have to be paranoid about accepting refnames in category\n   (3).  For example, it might be reasonable to emit a warning\n   mentioning the illegal reference name *and the SHA1 that it referred\n   to* then to drop it on the floor.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"190317","messageId":"7vd36pgn0e.fsf@alter.siamese.dyndns.org","threadId":"30354","inReplyTo":"4F9EBB6D.3090900@alum.mit.edu","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-30T17:14:41Z","receivedAt":"2012-04-30T17:14:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> There are not two but three classes of refnames:\n>\n> 1. Fully legal refnames (according to the rules of check-ref-format).\n>\n> 2. Refnames that might cause parsing trouble in some circumstances but\n> could otherwise be tolerated (with a warning) in the hope that the\n> user can delete them before they cause further confusion.  These\n> include all illegal refnames that aren't covered by (3).\n>\n> 3. Refnames that are so pathological that we don't want to let them\n> into our code at all, under any circumstances.  This category, I\n> think, includes refnames that include \"/../\" (because they could cause\n> our code to walk up the filesystem) or LF (because if written to a\n> packed-refs file they would make it unreadable) and perhaps \"//\"\n> (because they would confuse the loose reference code and the\n> hierarchical reference cache code).  And depending on how much you\n> trust shell scripts to do quoting correctly, other patterns might also\n> be risky.\n\nI can buy that we would want to reject /../ or // from the command line\neven when we are not creating such a ref (e.g. \"git symbolic-ref HEAD\nrefs/heads/../heads/foo\"). Note that even though we won't read any loose\nref of that form from the repository ever (unless your recent ref API\nupdates broke it ;-), we _could_ see them in .git/packed-refs if the user\nmanually edited the file. In such a case, I would say that recovering from\nsuch a corruption is totally up to the user, just like it is up to you\nafter you edit the bits on the disk platter to confuse the filesystem\ncode.\n\nBut if we were to do so, class (3) needs to be tightly controlled. For\nexample, a ref \"refs/heads/m${LF}aster\", e.g.\n\n    $ git update-ref 'refs/heads/m\n    aster' HEAD\n\nmay have been usable with an ancient git, and as long as the you never\npacked your refs, you should be able to see it with 'git branch', and you\nshould be able to fix it with 'git branch master \"m${LF}aster\"'.\n\n> I would like to be able to distinguish between (2) and (3) before\n> deciding what to do in any specific cases.  References in category (2)\n> can probably be accepted (with a warning) in old data but we should\n> not allow the user to create new ones.\n\nThere is no probably about it.  We should warn and accept---anything else\nis a regression---and refuse to create.\n\n> References in category (3) are\n> more critical; I see three options for dealing with them: die(), emit\n> a warning then drop them on the floor, or emit a warning but handle\n> them somehow (for example, by URL-encoding them).\n\nAs long as class (3) is tightly controlled, die() until the corruption is\nsorted out (if it is coming to us by reading from existing data) would be\nthe best option to avoid spreading the breakage further.\n\n> Treating category (3) the same as category (2) was more or less the\n> status quo before the changes, but I think it is dangerous, especially\n> when dealing with references that might come from remote sources (do\n> you disagree?).\n\nI actually do. You say \"parsing trouble\" in (2), but what are the examples\nof \"parsing trouble\" you have in mind?  \"refs/heads/a..b\" is an illegal\nname per check-ref-format and is (and should be) in class (2), but as long\nas we do not create such a ref, if we happen to be able to read it (maybe\n\"git rev-parse HEAD >.git/refs/heads/a..b\" dropped it there), the user\nshould be able to see it with \"git branch\", and should be able to recover\nwith \"git branch -m a..b a-dot-dot-b\". Until the user does so, we cannot\nsensibly respond to \"git log a..b\" (which is the \"parsing trouble\" you are\nreferring to, I think), of course, but that is to be expected.\n\nWhat kind of \"danger\" do you have in mind?\n\n> Regarding create_ref_entry(), there are three callers:\n>\n> * read_packed_refs(): Only used to read references from local\n>   packed-refs files.  Seems uncritical in terms of security, so for now\n>   I suppose we could change this caller to emit a warning but use the\n>   reference anyway.\n\nThere is no _could_.  This must show it with a warning and let the caller\nuse its value.\n\n> * get_ref_dir(): Only used to read loose references from the local\n>   filesystem.\n\nLikewise.\n\n> * add_packed_ref(): Used by write_remote_refs() to insert references\n>   from a cloned repository into the local packed-refs file.\n\nThe caller should be checking the names and rejecting the attempt to\ncreate a ref with a bogus name.\n\nI think create_ref_entry(), if the reader does not pay much attention to\nthe \"_entry\" part of its name, gives a false impression that this is about\n\"creating a ref\", but it isn't.  It is actually about keeping an in-core\nimage of the outside world, and the one that affects the outside world\n(either loose or packed refs) should be making sure the names are safe\nones to use.\n\nThanks for a good summary.\n"},{"id":"190341","messageId":"4F9EF60D.8030301@alum.mit.edu","threadId":"30354","inReplyTo":"7vd36pgn0e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-04-30T20:29:01Z","receivedAt":"2012-04-30T20:29:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/30/2012 07:14 PM, Junio C Hamano wrote:\n> Michael Haggerty<mhagger@alum.mit.edu>  writes:\n>\n>> There are not two but three classes of refnames:\n>>\n>> 1. Fully legal refnames (according to the rules of check-ref-format).\n>>\n>> 2. Refnames that might cause parsing trouble in some circumstances but\n>> could otherwise be tolerated (with a warning) in the hope that the\n>> user can delete them before they cause further confusion.  These\n>> include all illegal refnames that aren't covered by (3).\n>>\n>> 3. Refnames that are so pathological that we don't want to let them\n>> into our code at all, under any circumstances.  This category, I\n>> think, includes refnames that include \"/../\" (because they could cause\n>> our code to walk up the filesystem) or LF (because if written to a\n>> packed-refs file they would make it unreadable) and perhaps \"//\"\n>> (because they would confuse the loose reference code and the\n>> hierarchical reference cache code).  And depending on how much you\n>> trust shell scripts to do quoting correctly, other patterns might also\n>> be risky.\n>>[...]\n>> Treating category (3) the same as category (2) was more or less the\n>> status quo before the changes, but I think it is dangerous, especially\n>> when dealing with references that might come from remote sources (do\n>> you disagree?).\n>\n> I actually do. [...example involving class (2) refname omitted...]\n>\n> What kind of \"danger\" do you have in mind?\n\nI agree with you about how class (2) refnames can be handled, namely on \na best-effort basis, without paranoia.  The \"danger\" that I worry about \nis if we treat class (3) refnames the same non-paranoid way that we \ntreat class (2) refnames.  To be sure, my worries are at the level of \n\"it seems like a lot of data paths have to be trusted and I have not \npersonally verified that there is no danger and I have a vivid \nimagination\" rather than \"I have figured out how to implement an exploit\".\n\nFor example, have all of the following code paths been audited to make \nsure that they cannot introduce class (3) refnames into a repository \n(including via symbolic refs with class (3) targets) even in the face of \na malicious remote?  Can we (and do we want to) rely on this level of \nvigilance being sustained in the future?\n\n* git clone (for all transport types)?\n\n* git push (for all transport types)?\n\n* git fast-import?\n\n* git am?\n\n* All kinds of remote helpers (git svn, etc.) and import scripts (e.g., \nthe malicious refs might come directly from a Subversion repository)?\n\n...no doubt there are others.\n\nWhat if a malicious repository is copied using rsync (i.e., not \"git \nclone rsync:...\" but straight rsync)?  Granted, the victim would have \nworse problems if he didn't delete any version of $GIT/config that was \ncontained in the copy before using it.\n\nBanning class (3) references at lib level would provide a second level \nof defense against errors in the outer level.  Allowing \"class 3\" \nreferences at the lib level, by comparison, means putting a lot of \nconfidence in the auditing of all of the ref-creating code paths.\n\nIt's true that a loose reference cannot (by construction) contain \nconstructs like \"/../\" or \"//\".  And packed refs are not stored via \nfilesystem paths, so such constructs are not a direct hazard.  But there \nare lots of ways that a packed ref can be turned into a loose ref, and \nthe process of creating a loose ref from a class (3) packed ref could \ncause trouble.\n\nI propose that I implement a REFNAME_NONSTRICT flag which relaxes the \nnormal rules as follows:\n\n* \"..\" is only forbidden if it is a complete refname component\n\n* All characters (including control characters, '?', '*', and '[') are \nallowed, except for NUL, SP, LF, ':', and '\\'.\n\n* The character sequence \"@{\" is *not* prohibited.\n\nThe reason I suggest excluding ':' and '\\' is because of their \nsignificance in DOS pathnames; let me know if you think this is unnecessary.\n\nDo you want these changes in master or in maint (or even older)?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"190348","messageId":"7vsjfkexh1.fsf@alter.siamese.dyndns.org","threadId":"30354","inReplyTo":"4F9EF60D.8030301@alum.mit.edu","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-30T21:11:38Z","receivedAt":"2012-04-30T21:11:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> For example, have all of the following code paths been audited to make\n> sure that they cannot introduce class (3) refnames into a repository\n> (including via symbolic refs with class (3) targets) even in the face\n> of a malicious remote?  Can we (and do we want to) rely on this level\n> of vigilance being sustained in the future?\n\nAuditing is one thing, but perhaps the right solution to that issue is to\nrefactor the existing code so that we have only a handful (preferrably\none) API entry point that is used to create a new ref (not to be confused\nwith create_ref_entry(), which is not necessarily about creating a ref)?\n\nThe UI layer may place additional restrictions to the source data used to\neventually lead to a ref creation (e.g. your updated \"git branch\" may\nforbid you to create a branch with the name of an existing tag, perhaps),\nbut after passing its check, the API to create a new ref will do mandatory\n\"check-ref-format\" check.\n"},{"id":"190571","messageId":"4FA154DC.1060607@alum.mit.edu","threadId":"30354","inReplyTo":"7vsjfkexh1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T15:38:04Z","receivedAt":"2012-05-02T15:38:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/30/2012 11:11 PM, Junio C Hamano wrote:\n> Michael Haggerty<mhagger@alum.mit.edu>  writes:\n>\n>> For example, have all of the following code paths been audited to make\n>> sure that they cannot introduce class (3) refnames into a repository\n>> (including via symbolic refs with class (3) targets) even in the face\n>> of a malicious remote?  Can we (and do we want to) rely on this level\n>> of vigilance being sustained in the future?\n>\n> Auditing is one thing, but perhaps the right solution to that issue is to\n> refactor the existing code so that we have only a handful (preferrably\n> one) API entry point that is used to create a new ref (not to be confused\n> with create_ref_entry(), which is not necessarily about creating a ref)?\n\nYes, definitely.  And more broadly, I want refs.{c,h} to become the \n*only* mechanism for working with refs.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}