# [RFC/PATCH] upload-pack: disable object filtering when disabled by config 8 messages from 2018-03-28 to 2018-03-30. Participants: Jonathan Nieder, Junio C Hamano, Jeff Hostetler, Jeff King. Thread: https://gitlist.dev/t/48177 ## Jonathan Nieder, 2018-03-28 20:33 Subject: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: <20180328203303.GA260688@aiede.svl.corp.google.com> URL: https://gitlist.dev/e/20180328203303.GA260688%40aiede.svl.corp.google.com ``` When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, 2017-12-08), it was guarded by the uploadpack.allowFilter config item to allow server operators to control when they start supporting it. That config item didn't go far enough, though: it controls whether the 'filter' capability is advertised, but if a (custom) client ignores the capability advertisement and passes a filter specification anyway, the server would handle that despite allowFilter being false. This is particularly significant if a security bug is discovered in this new experimental partial clone code. Installations without uploadpack.allowFilter ought not to be affected since they don't intend to support partial clone, but they would be swept up into being vulnerable. Simplify and limit the attack surface by making uploadpack.allowFilter disable the feature, not just the advertisement of it. NEEDSWORK: tests Signed-off-by: Jonathan Nieder --- Noticed while reviewing the corresponding JGit code. If this change seems like a good idea, I can add tests and re-send it for real. Thanks, Jonathan Documentation/config.txt | 2 +- upload-pack.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ce9102cea8..4e0cff87f6 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3364,7 +3364,7 @@ uploadpack.packObjectsHook:: stdout. uploadpack.allowFilter:: - If this option is set, `upload-pack` will advertise partial + If this option is set, `upload-pack` will support partial clone and partial fetch object filtering. + Note that this configuration variable is ignored if it is seen in the diff --git a/upload-pack.c b/upload-pack.c index f51b6cfca9..4a82602be5 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -69,7 +69,7 @@ static int stateless_rpc; static const char *pack_objects_hook; static int filter_capability_requested; -static int filter_advertise; +static int allow_filter; static struct list_objects_filter_options filter_options; static void reset_timeout(void) @@ -846,7 +846,7 @@ static void receive_needs(void) no_progress = 1; if (parse_feature_request(features, "include-tag")) use_include_tag = 1; - if (parse_feature_request(features, "filter")) + if (allow_filter && parse_feature_request(features, "filter")) filter_capability_requested = 1; o = parse_object(&oid_buf); @@ -976,7 +976,7 @@ static int send_ref(const char *refname, const struct object_id *oid, " allow-reachable-sha1-in-want" : "", stateless_rpc ? " no-done" : "", symref_info.buf, - filter_advertise ? " filter" : "", + allow_filter ? " filter" : "", git_user_agent_sanitized()); strbuf_release(&symref_info); } else { @@ -1056,7 +1056,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused) if (!strcmp("uploadpack.packobjectshook", var)) return git_config_string(&pack_objects_hook, var, value); } else if (!strcmp("uploadpack.allowfilter", var)) { - filter_advertise = git_config_bool(var, value); + allow_filter = git_config_bool(var, value); } return parse_hide_refs_config(var, value, "uploadpack"); } -- 2.17.0.rc1.321.gba9d0f2565 ``` ## Junio C Hamano, 2018-03-28 22:12 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: URL: https://gitlist.dev/e/xmqqo9j77sub.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180328203303.GA260688@aiede.svl.corp.google.com> ``` Jonathan Nieder writes: > When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, > 2017-12-08), it was guarded by the uploadpack.allowFilter config item > to allow server operators to control when they start supporting it. > > That config item didn't go far enough, though: it controls whether the > 'filter' capability is advertised, but if a (custom) client ignores > the capability advertisement and passes a filter specification anyway, > the server would handle that despite allowFilter being false. > > This is particularly significant if a security bug is discovered in > this new experimental partial clone code. Installations without > uploadpack.allowFilter ought not to be affected since they don't > intend to support partial clone, but they would be swept up into being > vulnerable. > > Simplify and limit the attack surface by making uploadpack.allowFilter > disable the feature, not just the advertisement of it. > > NEEDSWORK: tests > > Signed-off-by: Jonathan Nieder > --- > Noticed while reviewing the corresponding JGit code. > > If this change seems like a good idea, I can add tests and re-send it > for real. Yup. The names of the static variables tell almost the whole story to convince me why this is a good change ;-). It is not about advertising a feature alone, but if the feature is actually enabled, so advertisement and handling of the feature should be either both enabled or disabled. Thanks. ``` ## Jeff Hostetler, 2018-03-29 13:47 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: <16479282-4241-7bf2-5f7c-b7889eb0e906@jeffhostetler.com> URL: https://gitlist.dev/e/16479282-4241-7bf2-5f7c-b7889eb0e906%40jeffhostetler.com In-Reply-To: ``` On 3/28/2018 6:12 PM, Junio C Hamano wrote: > Jonathan Nieder writes: > >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item >> to allow server operators to control when they start supporting it. >> >> That config item didn't go far enough, though: it controls whether the >> 'filter' capability is advertised, but if a (custom) client ignores >> the capability advertisement and passes a filter specification anyway, >> the server would handle that despite allowFilter being false. >> >> This is particularly significant if a security bug is discovered in >> this new experimental partial clone code. Installations without >> uploadpack.allowFilter ought not to be affected since they don't >> intend to support partial clone, but they would be swept up into being >> vulnerable. >> >> Simplify and limit the attack surface by making uploadpack.allowFilter >> disable the feature, not just the advertisement of it. >> >> NEEDSWORK: tests >> >> Signed-off-by: Jonathan Nieder >> --- >> Noticed while reviewing the corresponding JGit code. >> >> If this change seems like a good idea, I can add tests and re-send it >> for real. > > Yup. The names of the static variables tell almost the whole story > to convince me why this is a good change ;-). It is not about > advertising a feature alone, but if the feature is actually enabled, > so advertisement and handling of the feature should be either both > enabled or disabled. > > Thanks. > I agree. Thanks. ``` ## Jeff King, 2018-03-29 21:55 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: <20180329215502.GK2939@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180329215502.GK2939%40sigill.intra.peff.net In-Reply-To: <20180328203303.GA260688@aiede.svl.corp.google.com> ``` On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote: > When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, > 2017-12-08), it was guarded by the uploadpack.allowFilter config item > to allow server operators to control when they start supporting it. > > That config item didn't go far enough, though: it controls whether the > 'filter' capability is advertised, but if a (custom) client ignores > the capability advertisement and passes a filter specification anyway, > the server would handle that despite allowFilter being false. > > This is particularly significant if a security bug is discovered in > this new experimental partial clone code. Installations without > uploadpack.allowFilter ought not to be affected since they don't > intend to support partial clone, but they would be swept up into being > vulnerable. > > Simplify and limit the attack surface by making uploadpack.allowFilter > disable the feature, not just the advertisement of it. Great catch. If I understand correctly, this is an issue in the 2.17.0 release candidates. Is this worth delaying the release? It's a funny situation. The presence of a security bug is theoretical here, so nobody _should_ need to care, and this is just hardening. But it does make me a little nervous, given the experimental-ness of the new feature. -Peff ``` ## Jonathan Nieder, 2018-03-29 22:03 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: <20180329220257.GA209272@aiede.svl.corp.google.com> URL: https://gitlist.dev/e/20180329220257.GA209272%40aiede.svl.corp.google.com In-Reply-To: <20180329215502.GK2939@sigill.intra.peff.net> ``` Jeff King wrote: > On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote: >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item >> to allow server operators to control when they start supporting it. >> >> That config item didn't go far enough, though: it controls whether the >> 'filter' capability is advertised, but if a (custom) client ignores >> the capability advertisement and passes a filter specification anyway, >> the server would handle that despite allowFilter being false. [...] > Great catch. If I understand correctly, this is an issue in the 2.17.0 > release candidates. Is this worth delaying the release? Yes, IMHO this is a release blocker. (To put it another way, if this is going to delay the release, then we need to revert that patch.) Thanks, Jonathan ``` ## Jeff King, 2018-03-29 22:17 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: <20180329221725.GM2939@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180329221725.GM2939%40sigill.intra.peff.net In-Reply-To: <20180329220257.GA209272@aiede.svl.corp.google.com> ``` On Thu, Mar 29, 2018 at 03:03:54PM -0700, Jonathan Nieder wrote: > Jeff King wrote: > > On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote: > > >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12, > >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item > >> to allow server operators to control when they start supporting it. > >> > >> That config item didn't go far enough, though: it controls whether the > >> 'filter' capability is advertised, but if a (custom) client ignores > >> the capability advertisement and passes a filter specification anyway, > >> the server would handle that despite allowFilter being false. > [...] > > Great catch. If I understand correctly, this is an issue in the 2.17.0 > > release candidates. Is this worth delaying the release? > > Yes, IMHO this is a release blocker. (To put it another way, if this is > going to delay the release, then we need to revert that patch.) I think I'd agree on it being a release blocker. Given that your fix is really a one-liner (3 of the lines are just changing the variable name, which I agree with), I'd be fine with applying it on top rather than reverting the original, even if it means delaying the release slightly. It seems like about the same amount of risk to me. (Sometimes it's actually _less_ risky to apply the one-liner fix, because reverting a large feature can have unintended interactions with other features that were built in the meantime. Looking at the original patch, though, I doubt that is the case here, hence my "about the same amount of risk"). -Peff ``` ## Junio C Hamano, 2018-03-29 22:31 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: URL: https://gitlist.dev/e/xmqq4lky5x9j.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180329221725.GM2939@sigill.intra.peff.net> ``` Jeff King writes: > I think I'd agree on it being a release blocker. Given that your fix is > really a one-liner (3 of the lines are just changing the variable name, > which I agree with), I'd be fine with applying it on top rather than > reverting the original, even if it means delaying the release slightly. > It seems like about the same amount of risk to me. Yeah, I would say we should just apply the rfc/patch as-is directly on 'master'. Thanks. ``` ## Junio C Hamano, 2018-03-30 01:45 Subject: Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config Message-ID: URL: https://gitlist.dev/e/xmqqsh8i49qn.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Junio C Hamano writes: > Jeff King writes: > >> I think I'd agree on it being a release blocker. Given that your fix is >> really a one-liner (3 of the lines are just changing the variable name, >> which I agree with), I'd be fine with applying it on top rather than >> reverting the original, even if it means delaying the release slightly. >> It seems like about the same amount of risk to me. > > Yeah, I would say we should just apply the rfc/patch as-is directly > on 'master'. ... which just has happened. Thanks. ```