RFC patchlist-objects-filter: disable 'sparse:path' filters

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

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

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

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

> On Fri, May 24, 2019 at 02:46:06PM -0400, Jeff Hostetler wrote:
> 
>> 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.
> 
> Yeah, it had been my plan to limit it only via upload-pack, under the
> assumption that somebody probably wanted it on the local side. If you,
> who added it, are OK with removing it completely, that gives me more
> confidence that nobody is using it (coupled with the general
> experimental nature of partial clones at this point). But I'm still a
> little worried somebody may have found a use for it in the meantime.
> 
> -Peff
> 

>  	} else if (skip_prefix(arg, "sparse:path=", &v0)) {
> -		filter_options->choice = LOFC_SPARSE_PATH;
> -		filter_options->sparse_path_value = strdup(v0);
> -		return 0;
> +		if (errbuf) {
> +			strbuf_addstr(
> +				errbuf,
> +				_("sparse:path filters are now disabled"));

> +		}
> +		return 1;
>  	}
>  	/*
>  	 * Please update _git_fetch() in git-completion.bash when you

>
> On Fri, May 24, 2019 at 02:03:18PM +0200, Christian Couder wrote:
> > For now though, let's just disable 'sparse:path' filters.
>
> This is probably the right thing to do. I did jump through a lot of hoops to
> support escaping sub-filters in my pending filter combination patchset, since
> sparse spec path names can have arbitrary characters. After this patch we only
> support a handful of characters in filterspecs, so a lot of that escaping logic
> can be dropped, at least for now. Anyway, this is not a complaint, just an
> observation.

> >       } else if (skip_prefix(arg, "sparse:path=", &v0)) {
> > -             filter_options->choice = LOFC_SPARSE_PATH;
> > -             filter_options->sparse_path_value = strdup(v0);
> > -             return 0;
> > +             if (errbuf) {
> > +                     strbuf_addstr(
> > +                             errbuf,
> > +                             _("sparse:path filters are now disabled"));
>
> This wording may leave room for misunderstanding, since it sounds a little like
> the filter can be re-enabled somehow. Maybe you can say "sparse:path filters
> support has been dropped [optional: 'for security reasons' etc.]"

> 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.
> 
> If someone is interested in using a sparse file that is not in the
> repository as a filter, then at the minimum a config option, such
> as "uploadpack.sparsePathFilter", should be implemented first to
> restrict the directory from which the files specified by
> 'sparse:path' can be read.
> 
> For now though, let's just disable 'sparse:path' filters.
> ---
>   list-objects-filter-options.c |  9 ++++++---
>   list-objects-filter-options.h |  2 --
>   list-objects-filter.c         | 22 ----------------------
>   3 files changed, 6 insertions(+), 27 deletions(-)
> 
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index c0036f7378..007c104b93 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -78,9 +78,12 @@ static int gently_parse_list_objects_filter(
>   		return 0;
>   
>   	} else if (skip_prefix(arg, "sparse:path=", &v0)) {
> -		filter_options->choice = LOFC_SPARSE_PATH;
> -		filter_options->sparse_path_value = strdup(v0);
> -		return 0;
> +		if (errbuf) {
> +			strbuf_addstr(
> +				errbuf,
> +				_("sparse:path filters are now disabled"));
> +		}
> +		return 1;
>   	}
>   	/*
>   	 * Please update _git_fetch() in git-completion.bash when you