Jonathan NiederWhen 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(-)
Show changes to 2 files +5 −5
Documentation/config.txt, upload-pack.c
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
