Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jan 2, 2017, 18:06 UTC
Message-ID: <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
In-Reply-To: <20170101055947.7b5jxih3wlprqcil@sigill.intra.peff.net>

On 01/01/2017 06:59 AM, Jeff King wrote:

Show 21 quoted lines

> On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:
> 
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> "refname" has already been checked by check_refname_format(), so it
>>> cannot have consecutive slashes.
>>
>> In the endgame state, this has two callers.  Both use what came in
>> the transaction->updates[] array.  Presumably "has already been
>> checked by check_refname_format()" says that whoever created entries
>> in that array must have called the function, but it would be helpful
>> to be more explicit here.
> 
> Hmm, yeah. This is called when we are deleting a ref, and I thought we
> explicitly _didn't_ do check_refname_format() when deleting, so that
> funny-named refs could be deleted. It's only is_refname_safe() that we
> must pass.
> 
> So I have no idea if that's a problem in the code or not, but it is at
> least not immediately obvious who is responsible for calling
> check_refname_format() here.

My assumption was that only valid reference names should ever be allowed to be inserted into a `ref_transaction` entry. But Peff is right that sometimes the reference name is checked by `refname_is_safe()` rather than `check_refname_format()`. Let's audit this more carefully...

* `ref_transaction_add_update()` relies on its caller doing the check
  (this fact is documented). Its callers are:
  * `ref_transaction_update()` (the usual codepath), which checks the
    reference itself using either check_refname_format() or
    refname_is_safe() depending on what kind of update it is.
  * `split_head_update()` passes the literal string "HEAD".
  * `split_symref_update()` picks apart reference updates that go
    through existing symbolic references. As such I don't think they
    are an attack surface. It doesn't do any checking itself (here
    we're talking about its `referent` argument). It has only one
    caller:
    * `lock_ref_for_update()`, which gets `referent` from:
      * `files_read_raw_ref()`, which gets the value either:
        * by reading a filesystem-level symlink's contents and
          checking it with `check_refname_format()`, or
        * reading a symref from the filesystem. In this case, *the
          value is not checked*.

Obviously this chain of custody is disconcertingly long and complicated. And the gap for symrefs should probably be fixed, even though they are hopefully trustworthy.

`refname_is_safe()` checks that its argument is either UPPER_CASE or looks like a plausible filename under "refs/". Contrary to its docstring, it *does not* accept strings that contain successive slashes or "." or ".." components. It was made stricter in

    e40f355 "refname_is_safe(): insist that the refname already be
normalized", 2016-04-27

, but the docstring wasn't updated at that time. I'll fix it.

I think the best thing to do is to drop this patch from the patch series, and address these checks in a separate series. (There are more known problems in this area, for example that the checks in `check_refname_format()` are not a strict superset of the checks in `refname_is_safe()`.)

Michael