Re: Checks added for CVE-2018-11235 too restrictive?
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 3, 2018, 17:45 UTC
- Message-ID
- <xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180703141518.GA21629@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 25 quoted lines
>> A small testcase to reproduce looks like this: >> >> $ git init bar; cd bar >> $ git fast-import <<EOF >> commit refs/heads/bar >> committer Bar <bar@bar> 0 +0000 >> data 0 >> >> M 160000 81eae74b046d284c47e788143bbbcc681cb53418 bar/.gitmodules >> >> EOF >> >> [...] >> >> Would it be reasonable to make the transfer.fsckObject checks ignore >> non-blob .gitmodules? > > I'm open to the idea that the new checks are too restrictive (both this > and the gitmodulesParse error we're discussing in [1]). They definitely > outlaw things that _used_ to be OK. And the security benefit is a little > hand-wavy. They're not strictly needed to block the recent > vulnerability[2]. However, they _could_ protect us from future problems > (e.g., an overflow in the config-parsing code, which is not accessible > to attackers outside of .gitmodules). So there is some value in being > restrictive, but it's mostly hypothetical for now.
As you said elsewhere, the above example would probably not cause harm to the production use of .gitmodules (as opposed to what fsck checks) as it is not at the top of the working tree, but does the production code do a wrong thing if a directory, a symbolic link or a gitlink appear at ".gitmodules" at the top level?
If so, we probably need to tighten code in submodules.c and elsewhere to ignore such ".gitmodules" directory etc [*1*].
And if not, I think it is OK to loosen fsck to exclude ".gitmodules" that are not regular files, e.g. in fsck.c::fsck_tree(), we have this for each entry we encounter in a tree:
... has_zero_pad |= *(char *)desc.buffer == '0';
if (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {
if (!S_ISLNK(mode))
oidset_insert(&gitmodules_found, oid);
else
retval += report(options, &item->object,
FSCK_MSG_GITMODULES_SYMLINK,
".gitmodules is a symbolic link");
}Can mode be 160000 or 40000 here? The code seems to assume that, as long as the thing is named ".gitmodules", we are looking at a blob, so symlinks are worth reporting immediately and all others are worth investigating later by marking them in the gitmodules_found oidset.
I think it is a good idea to be tighter than necessary by default, so rejecting a gitlink ".gitmodules" unless the user tells us with some configuration knob is a good thing to do, but it probably makes sense to have users a way to opt-out of this "There is .gitmodules but it is not a readable blob" sanity check.
[Footnote]
*1* A quick scan of the code there looking for GITMODULES_FILE did not give me a very good feeling---it seems that the code trusts what is in the index and in the working tree to be non-hostile a bit too much.