From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 24 May 2019 18:46:06 GMT
Subject: Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
Message-ID: <83f6b1f6-2aba-f81c-03d2-69815e8e6cfb@jeffhostetler.com>
In-Reply-To: <CAP8UFD2HJ2D9Sndud5Wu7Vyz9Jj-mVkpE4oowd3VaXoV3nugcw@mail.gmail.com>



On 5/24/2019 8:39 AM, Christian Couder wrote:
> On Fri, May 24, 2019 at 2:21 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Fri, May 24 2019, Christian Couder wrote:
>>
>>> If someone wants to use as a filter a sparse file that is in the
>>> repository, something like "--filter=sparse:oid=<ref>:<path>"
>>> already works.
>>>
>>> So 'sparse:path' is only interesting if the sparse file is not in
>>> the repository. In this case though the current implementation has
>>> a big security issue, as it makes it possible to ask the server to
>>> read any file, like for example /etc/password, and to explore the
>>> filesystem, as well as individual lines of files.
>>
>> Removing this is a good idea.
>>
>> Just to clarify, what was the attack surface in practice? We pass this
>> to add_excludes_from_file_to_list(), are there cases where it'll spew
>> out parse errors/warnings that allow you to extract content from such a
>> file?
> 
> Peff provided an example script in:
> 
> https://public-inbox.org/git/20181108050755.GA32158@sigill.intra.peff.net/
> 
>> Or does it amount to a DoS vector by pointing to some huge (binary?)
>> file on-disk, or a security issue where you could via the error or
>> timings discover whether the file exists or not, something else?
>>
>> I wonder if server operators need to be paranoid about the DoS from the
>> issue with <oid>:<path> noted int/perf/p0100-globbing.sh which this is
>> presumably vulnerable to, i.e. someone with repository write access
>> uploading pathological patterns. That might be particularly annoying for
>> e.g. GitHub where the fork network's object storage is shared.
> 
> In general servers should limit the git processes they launch, but
> yeah it might be interesting to look at that too.
> 

My original thoughts were that we could limit the sparse:path to
local use and disallow it over the wire to the server, but that
distinction is probably not worth the bother.  Removing it completely
is fine.

Thanks,
Jeff