{"thread":{"id":"48177","subject":"[RFC/PATCH] upload-pack: disable object filtering when disabled by config","startedAt":"2018-03-28T20:33:36Z","lastAt":"2018-03-30T01:45:29Z","messageCount":8,"participants":["Jonathan Nieder","Junio C Hamano","Jeff Hostetler","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343272","messageId":"20180328203303.GA260688@aiede.svl.corp.google.com","threadId":"48177","inReplyTo":null,"subject":"[RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-28T20:33:03Z","receivedAt":"2018-03-28T20:33:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n2017-12-08), it was guarded by the uploadpack.allowFilter config item\nto allow server operators to control when they start supporting it.\n\nThat config item didn't go far enough, though: it controls whether the\n'filter' capability is advertised, but if a (custom) client ignores\nthe capability advertisement and passes a filter specification anyway,\nthe server would handle that despite allowFilter being false.\n\nThis is particularly significant if a security bug is discovered in\nthis new experimental partial clone code.  Installations without\nuploadpack.allowFilter ought not to be affected since they don't\nintend to support partial clone, but they would be swept up into being\nvulnerable.\n\nSimplify and limit the attack surface by making uploadpack.allowFilter\ndisable the feature, not just the advertisement of it.\n\nNEEDSWORK: tests\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nNoticed while reviewing the corresponding JGit code.\n\nIf this change seems like a good idea, I can add tests and re-send it\nfor real.\n\nThanks,\nJonathan\n\n Documentation/config.txt | 2 +-\n upload-pack.c            | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ce9102cea8..4e0cff87f6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3364,7 +3364,7 @@ uploadpack.packObjectsHook::\n \tstdout.\n \n uploadpack.allowFilter::\n-\tIf this option is set, `upload-pack` will advertise partial\n+\tIf this option is set, `upload-pack` will support partial\n \tclone and partial fetch object filtering.\n +\n Note that this configuration variable is ignored if it is seen in the\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f51b6cfca9..4a82602be5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -69,7 +69,7 @@ static int stateless_rpc;\n static const char *pack_objects_hook;\n \n static int filter_capability_requested;\n-static int filter_advertise;\n+static int allow_filter;\n static struct list_objects_filter_options filter_options;\n \n static void reset_timeout(void)\n@@ -846,7 +846,7 @@ static void receive_needs(void)\n \t\t\tno_progress = 1;\n \t\tif (parse_feature_request(features, \"include-tag\"))\n \t\t\tuse_include_tag = 1;\n-\t\tif (parse_feature_request(features, \"filter\"))\n+\t\tif (allow_filter && parse_feature_request(features, \"filter\"))\n \t\t\tfilter_capability_requested = 1;\n \n \t\to = parse_object(&oid_buf);\n@@ -976,7 +976,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t\t     \" allow-reachable-sha1-in-want\" : \"\",\n \t\t\t     stateless_rpc ? \" no-done\" : \"\",\n \t\t\t     symref_info.buf,\n-\t\t\t     filter_advertise ? \" filter\" : \"\",\n+\t\t\t     allow_filter ? \" filter\" : \"\",\n \t\t\t     git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t} else {\n@@ -1056,7 +1056,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n \t\t\treturn git_config_string(&pack_objects_hook, var, value);\n \t} else if (!strcmp(\"uploadpack.allowfilter\", var)) {\n-\t\tfilter_advertise = git_config_bool(var, value);\n+\t\tallow_filter = git_config_bool(var, value);\n \t}\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n-- \n2.17.0.rc1.321.gba9d0f2565\n\n"},{"id":"343279","messageId":"xmqqo9j77sub.fsf@gitster-ct.c.googlers.com","threadId":"48177","inReplyTo":"20180328203303.GA260688@aiede.svl.corp.google.com","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-28T22:12:12Z","receivedAt":"2018-03-28T22:12:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n> 2017-12-08), it was guarded by the uploadpack.allowFilter config item\n> to allow server operators to control when they start supporting it.\n>\n> That config item didn't go far enough, though: it controls whether the\n> 'filter' capability is advertised, but if a (custom) client ignores\n> the capability advertisement and passes a filter specification anyway,\n> the server would handle that despite allowFilter being false.\n>\n> This is particularly significant if a security bug is discovered in\n> this new experimental partial clone code.  Installations without\n> uploadpack.allowFilter ought not to be affected since they don't\n> intend to support partial clone, but they would be swept up into being\n> vulnerable.\n>\n> Simplify and limit the attack surface by making uploadpack.allowFilter\n> disable the feature, not just the advertisement of it.\n>\n> NEEDSWORK: tests\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Noticed while reviewing the corresponding JGit code.\n>\n> If this change seems like a good idea, I can add tests and re-send it\n> for real.\n\nYup.  The names of the static variables tell almost the whole story\nto convince me why this is a good change ;-).  It is not about\nadvertising a feature alone, but if the feature is actually enabled,\nso advertisement and handling of the feature should be either both\nenabled or disabled.\n\nThanks.\n"},{"id":"343322","messageId":"16479282-4241-7bf2-5f7c-b7889eb0e906@jeffhostetler.com","threadId":"48177","inReplyTo":"xmqqo9j77sub.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-03-29T13:47:12Z","receivedAt":"2018-03-29T13:47:18Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/28/2018 6:12 PM, Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n>> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n>> 2017-12-08), it was guarded by the uploadpack.allowFilter config item\n>> to allow server operators to control when they start supporting it.\n>>\n>> That config item didn't go far enough, though: it controls whether the\n>> 'filter' capability is advertised, but if a (custom) client ignores\n>> the capability advertisement and passes a filter specification anyway,\n>> the server would handle that despite allowFilter being false.\n>>\n>> This is particularly significant if a security bug is discovered in\n>> this new experimental partial clone code.  Installations without\n>> uploadpack.allowFilter ought not to be affected since they don't\n>> intend to support partial clone, but they would be swept up into being\n>> vulnerable.\n>>\n>> Simplify and limit the attack surface by making uploadpack.allowFilter\n>> disable the feature, not just the advertisement of it.\n>>\n>> NEEDSWORK: tests\n>>\n>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n>> ---\n>> Noticed while reviewing the corresponding JGit code.\n>>\n>> If this change seems like a good idea, I can add tests and re-send it\n>> for real.\n> \n> Yup.  The names of the static variables tell almost the whole story\n> to convince me why this is a good change ;-).  It is not about\n> advertising a feature alone, but if the feature is actually enabled,\n> so advertisement and handling of the feature should be either both\n> enabled or disabled.\n> \n> Thanks.\n> \n\nI agree.  Thanks.\n"},{"id":"343375","messageId":"20180329215502.GK2939@sigill.intra.peff.net","threadId":"48177","inReplyTo":"20180328203303.GA260688@aiede.svl.corp.google.com","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T21:55:02Z","receivedAt":"2018-03-29T21:55:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:\n\n> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n> 2017-12-08), it was guarded by the uploadpack.allowFilter config item\n> to allow server operators to control when they start supporting it.\n> \n> That config item didn't go far enough, though: it controls whether the\n> 'filter' capability is advertised, but if a (custom) client ignores\n> the capability advertisement and passes a filter specification anyway,\n> the server would handle that despite allowFilter being false.\n> \n> This is particularly significant if a security bug is discovered in\n> this new experimental partial clone code.  Installations without\n> uploadpack.allowFilter ought not to be affected since they don't\n> intend to support partial clone, but they would be swept up into being\n> vulnerable.\n> \n> Simplify and limit the attack surface by making uploadpack.allowFilter\n> disable the feature, not just the advertisement of it.\n\nGreat catch. If I understand correctly, this is an issue in the 2.17.0\nrelease candidates. Is this worth delaying the release?\n\nIt's a funny situation. The presence of a security bug is theoretical\nhere, so nobody _should_ need to care, and this is just hardening. But\nit does make me a little nervous, given the experimental-ness of the new\nfeature.\n\n-Peff\n"},{"id":"343376","messageId":"20180329220257.GA209272@aiede.svl.corp.google.com","threadId":"48177","inReplyTo":"20180329215502.GK2939@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-29T22:03:54Z","receivedAt":"2018-03-29T22:04:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:\n\n>> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n>> 2017-12-08), it was guarded by the uploadpack.allowFilter config item\n>> to allow server operators to control when they start supporting it.\n>>\n>> That config item didn't go far enough, though: it controls whether the\n>> 'filter' capability is advertised, but if a (custom) client ignores\n>> the capability advertisement and passes a filter specification anyway,\n>> the server would handle that despite allowFilter being false.\n[...]\n> Great catch. If I understand correctly, this is an issue in the 2.17.0\n> release candidates. Is this worth delaying the release?\n\nYes, IMHO this is a release blocker.  (To put it another way, if this is\ngoing to delay the release, then we need to revert that patch.)\n\nThanks,\nJonathan\n"},{"id":"343378","messageId":"20180329221725.GM2939@sigill.intra.peff.net","threadId":"48177","inReplyTo":"20180329220257.GA209272@aiede.svl.corp.google.com","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T22:17:25Z","receivedAt":"2018-03-29T22:17:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 03:03:54PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:\n> \n> >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,\n> >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item\n> >> to allow server operators to control when they start supporting it.\n> >>\n> >> That config item didn't go far enough, though: it controls whether the\n> >> 'filter' capability is advertised, but if a (custom) client ignores\n> >> the capability advertisement and passes a filter specification anyway,\n> >> the server would handle that despite allowFilter being false.\n> [...]\n> > Great catch. If I understand correctly, this is an issue in the 2.17.0\n> > release candidates. Is this worth delaying the release?\n> \n> Yes, IMHO this is a release blocker.  (To put it another way, if this is\n> going to delay the release, then we need to revert that patch.)\n\nI think I'd agree on it being a release blocker. Given that your fix is\nreally a one-liner (3 of the lines are just changing the variable name,\nwhich I agree with), I'd be fine with applying it on top rather than\nreverting the original, even if it means delaying the release slightly.\nIt seems like about the same amount of risk to me.\n\n(Sometimes it's actually _less_ risky to apply the one-liner fix,\nbecause reverting a large feature can have unintended interactions with\nother features that were built in the meantime.  Looking at the original\npatch, though, I doubt that is the case here, hence my \"about the same\namount of risk\").\n\n-Peff\n"},{"id":"343381","messageId":"xmqq4lky5x9j.fsf@gitster-ct.c.googlers.com","threadId":"48177","inReplyTo":"20180329221725.GM2939@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-29T22:31:52Z","receivedAt":"2018-03-29T22:32:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think I'd agree on it being a release blocker. Given that your fix is\n> really a one-liner (3 of the lines are just changing the variable name,\n> which I agree with), I'd be fine with applying it on top rather than\n> reverting the original, even if it means delaying the release slightly.\n> It seems like about the same amount of risk to me.\n\nYeah, I would say we should just apply the rfc/patch as-is directly\non 'master'.\n\nThanks.\n\n"},{"id":"343385","messageId":"xmqqsh8i49qn.fsf@gitster-ct.c.googlers.com","threadId":"48177","inReplyTo":"xmqq4lky5x9j.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T01:45:20Z","receivedAt":"2018-03-30T01:45:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> I think I'd agree on it being a release blocker. Given that your fix is\n>> really a one-liner (3 of the lines are just changing the variable name,\n>> which I agree with), I'd be fine with applying it on top rather than\n>> reverting the original, even if it means delaying the release slightly.\n>> It seems like about the same amount of risk to me.\n>\n> Yeah, I would say we should just apply the rfc/patch as-is directly\n> on 'master'.\n\n... which just has happened.\n\nThanks.\n"}]}