git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
@ 2019-05-24 12:03 Christian Couder
  2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Christian Couder @ 2019-05-24 12:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Jeff Hostetler, Jeff Hostetler,
	Jonathan Tan, Matthew DeVore, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 12:03 [RFC PATCH] list-objects-filter: disable 'sparse:path' filters Christian Couder
@ 2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
  2019-05-24 12:39   ` Christian Couder
  2019-05-24 17:07 ` Matthew DeVore
  2019-05-24 19:33 ` Jeff Hostetler
  2 siblings, 1 reply; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-24 12:21 UTC (permalink / raw)
  To: Christian Couder
  Cc: git, Junio C Hamano, Jeff King, Jeff Hostetler, Jeff Hostetler,
	Jonathan Tan, Matthew DeVore, Christian Couder


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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
@ 2019-05-24 12:39   ` Christian Couder
  2019-05-24 18:46     ` Jeff Hostetler
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Couder @ 2019-05-24 12:39 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Jeff Hostetler, Jeff Hostetler,
	Jonathan Tan, Matthew DeVore, 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 12:03 [RFC PATCH] list-objects-filter: disable 'sparse:path' filters Christian Couder
  2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
@ 2019-05-24 17:07 ` Matthew DeVore
  2019-05-24 19:43   ` Christian Couder
  2019-05-24 19:33 ` Jeff Hostetler
  2 siblings, 1 reply; 9+ messages in thread
From: Matthew DeVore @ 2019-05-24 17:07 UTC (permalink / raw)
  To: Christian Couder
  Cc: git, Junio C Hamano, Jeff King, Jeff Hostetler, Jeff Hostetler,
	Jonathan Tan, Matthew DeVore, Christian Couder

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 :)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 12:39   ` Christian Couder
@ 2019-05-24 18:46     ` Jeff Hostetler
  2019-05-28  6:13       ` Jeff King
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Hostetler @ 2019-05-24 18:46 UTC (permalink / raw)
  To: Christian Couder, Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Jeff Hostetler, Jonathan Tan,
	Matthew DeVore, Christian Couder



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 12:03 [RFC PATCH] list-objects-filter: disable 'sparse:path' filters Christian Couder
  2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
  2019-05-24 17:07 ` Matthew DeVore
@ 2019-05-24 19:33 ` Jeff Hostetler
  2 siblings, 0 replies; 9+ messages in thread
From: Jeff Hostetler @ 2019-05-24 19:33 UTC (permalink / raw)
  To: Christian Couder, git
  Cc: Junio C Hamano, Jeff King, Jeff Hostetler, Jonathan Tan,
	Matthew DeVore, Christian Couder



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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 17:07 ` Matthew DeVore
@ 2019-05-24 19:43   ` Christian Couder
  0 siblings, 0 replies; 9+ messages in thread
From: Christian Couder @ 2019-05-24 19:43 UTC (permalink / raw)
  To: Matthew DeVore
  Cc: git, Junio C Hamano, Jeff King, Jeff Hostetler, Jeff Hostetler,
	Jonathan Tan, Matthew DeVore, 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".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-24 18:46     ` Jeff Hostetler
@ 2019-05-28  6:13       ` Jeff King
  2019-05-28 13:29         ` Jeff Hostetler
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2019-05-28  6:13 UTC (permalink / raw)
  To: Jeff Hostetler
  Cc: Christian Couder, Ævar Arnfjörð Bjarmason, git,
	Junio C Hamano, Jeff Hostetler, Jonathan Tan, Matthew DeVore,
	Christian Couder

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters
  2019-05-28  6:13       ` Jeff King
@ 2019-05-28 13:29         ` Jeff Hostetler
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Hostetler @ 2019-05-28 13:29 UTC (permalink / raw)
  To: Jeff King
  Cc: Christian Couder, Ævar Arnfjörð Bjarmason, git,
	Junio C Hamano, Jeff Hostetler, Jonathan Tan, Matthew DeVore,
	Christian Couder



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-05-28 13:29 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 12:03 [RFC PATCH] list-objects-filter: disable 'sparse:path' filters Christian Couder
2019-05-24 12:21 ` Ævar Arnfjörð Bjarmason
2019-05-24 12:39   ` Christian Couder
2019-05-24 18:46     ` Jeff Hostetler
2019-05-28  6:13       ` Jeff King
2019-05-28 13:29         ` Jeff Hostetler
2019-05-24 17:07 ` Matthew DeVore
2019-05-24 19:43   ` Christian Couder
2019-05-24 19:33 ` Jeff Hostetler

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git