git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Mike Hommey <mh@glandium.org>,
	git@vger.kernel.org, Ramsay Jones <ramsay@ramsayjones.plus.com>
Subject: Re: Checks added for CVE-2018-11235 too restrictive?
Date: Tue, 03 Jul 2018 10:45:51 -0700	[thread overview]
Message-ID: <xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180703141518.GA21629@sigill.intra.peff.net> (Jeff King's message of "Tue, 3 Jul 2018 10:15:19 -0400")

Jeff King <peff@peff.net> writes:

>> 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.

  reply	other threads:[~2018-07-03 17:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-03  7:06 Checks added for CVE-2018-11235 too restrictive? Mike Hommey
2018-07-03 14:15 ` Jeff King
2018-07-03 17:45   ` Junio C Hamano [this message]
2018-07-03 18:34     ` Jeff King
2018-07-03 22:30   ` Mike Hommey
2018-07-04  8:18     ` Mike Hommey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=mh@glandium.org \
    --cc=peff@peff.net \
    --cc=ramsay@ramsayjones.plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).