[RFC/PATCH] upload-pack: disable object filtering when disabled by config

[RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
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

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> 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 <jrnieder@gmail.com>
> ---
> 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.

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff Hostetler]

On 3/28/2018 6:12 PM, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> 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 <jrnieder@gmail.com>
>> ---
>> 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.

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff King]

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

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jonathan Nieder]

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

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jeff King]

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

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> 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.