{"thread":{"id":"48821","subject":"Checks added for CVE-2018-11235 too restrictive?","startedAt":"2018-07-03T07:06:59Z","lastAt":"2018-07-04T08:19:09Z","messageCount":6,"participants":["Mike Hommey","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"351576","messageId":"20180703070650.b3drk5a6kb4k4tnp@glandium.org","threadId":"48821","inReplyTo":null,"subject":"Checks added for CVE-2018-11235 too restrictive?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2018-07-03T07:06:50Z","receivedAt":"2018-07-03T07:06:59Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\n(Background) I'm the author of a git remote-helper that can talk\ndirectly to mercurial servers, known as git-cinnabar. One of the design\ndecisions was to use git objects to store all the metadata necessary to\nreconstruct mercurial changesets, manifests and files, and one special\nthing that's done is that git trees are (ab)used to store mercurial\nsha1s in commit references. This design decision was made so that the\nmetadata could possibly be exchanged, allowing to jumpstart clone of\nlarge repositories faster than by converting from scratch from\nmercurial.\n\nI had a first shot at that a few months ago, but the format of the\nmetadata branch made it impossible to push to github, hitting its push\nsize limit. With some pre-processing work, I was able to push the data\nto github, with the intent to come back with a new metadata branch\nformat that would make the push directly possible.\n\nFast forward to this week, where I was trying to upload a new metadata\nbranch, and failed in a rather interesting way: multiple lines looking\nlike:\n\nremote: error: object 81eae74b046d284c47e788143bbbcc681cb53418: gitmodulesMissing: unable to read .gitmodules blob\n\nwhich, apart from the fact that they have some formatting issue, appear\nto be new from the fixes for CVE-2018-11235.\n\nI can see what those fixes are trying to prevent, but they seem to be\noverly restrictive, at least in the context of transfer.fsckObjects.\n\nThe core problem is that the mercurial repository has some .gitmodules\nfiles in some subdirectories. As a consequence, the git objects storing\nthe info for those mercurial files contain trees with .gitmodules files\nwith a commit ref, and that's what the remote is complaining about.\n\n(Surpringly, it doesn't hit the \"non-blob found at .gitmodules\" case\nfirst, which is weird).\n\nA small testcase to reproduce looks like this:\n\n$ git init bar; cd bar\n$ git fast-import <<EOF\ncommit refs/heads/bar\ncommitter Bar <bar@bar> 0 +0000\ndata 0\n                                                                 \nM 160000 81eae74b046d284c47e788143bbbcc681cb53418 bar/.gitmodules\n                                                                 \nEOF\n$ git init ../qux\n$ git -C ../qux config receive.fsckObjects true\n$ git push -q ../qux bar\nremote: error: object 81eae74b046d284c47e788143bbbcc681cb53418: gitmodulesMissing: unable to read .gitmodules blob\nremote: fatal: fsck error in pack objects\nerror: remote unpack failed: unpack-objects abnormal exit\nTo ../qux\n ! [remote rejected] bar -> bar (unpacker error)\nerror: failed to push some refs to '../qux'\n\nWould it be reasonable to make the transfer.fsckObject checks ignore\nnon-blob .gitmodules?\n\nCheers,\n\nMike\n"},{"id":"351623","messageId":"20180703141518.GA21629@sigill.intra.peff.net","threadId":"48821","inReplyTo":"20180703070650.b3drk5a6kb4k4tnp@glandium.org","subject":"Re: Checks added for CVE-2018-11235 too restrictive?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-03T14:15:19Z","receivedAt":"2018-07-03T14:15:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 03, 2018 at 04:06:50PM +0900, Mike Hommey wrote:\n\n> I had a first shot at that a few months ago, but the format of the\n> metadata branch made it impossible to push to github, hitting its push\n> size limit. With some pre-processing work, I was able to push the data\n> to github, with the intent to come back with a new metadata branch\n> format that would make the push directly possible.\n\nThis is sort of tangential to your email here, but I'm curious which\nlimit you hit. Too-big blob, tree, or commit?\n\n> Fast forward to this week, where I was trying to upload a new metadata\n> branch, and failed in a rather interesting way: multiple lines looking\n> like:\n> \n> remote: error: object 81eae74b046d284c47e788143bbbcc681cb53418: gitmodulesMissing: unable to read .gitmodules blob\n> \n> which, apart from the fact that they have some formatting issue, appear\n> to be new from the fixes for CVE-2018-11235.\n\nAlso tangential to your main point (I promise I'll get to it ;) ), but\nwhat formatting issue do you mean? Are you referring to\n\"gitmodulesMissing\"? That's a machine-readable tag which means you can\nset \"fsck.gitmodulesMissing\" to \"ignore\".\n\n> I can see what those fixes are trying to prevent, but they seem to be\n> overly restrictive, at least in the context of transfer.fsckObjects.\n> \n> The core problem is that the mercurial repository has some .gitmodules\n> files in some subdirectories. As a consequence, the git objects storing\n> the info for those mercurial files contain trees with .gitmodules files\n> with a commit ref, and that's what the remote is complaining about.\n> \n> (Surpringly, it doesn't hit the \"non-blob found at .gitmodules\" case\n> first, which is weird).\n\nThe reason it doesn't hit the \"non-blob\" case is that we are trying to\nanalyze the object itself. So we don't pay any attention to the mode in\nthe containing tree, but instead literally look for 81eae74b0. If it\nwere a non-blob we'd complain then, but in fact we don't have it at all\n(which is otherwise OK because it's a gitlink).\n\n> A small testcase to reproduce looks like this:\n> \n> $ git init bar; cd bar\n> $ git fast-import <<EOF\n> commit refs/heads/bar\n> committer Bar <bar@bar> 0 +0000\n> data 0\n>                                                                  \n> M 160000 81eae74b046d284c47e788143bbbcc681cb53418 bar/.gitmodules\n>                                                                  \n> EOF\n>\n> [...]\n> \n> Would it be reasonable to make the transfer.fsckObject checks ignore\n> non-blob .gitmodules?\n\nI'm open to the idea that the new checks are too restrictive (both this\nand the gitmodulesParse error we're discussing in [1]). They definitely\noutlaw things that _used_ to be OK. And the security benefit is a little\nhand-wavy. They're not strictly needed to block the recent\nvulnerability[2].  However, they _could_ protect us from future problems\n(e.g., an overflow in the config-parsing code, which is not accessible\nto attackers outside of .gitmodules). So there is some value in being\nrestrictive, but it's mostly hypothetical for now.\n\nSo I think we could simply loosen these cases without immediate effect.\nThat said, I'm not sure that having a gitlink .gitmodules is sane in the\nfirst place. Attempting to \"git submodule init\" there is going to cause\nerrors. Well, sort of -- your example actually includes it in a\nsubdirectory of the repository, so technically we wouldn't use it for\nreal submodules. That fudging (finding .gitmodules anywhere instead of\njust at the root) was a conscious decision to reduce the complexity and\ncost of the check.\n\nIt sounds like in this case you don't have existing history that does\nthis with .gitmodules, but are rather looking into designing a new\nfeature that stuffs data into git trees. I'd be interested to hear a bit\nmore about that feature to see if there are other alternatives.\n\n[1] https://public-inbox.org/git/2fc2d53f-e193-2a2a-9f8f-b3e1d256d940@ramsayjones.plus.com/\n\n[2] If you do not have the object, or cannot parse it as config, it\n    cannot have a malicious config name in it. We do have to be wary of\n    cases where we get the tree in one push, and then later get the\n    pointed-to object. But I think for anything _except_ a gitlink, we'd\n    complain about the missing object for the first push. And for a\n    gitlink, we wouldn't check out the blob, so we're OK.\n"},{"id":"351630","messageId":"xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com","threadId":"48821","inReplyTo":"20180703141518.GA21629@sigill.intra.peff.net","subject":"Re: Checks added for CVE-2018-11235 too restrictive?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T17:45:51Z","receivedAt":"2018-07-03T17:45:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> A small testcase to reproduce looks like this:\n>> \n>> $ git init bar; cd bar\n>> $ git fast-import <<EOF\n>> commit refs/heads/bar\n>> committer Bar <bar@bar> 0 +0000\n>> data 0\n>>                                                                  \n>> M 160000 81eae74b046d284c47e788143bbbcc681cb53418 bar/.gitmodules\n>>                                                                  \n>> EOF\n>>\n>> [...]\n>> \n>> Would it be reasonable to make the transfer.fsckObject checks ignore\n>> non-blob .gitmodules?\n>\n> I'm open to the idea that the new checks are too restrictive (both this\n> and the gitmodulesParse error we're discussing in [1]). They definitely\n> outlaw things that _used_ to be OK. And the security benefit is a little\n> hand-wavy. They're not strictly needed to block the recent\n> vulnerability[2].  However, they _could_ protect us from future problems\n> (e.g., an overflow in the config-parsing code, which is not accessible\n> to attackers outside of .gitmodules). So there is some value in being\n> restrictive, but it's mostly hypothetical for now.\n\nAs you said elsewhere, the above example would probably not cause\nharm to the production use of .gitmodules (as opposed to what fsck\nchecks) as it is not at the top of the working tree, but does the\nproduction code do a wrong thing if a directory, a symbolic link or\na gitlink appear at \".gitmodules\" at the top level?\n\nIf so, we probably need to tighten code in submodules.c and\nelsewhere to ignore such \".gitmodules\" directory etc [*1*].\n\nAnd if not, I think it is OK to loosen fsck to exclude \".gitmodules\"\nthat are not regular files, e.g. in fsck.c::fsck_tree(), we have\nthis for each entry we encounter in a tree:\n\n\t\t...\n\t\thas_zero_pad |= *(char *)desc.buffer == '0';\n\n\t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n\t\t\tif (!S_ISLNK(mode))\n\t\t\t\toidset_insert(&gitmodules_found, oid);\n\t\t\telse\n\t\t\t\tretval += report(options, &item->object,\n\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n\t\t}\n\nCan mode be 160000 or 40000 here?  The code seems to assume that, as\nlong as the thing is named \".gitmodules\", we are looking at a blob,\nso symlinks are worth reporting immediately and all others are worth\ninvestigating later by marking them in the gitmodules_found oidset.\n\nI think it is a good idea to be tighter than necessary by default,\nso rejecting a gitlink \".gitmodules\" unless the user tells us with\nsome configuration knob is a good thing to do, but it probably makes\nsense to have users a way to opt-out of this \"There is .gitmodules\nbut it is not a readable blob\" sanity check.\n\n\n\n[Footnote]\n\n*1* A quick scan of the code there looking for GITMODULES_FILE did\nnot give me a very good feeling---it seems that the code trusts what\nis in the index and in the working tree to be non-hostile a bit too\nmuch.\n"},{"id":"351639","messageId":"20180703183423.GA14371@sigill.intra.peff.net","threadId":"48821","inReplyTo":"xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com","subject":"Re: Checks added for CVE-2018-11235 too restrictive?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-03T18:34:23Z","receivedAt":"2018-07-03T18:34:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 03, 2018 at 10:45:51AM -0700, Junio C Hamano wrote:\n\n> > I'm open to the idea that the new checks are too restrictive (both this\n> > and the gitmodulesParse error we're discussing in [1]). They definitely\n> > outlaw things that _used_ to be OK. And the security benefit is a little\n> > hand-wavy. They're not strictly needed to block the recent\n> > vulnerability[2].  However, they _could_ protect us from future problems\n> > (e.g., an overflow in the config-parsing code, which is not accessible\n> > to attackers outside of .gitmodules). So there is some value in being\n> > restrictive, but it's mostly hypothetical for now.\n> \n> As you said elsewhere, the above example would probably not cause\n> harm to the production use of .gitmodules (as opposed to what fsck\n> checks) as it is not at the top of the working tree, but does the\n> production code do a wrong thing if a directory, a symbolic link or\n> a gitlink appear at \".gitmodules\" at the top level?\n> \n> If so, we probably need to tighten code in submodules.c and\n> elsewhere to ignore such \".gitmodules\" directory etc [*1*].\n\nI tried it when writing my earlier message, and I couldn't convince it\nto do anything too terrible (these both have one true submodule and a\ngitlink .gitmodules):\n\n  $ git status\n  warning: unable to access '/home/peff/tmp/.gitmodules': Is a directory\n  On branch master\n  nothing to commit, working tree clean\n\n  $ git submodule update --init\n  warning: unable to access '/home/peff/tmp/.gitmodules': Is a directory\n  fatal: No url found for submodule path '.gitmodules' in .gitmodules\n\nObviously the setup is broken, but I think most code paths would hit\nthat fopen() failure. Possibly something that reads directly from the\nindex might not check the object type carefully and could read cruft\nfrom a non-blob (though my understanding from past discussions is that\nwe don't actually do direct from-index reading yet).\n\n> And if not, I think it is OK to loosen fsck to exclude \".gitmodules\"\n> that are not regular files, e.g. in fsck.c::fsck_tree(), we have\n> this for each entry we encounter in a tree:\n> \n> \t\t...\n> \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> \n> \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n> \t\t\tif (!S_ISLNK(mode))\n> \t\t\t\toidset_insert(&gitmodules_found, oid);\n> \t\t\telse\n> \t\t\t\tretval += report(options, &item->object,\n> \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n> \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n> \t\t}\n> \n> Can mode be 160000 or 40000 here?  The code seems to assume that, as\n> long as the thing is named \".gitmodules\", we are looking at a blob,\n> so symlinks are worth reporting immediately and all others are worth\n> investigating later by marking them in the gitmodules_found oidset.\n\nRight, that's more or less how the logic works now. Which actually\nbrings up another philosophical issue. We wouldn't want to loosen the\nsymlink check here, since it is serving an actual security purpose.  So\nif we were to loosen the other bits (say, allowing .gitmodules to be a\nnon-blob), we still could not say \"...and there is no regression with\nrespect to all old .gitmodules tree entries\". Some would work, and some.\nwould not. Is it better to be consistent (\"we assume .gitmodules entries\nare well-formed, and do not use that name if you are not\") or to try to\nbe as permissive as possible?\n\n> I think it is a good idea to be tighter than necessary by default,\n> so rejecting a gitlink \".gitmodules\" unless the user tells us with\n> some configuration knob is a good thing to do, but it probably makes\n> sense to have users a way to opt-out of this \"There is .gitmodules\n> but it is not a readable blob\" sanity check.\n\nThat already exists using the fsck \"ignore\" config and skiplist. I think\nthe question is just whether it is an undue burden to have to deal with\nthose. Setting a config option is not too bad, but quite often you are\ncrossing an administrative boundary (convincing a hoster to loosen fsck,\nor convincing users who fetch from you that they need to set a certain\nconfig variable).\n\n> *1* A quick scan of the code there looking for GITMODULES_FILE did\n> not give me a very good feeling---it seems that the code trusts what\n> is in the index and in the working tree to be non-hostile a bit too\n> much.\n\nI won't be surprised if there is a way to cause mischief there. The\nextent of my probing was the two commands I showed above. :)\n\n-Peff\n"},{"id":"351668","messageId":"20180703223030.xds2bfgeuaa35isj@glandium.org","threadId":"48821","inReplyTo":"20180703141518.GA21629@sigill.intra.peff.net","subject":"Re: Checks added for CVE-2018-11235 too restrictive?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2018-07-03T22:30:30Z","receivedAt":"2018-07-03T22:30:38Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jul 03, 2018 at 10:15:19AM -0400, Jeff King wrote:\n> On Tue, Jul 03, 2018 at 04:06:50PM +0900, Mike Hommey wrote:\n> \n> > I had a first shot at that a few months ago, but the format of the\n> > metadata branch made it impossible to push to github, hitting its push\n> > size limit. With some pre-processing work, I was able to push the data\n> > to github, with the intent to come back with a new metadata branch\n> > format that would make the push directly possible.\n> \n> This is sort of tangential to your email here, but I'm curious which\n> limit you hit. Too-big blob, tree, or commit?\n\nToo-big pack, iirc.\n\n> > Fast forward to this week, where I was trying to upload a new metadata\n> > branch, and failed in a rather interesting way: multiple lines looking\n> > like:\n> > \n> > remote: error: object 81eae74b046d284c47e788143bbbcc681cb53418: gitmodulesMissing: unable to read .gitmodules blob\n> > \n> > which, apart from the fact that they have some formatting issue, appear\n> > to be new from the fixes for CVE-2018-11235.\n> \n> Also tangential to your main point (I promise I'll get to it ;) ), but\n> what formatting issue do you mean? Are you referring to\n> \"gitmodulesMissing\"? That's a machine-readable tag which means you can\n> set \"fsck.gitmodulesMissing\" to \"ignore\".\n\nOh, I was reading it as something finishing with \"gitmodules\", and\nanother message starting with \"Missing\". The fact the error is so\nlong and on one line made me think that, I guess.\n\n> > I can see what those fixes are trying to prevent, but they seem to be\n> > overly restrictive, at least in the context of transfer.fsckObjects.\n> > \n> > The core problem is that the mercurial repository has some .gitmodules\n> > files in some subdirectories. As a consequence, the git objects storing\n> > the info for those mercurial files contain trees with .gitmodules files\n> > with a commit ref, and that's what the remote is complaining about.\n> > \n> > (Surpringly, it doesn't hit the \"non-blob found at .gitmodules\" case\n> > first, which is weird).\n> \n> The reason it doesn't hit the \"non-blob\" case is that we are trying to\n> analyze the object itself. So we don't pay any attention to the mode in\n> the containing tree, but instead literally look for 81eae74b0. If it\n> were a non-blob we'd complain then, but in fact we don't have it at all\n> (which is otherwise OK because it's a gitlink).\n> \n> > A small testcase to reproduce looks like this:\n> > \n> > $ git init bar; cd bar\n> > $ git fast-import <<EOF\n> > commit refs/heads/bar\n> > committer Bar <bar@bar> 0 +0000\n> > data 0\n> >                                                                  \n> > M 160000 81eae74b046d284c47e788143bbbcc681cb53418 bar/.gitmodules\n> >                                                                  \n> > EOF\n> >\n> > [...]\n> > \n> > Would it be reasonable to make the transfer.fsckObject checks ignore\n> > non-blob .gitmodules?\n> \n> I'm open to the idea that the new checks are too restrictive (both this\n> and the gitmodulesParse error we're discussing in [1]). They definitely\n> outlaw things that _used_ to be OK. And the security benefit is a little\n> hand-wavy. They're not strictly needed to block the recent\n> vulnerability[2].  However, they _could_ protect us from future problems\n> (e.g., an overflow in the config-parsing code, which is not accessible\n> to attackers outside of .gitmodules). So there is some value in being\n> restrictive, but it's mostly hypothetical for now.\n> \n> So I think we could simply loosen these cases without immediate effect.\n> That said, I'm not sure that having a gitlink .gitmodules is sane in the\n> first place. Attempting to \"git submodule init\" there is going to cause\n> errors. Well, sort of -- your example actually includes it in a\n> subdirectory of the repository, so technically we wouldn't use it for\n> real submodules. That fudging (finding .gitmodules anywhere instead of\n> just at the root) was a conscious decision to reduce the complexity and\n> cost of the check.\n> \n> It sounds like in this case you don't have existing history that does\n> this with .gitmodules, but are rather looking into designing a new\n> feature that stuffs data into git trees. I'd be interested to hear a bit\n> more about that feature to see if there are other alternatives.\n\nYes, in fact, this history is not even meant to be checked out. It's\ninternal stuff, stuck in a ref in a non-traditional ref namespace (that\nis, it's not under refs/heads, refs/tags, etc., it's under\nrefs/cinnabar)\n\nI'd rather avoid using entirely new features that wouldn't work with\nwidely used git versions. I can actually work around the current problem\nby adding a prefix to all file names. That doesn't sound great, but is\nprobably good enough. In fact, it might even avoid future similar problems.\n\nThat being said, I'm not even sure this particular use case is worth a\nnew feature. I'm not storing random stuff as gitlinks, I'm storing\nsha1s. Well, maybe a mode that makes the distinction between \"git oid\"\nand \"external oid\" might make things clearer for git itself, especially\nfor fsck.\n\nMike\n"},{"id":"351681","messageId":"20180704081857.kgoiniydk44ooq6v@glandium.org","threadId":"48821","inReplyTo":"20180703223030.xds2bfgeuaa35isj@glandium.org","subject":"Re: Checks added for CVE-2018-11235 too restrictive?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2018-07-04T08:18:57Z","receivedAt":"2018-07-04T08:19:09Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Jul 04, 2018 at 07:30:30AM +0900, Mike Hommey wrote:\n> That being said, I'm not even sure this particular use case is worth a\n> new feature. I'm not storing random stuff as gitlinks, I'm storing\n> sha1s. Well, maybe a mode that makes the distinction between \"git oid\"\n> and \"external oid\" might make things clearer for git itself, especially\n> for fsck.\n\nActually, I'm also abusing the lower bits of the gitlink mode, to\ndifferentiate between regular files, executable files, and symlinks.\n\nMike\n"}]}