[RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Christian Couder]

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
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index e3adc78ebf..c54f0000fb 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -13,7 +13,6 @@ enum list_objects_filter_choice {
 	LOFC_BLOB_LIMIT,
 	LOFC_TREE_DEPTH,
 	LOFC_SPARSE_OID,
-	LOFC_SPARSE_PATH,
 	LOFC__COUNT /* must be last */
 };
 
@@ -44,7 +43,6 @@ struct list_objects_filter_options {
 	 * choice.
 	 */
 	struct object_id *sparse_oid_value;
-	char *sparse_path_value;
 	unsigned long blob_limit_value;
 	unsigned long tree_exclude_depth;
 };
diff --git a/list-objects-filter.c b/list-objects-filter.c
index ee449de3f7..53f90442c5 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -478,27 +478,6 @@ static void *filter_sparse_oid__init(
 	return d;
 }
 
-static void *filter_sparse_path__init(
-	struct oidset *omitted,
-	struct list_objects_filter_options *filter_options,
-	filter_object_fn *filter_fn,
-	filter_free_fn *filter_free_fn)
-{
-	struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
-	d->omits = omitted;
-	if (add_excludes_from_file_to_list(filter_options->sparse_path_value,
-					   NULL, 0, &d->el, NULL) < 0)
-		die("could not load filter specification");
-
-	ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
-	d->array_frame[d->nr].defval = 0; /* default to include */
-	d->array_frame[d->nr].child_prov_omit = 0;
-
-	*filter_fn = filter_sparse;
-	*filter_free_fn = filter_sparse_free;
-	return d;
-}
-
 typedef void *(*filter_init_fn)(
 	struct oidset *omitted,
 	struct list_objects_filter_options *filter_options,
@@ -514,7 +493,6 @@ static filter_init_fn s_filters[] = {
 	filter_blobs_limit__init,
 	filter_trees_depth__init,
 	filter_sparse_oid__init,
-	filter_sparse_path__init,
 };
 
 void *list_objects_filter__init(
-- 
2.22.0.rc1

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Ævar Arnfjörð Bjarmason]

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.

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Christian Couder]

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.

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Jeff Hostetler]

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

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Jeff King]

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

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Jeff Hostetler]

On 5/28/2019 2:13 AM, Jeff King wrote:
> 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
> 

yeah, let's simplify things and remove it completely for now.

Jeff

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Matthew DeVore]

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.

The alternative is to hide sparse:path= support behind a flag which is disabled
by default, but I don't recommend doing that just to have an excuse to include
the URL-encoding logic.

Thank you for cleaning up.

>  	} 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.]"

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

As the comment states, don't forget to update git-completion.bash :)

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Christian Couder]

On Fri, May 24, 2019 at 7:08 PM Matthew DeVore <matvore@comcast.net> wrote:
>
> 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.

Thanks for telling about it.

> >       } 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.]"

Yeah, that seems better to me.

> >        * Please update _git_fetch() in git-completion.bash when you
>
> As the comment states, don't forget to update git-completion.bash :)

Ok, I will resend soon with better wording and an update to
"git-completion.bash".

Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

[Jeff Hostetler]

On 5/24/2019 8:03 AM, 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.
> 
> 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
[...]

We should update git-completion.bash to remove this option.

Jeff