| Message ID | 20190524120318.4851-1-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] list-objects-filter: disable 'sparse:path' filters | expand |
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: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 :)
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
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
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".
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
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
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(