{"thread":{"id":"51167","subject":"[RFC PATCH] list-objects-filter: disable 'sparse:path' filters","startedAt":"2019-05-24T12:03:37Z","lastAt":"2019-05-28T13:29:53Z","messageCount":9,"participants":["Christian Couder","Ævar Arnfjörð Bjarmason","Matthew DeVore","Jeff Hostetler","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"376182","messageId":"20190524120318.4851-1-chriscool@tuxfamily.org","threadId":"51167","inReplyTo":null,"subject":"[RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-24T12:03:18Z","receivedAt":"2019-05-24T12:03:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"If someone wants to use as a filter a sparse file that is in the\nrepository, something like \"--filter=sparse:oid=:\"\nalready works.\n\nSo 'sparse:path' is only interesting if the sparse file is not in\nthe repository. In this case though the current implementation has\na big security issue, as it makes it possible to ask the server to\nread any file, like for example /etc/password, and to explore the\nfilesystem, as well as individual lines of files.\n\nIf someone is interested in using a sparse file that is not in the\nrepository as a filter, then at the minimum a config option, such\nas \"uploadpack.sparsePathFilter\", should be implemented first to\nrestrict the directory from which the files specified by\n'sparse:path' can be read.\n\nFor now though, let's just disable 'sparse:path' filters.\n---\n list-objects-filter-options.c | 9 ++++++---\n list-objects-filter-options.h | 2 --\n list-objects-filter.c | 22 ----------------------\n 3 files changed, 6 insertions(+), 27 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex c0036f7378..007c104b93 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -78,9 +78,12 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n-\t\tfilter_options->choice = LOFC_SPARSE_PATH;\n-\t\tfilter_options->sparse_path_value = strdup(v0);\n-\t\treturn 0;\n+\t\tif (errbuf) {\n+\t\t\tstrbuf_addstr(\n+\t\t\t\terrbuf,\n+\t\t\t\t_(\"sparse:path filters are now disabled\"));\n+\t\t}\n+\t\treturn 1;\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..c54f0000fb 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -13,7 +13,6 @@ enum list_objects_filter_choice {\n \tLOFC_BLOB_LIMIT,\n \tLOFC_TREE_DEPTH,\n \tLOFC_SPARSE_OID,\n-\tLOFC_SPARSE_PATH,\n \tLOFC__COUNT /* must be last */\n };\n \n@@ -44,7 +43,6 @@ struct list_objects_filter_options {\n \t * choice.\n \t */\n \tstruct object_id *sparse_oid_value;\n-\tchar *sparse_path_value;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..53f90442c5 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -478,27 +478,6 @@ static void *filter_sparse_oid__init(\n \treturn d;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n-{\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n-\t\t\t\t\t NULL, 0, &d->el, NULL) < 0)\n-\t\tdie(\"could not load filter specification\");\n-\n-\tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n-\td->array_frame[d->nr].defval = 0; /* default to include */\n-\td->array_frame[d->nr].child_prov_omit = 0;\n-\n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n-}\n-\n typedef void *(*filter_init_fn)(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n@@ -514,7 +493,6 @@ static filter_init_fn s_filters[] = {\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n-\tfilter_sparse_path__init,\n };\n \n void *list_objects_filter__init(\n-- \n2.22.0.rc1\n\n"},{"id":"376183","messageId":"87ef4of3g6.fsf@evledraar.gmail.com","threadId":"51167","inReplyTo":"20190524120318.4851-1-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-24T12:21:29Z","receivedAt":"2019-05-24T12:21:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 24 2019, Christian Couder wrote:\n\n> If someone wants to use as a filter a sparse file that is in the\n> repository, something like \"--filter=sparse:oid=:\"\n> already works.\n>\n> So 'sparse:path' is only interesting if the sparse file is not in\n> the repository. In this case though the current implementation has\n> a big security issue, as it makes it possible to ask the server to\n> read any file, like for example /etc/password, and to explore the\n> filesystem, as well as individual lines of files.\n\nRemoving this is a good idea.\n\nJust to clarify, what was the attack surface in practice? We pass this\nto add_excludes_from_file_to_list(), are there cases where it'll spew\nout parse errors/warnings that allow you to extract content from such a\nfile?\n\nOr does it amount to a DoS vector by pointing to some huge (binary?)\nfile on-disk, or a security issue where you could via the error or\ntimings discover whether the file exists or not, something else?\n\nI wonder if server operators need to be paranoid about the DoS from the\nissue with : noted int/perf/p0100-globbing.sh which this is\npresumably vulnerable to, i.e. someone with repository write access\nuploading pathological patterns. That might be particularly annoying for\ne.g. GitHub where the fork network's object storage is shared.\n"},{"id":"376188","messageId":"CAP8UFD2HJ2D9Sndud5Wu7Vyz9Jj-mVkpE4oowd3VaXoV3nugcw@mail.gmail.com","threadId":"51167","inReplyTo":"87ef4of3g6.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-24T12:39:19Z","receivedAt":"2019-05-24T12:39:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 24, 2019 at 2:21 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, May 24 2019, Christian Couder wrote:\n>\n> > If someone wants to use as a filter a sparse file that is in the\n> > repository, something like \"--filter=sparse:oid=:\"\n> > already works.\n> >\n> > So 'sparse:path' is only interesting if the sparse file is not in\n> > the repository. In this case though the current implementation has\n> > a big security issue, as it makes it possible to ask the server to\n> > read any file, like for example /etc/password, and to explore the\n> > filesystem, as well as individual lines of files.\n>\n> Removing this is a good idea.\n>\n> Just to clarify, what was the attack surface in practice? We pass this\n> to add_excludes_from_file_to_list(), are there cases where it'll spew\n> out parse errors/warnings that allow you to extract content from such a\n> file?\n\nPeff provided an example script in:\n\nhttps://public-inbox.org/git/20181108050755.GA32158@sigill.intra.peff.net/\n\n> Or does it amount to a DoS vector by pointing to some huge (binary?)\n> file on-disk, or a security issue where you could via the error or\n> timings discover whether the file exists or not, something else?\n>\n> I wonder if server operators need to be paranoid about the DoS from the\n> issue with : noted int/perf/p0100-globbing.sh which this is\n> presumably vulnerable to, i.e. someone with repository write access\n> uploading pathological patterns. That might be particularly annoying for\n> e.g. GitHub where the fork network's object storage is shared.\n\nIn general servers should limit the git processes they launch, but\nyeah it might be interesting to look at that too.\n"},{"id":"376193","messageId":"20190524170758.GB7160@comcast.net","threadId":"51167","inReplyTo":"20190524120318.4851-1-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-24T17:07:58Z","receivedAt":"2019-05-24T17:08:03Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Fri, May 24, 2019 at 02:03:18PM +0200, Christian Couder wrote:\n> For now though, let's just disable 'sparse:path' filters.\n\nThis is probably the right thing to do. I did jump through a lot of hoops to\nsupport escaping sub-filters in my pending filter combination patchset, since\nsparse spec path names can have arbitrary characters. After this patch we only\nsupport a handful of characters in filterspecs, so a lot of that escaping logic\ncan be dropped, at least for now. Anyway, this is not a complaint, just an\nobservation.\n\nThe alternative is to hide sparse:path= support behind a flag which is disabled\nby default, but I don't recommend doing that just to have an excuse to include\nthe URL-encoding logic.\n\nThank you for cleaning up.\n\n> \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n> -\t\tfilter_options->choice = LOFC_SPARSE_PATH;\n> -\t\tfilter_options->sparse_path_value = strdup(v0);\n> -\t\treturn 0;\n> +\t\tif (errbuf) {\n> +\t\t\tstrbuf_addstr(\n> +\t\t\t\terrbuf,\n> +\t\t\t\t_(\"sparse:path filters are now disabled\"));\n\nThis wording may leave room for misunderstanding, since it sounds a little like\nthe filter can be re-enabled somehow. Maybe you can say \"sparse:path filters\nsupport has been dropped [optional: 'for security reasons' etc.]\"\n\n> +\t\t}\n> +\t\treturn 1;\n> \t}\n> \t/*\n> \t * Please update _git_fetch() in git-completion.bash when you\n\nAs the comment states, don't forget to update git-completion.bash :)\n"},{"id":"376199","messageId":"83f6b1f6-2aba-f81c-03d2-69815e8e6cfb@jeffhostetler.com","threadId":"51167","inReplyTo":"CAP8UFD2HJ2D9Sndud5Wu7Vyz9Jj-mVkpE4oowd3VaXoV3nugcw@mail.gmail.com","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-24T18:46:06Z","receivedAt":"2019-05-24T18:46:10Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/24/2019 8:39 AM, Christian Couder wrote:\n> On Fri, May 24, 2019 at 2:21 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Fri, May 24 2019, Christian Couder wrote:\n>>\n>>> If someone wants to use as a filter a sparse file that is in the\n>>> repository, something like \"--filter=sparse:oid=:\"\n>>> already works.\n>>>\n>>> So 'sparse:path' is only interesting if the sparse file is not in\n>>> the repository. In this case though the current implementation has\n>>> a big security issue, as it makes it possible to ask the server to\n>>> read any file, like for example /etc/password, and to explore the\n>>> filesystem, as well as individual lines of files.\n>>\n>> Removing this is a good idea.\n>>\n>> Just to clarify, what was the attack surface in practice? We pass this\n>> to add_excludes_from_file_to_list(), are there cases where it'll spew\n>> out parse errors/warnings that allow you to extract content from such a\n>> file?\n> \n> Peff provided an example script in:\n> \n> https://public-inbox.org/git/20181108050755.GA32158@sigill.intra.peff.net/\n> \n>> Or does it amount to a DoS vector by pointing to some huge (binary?)\n>> file on-disk, or a security issue where you could via the error or\n>> timings discover whether the file exists or not, something else?\n>>\n>> I wonder if server operators need to be paranoid about the DoS from the\n>> issue with : noted int/perf/p0100-globbing.sh which this is\n>> presumably vulnerable to, i.e. someone with repository write access\n>> uploading pathological patterns. That might be particularly annoying for\n>> e.g. GitHub where the fork network's object storage is shared.\n> \n> In general servers should limit the git processes they launch, but\n> yeah it might be interesting to look at that too.\n> \n\nMy original thoughts were that we could limit the sparse:path to\nlocal use and disallow it over the wire to the server, but that\ndistinction is probably not worth the bother. Removing it completely\nis fine.\n\nThanks,\nJeff\n"},{"id":"376202","messageId":"3ac687d6-d534-13c8-db9a-f481403634d1@jeffhostetler.com","threadId":"51167","inReplyTo":"20190524120318.4851-1-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-24T19:33:57Z","receivedAt":"2019-05-24T19:34:00Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/24/2019 8:03 AM, Christian Couder wrote:\n> If someone wants to use as a filter a sparse file that is in the\n> repository, something like \"--filter=sparse:oid=:\"\n> already works.\n> \n> So 'sparse:path' is only interesting if the sparse file is not in\n> the repository. In this case though the current implementation has\n> a big security issue, as it makes it possible to ask the server to\n> read any file, like for example /etc/password, and to explore the\n> filesystem, as well as individual lines of files.\n> \n> If someone is interested in using a sparse file that is not in the\n> repository as a filter, then at the minimum a config option, such\n> as \"uploadpack.sparsePathFilter\", should be implemented first to\n> restrict the directory from which the files specified by\n> 'sparse:path' can be read.\n> \n> For now though, let's just disable 'sparse:path' filters.\n> ---\n> list-objects-filter-options.c | 9 ++++++---\n> list-objects-filter-options.h | 2 --\n> list-objects-filter.c | 22 ----------------------\n> 3 files changed, 6 insertions(+), 27 deletions(-)\n> \n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index c0036f7378..007c104b93 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -78,9 +78,12 @@ static int gently_parse_list_objects_filter(\n> \t\treturn 0;\n> \n> \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n> -\t\tfilter_options->choice = LOFC_SPARSE_PATH;\n> -\t\tfilter_options->sparse_path_value = strdup(v0);\n> -\t\treturn 0;\n> +\t\tif (errbuf) {\n> +\t\t\tstrbuf_addstr(\n> +\t\t\t\terrbuf,\n> +\t\t\t\t_(\"sparse:path filters are now disabled\"));\n> +\t\t}\n> +\t\treturn 1;\n> \t}\n> \t/*\n> \t * Please update _git_fetch() in git-completion.bash when you\n[...]\n\nWe should update git-completion.bash to remove this option.\n\nJeff\n\n"},{"id":"376203","messageId":"CAP8UFD1uojiT+PSAWTq9+hvr5Nzc707=4+Wfmm=pwJbzN1es-w@mail.gmail.com","threadId":"51167","inReplyTo":"20190524170758.GB7160@comcast.net","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-24T19:43:18Z","receivedAt":"2019-05-24T19:43:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 24, 2019 at 7:08 PM Matthew DeVore wrote:\n>\n> On Fri, May 24, 2019 at 02:03:18PM +0200, Christian Couder wrote:\n> > For now though, let's just disable 'sparse:path' filters.\n>\n> This is probably the right thing to do. I did jump through a lot of hoops to\n> support escaping sub-filters in my pending filter combination patchset, since\n> sparse spec path names can have arbitrary characters. After this patch we only\n> support a handful of characters in filterspecs, so a lot of that escaping logic\n> can be dropped, at least for now. Anyway, this is not a complaint, just an\n> observation.\n\nThanks for telling about it.\n\n> > } else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n> > - filter_options->choice = LOFC_SPARSE_PATH;\n> > - filter_options->sparse_path_value = strdup(v0);\n> > - return 0;\n> > + if (errbuf) {\n> > + strbuf_addstr(\n> > + errbuf,\n> > + _(\"sparse:path filters are now disabled\"));\n>\n> This wording may leave room for misunderstanding, since it sounds a little like\n> the filter can be re-enabled somehow. Maybe you can say \"sparse:path filters\n> support has been dropped [optional: 'for security reasons' etc.]\"\n\nYeah, that seems better to me.\n\n> > * Please update _git_fetch() in git-completion.bash when you\n>\n> As the comment states, don't forget to update git-completion.bash :)\n\nOk, I will resend soon with better wording and an update to\n\"git-completion.bash\".\n"},{"id":"376271","messageId":"20190528061317.GD7946@sigill.intra.peff.net","threadId":"51167","inReplyTo":"83f6b1f6-2aba-f81c-03d2-69815e8e6cfb@jeffhostetler.com","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T06:13:18Z","receivedAt":"2019-05-28T06:13:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 24, 2019 at 02:46:06PM -0400, Jeff Hostetler wrote:\n\n> My original thoughts were that we could limit the sparse:path to\n> local use and disallow it over the wire to the server, but that\n> distinction is probably not worth the bother. Removing it completely\n> is fine.\n\nYeah, it had been my plan to limit it only via upload-pack, under the\nassumption that somebody probably wanted it on the local side. If you,\nwho added it, are OK with removing it completely, that gives me more\nconfidence that nobody is using it (coupled with the general\nexperimental nature of partial clones at this point). But I'm still a\nlittle worried somebody may have found a use for it in the meantime.\n\n-Peff\n"},{"id":"376299","messageId":"59f8a7c4-1df9-2b44-2456-485fbce750ba@jeffhostetler.com","threadId":"51167","inReplyTo":"20190528061317.GD7946@sigill.intra.peff.net","subject":"Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-28T13:29:49Z","receivedAt":"2019-05-28T13:29:53Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/28/2019 2:13 AM, Jeff King wrote:\n> On Fri, May 24, 2019 at 02:46:06PM -0400, Jeff Hostetler wrote:\n> \n>> My original thoughts were that we could limit the sparse:path to\n>> local use and disallow it over the wire to the server, but that\n>> distinction is probably not worth the bother. Removing it completely\n>> is fine.\n> \n> Yeah, it had been my plan to limit it only via upload-pack, under the\n> assumption that somebody probably wanted it on the local side. If you,\n> who added it, are OK with removing it completely, that gives me more\n> confidence that nobody is using it (coupled with the general\n> experimental nature of partial clones at this point). But I'm still a\n> little worried somebody may have found a use for it in the meantime.\n> \n> -Peff\n> \n\nyeah, let's simplify things and remove it completely for now.\n\nJeff\n"}]}