On Wed, Feb 28, 2024 at 05:38:58PM -0500, Jeff King wrote: > From: Taylor Blau <me@xxxxxxxxxxxx> > > We added an "object-info" capability to the v2 upload-pack protocol in > a2ba162cda (object-info: support for retrieving object info, > 2021-04-20). In the almost 3 years since, we have not added any > client-side support, and it does not appear to exist in other > implementations either (JGit understands the verb on the server side, > but not on the client side). > > Since this largely unused code is accessible over the network by > default, it increases the attack surface of upload-pack. I don't know of > any particularly severe problem, but one issue is that because of the > request/response nature of the v2 protocol, it will happily read an > unbounded number of packets, adding each one to a string list (without > regard to whether they are objects we know about, duplicates, etc). > > This may be something we want to improve in the long run, but in the > short term it makes sense to disable the feature entirely. We'll add a > config option as an escape hatch for anybody who wants to develop the > feature further. > > A more gentle option would be to add the config option to let people > disable it manually, but leave it enabled by default. But given that > there's no client side support, that seems like the wrong balance with > security. > > Disabling by default will slow adoption a bit once client-side support > does become available (there were some patches[1] in 2022, but nothing > got merged and there's been nothing since). But clients have to deal > with older servers that do not understand the option anyway (and the > capability system handles that), so it will just be a matter of servers > flipping their config at that point (and hopefully once any unbounded > allocations have been addressed). > > [jk: this is a patch that GitHub has been running for several years, but > rebased forward and with a new commit message for upstream] > > [1] https://lore.kernel.org/git/20220208231911.725273-1-calvinwan@xxxxxxxxxx/ > > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > Documentation/config/transfer.txt | 4 ++++ > serve.c | 14 +++++++++++++- > t/t5555-http-smart-common.sh | 1 - > t/t5701-git-serve.sh | 24 +++++++++++++++++++++++- > 4 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt > index a9cbdb88a1..f1ce50f4a6 100644 > --- a/Documentation/config/transfer.txt > +++ b/Documentation/config/transfer.txt > @@ -121,3 +121,7 @@ transfer.bundleURI:: > information from the remote server (if advertised) and download > bundles before continuing the clone through the Git protocol. > Defaults to `false`. > + > +transfer.advertiseObjectInfo:: > + When `true`, the `object-info` capability is advertised by > + servers. Defaults to false. > diff --git a/serve.c b/serve.c > index a1d71134d4..aa651b73e9 100644 > --- a/serve.c > +++ b/serve.c > @@ -12,6 +12,7 @@ > #include "trace2.h" > > static int advertise_sid = -1; > +static int advertise_object_info = -1; > static int client_hash_algo = GIT_HASH_SHA1; > > static int always_advertise(struct repository *r UNUSED, > @@ -67,6 +68,17 @@ static void session_id_receive(struct repository *r UNUSED, > trace2_data_string("transfer", NULL, "client-sid", client_sid); > } > > +static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED) > +{ > + if (advertise_object_info == -1 && > + repo_config_get_bool(r, "transfer.advertiseobjectinfo", > + &advertise_object_info)) { > + /* disabled by default */ > + advertise_object_info = 0; > + } > + return advertise_object_info; > +} > + > struct protocol_capability { > /* > * The name of the capability. The server uses this name when > @@ -135,7 +147,7 @@ static struct protocol_capability capabilities[] = { > }, > { > .name = "object-info", > - .advertise = always_advertise, > + .advertise = object_info_advertise, > .command = cap_object_info, > }, > { > diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh > index b1cfe8b7db..3dcb3340a3 100755 > --- a/t/t5555-http-smart-common.sh > +++ b/t/t5555-http-smart-common.sh > @@ -131,7 +131,6 @@ test_expect_success 'git upload-pack --advertise-refs: v2' ' > fetch=shallow wait-for-done > server-option > object-format=$(test_oid algo) > - object-info > 0000 > EOF > > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > index 3591bc2417..c48830de8f 100755 > --- a/t/t5701-git-serve.sh > +++ b/t/t5701-git-serve.sh > @@ -20,7 +20,6 @@ test_expect_success 'test capability advertisement' ' > fetch=shallow wait-for-done > server-option > object-format=$(test_oid algo) > - object-info > EOF > cat >expect.trailer <<-EOF && > 0000 > @@ -323,6 +322,8 @@ test_expect_success 'unexpected lines are not allowed in fetch request' ' > # Test the basics of object-info > # > test_expect_success 'basics of object-info' ' > + test_config transfer.advertiseObjectInfo true && > + > test-tool pkt-line pack >in <<-EOF && > command=object-info > object-format=$(test_oid algo) > @@ -380,4 +381,25 @@ test_expect_success 'basics of bundle-uri: dies if not enabled' ' > test_must_be_empty out > ' > > +test_expect_success 'object-info missing from capabilities when disabled' ' > + test_config transfer.advertiseObjectInfo false && > + > + GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \ > + --advertise-capabilities >out && > + test-tool pkt-line unpack <out >actual && > + > + ! grep object.info actual > +' Is it intentional that you grep for "object.info" instead of "object-info"? Patrick > +test_expect_success 'object-info commands rejected when disabled' ' > + test_config transfer.advertiseObjectInfo false && > + > + test-tool pkt-line pack >in <<-EOF && > + command=object-info > + EOF > + > + test_must_fail test-tool serve-v2 --stateless-rpc <in 2>err && > + grep invalid.command err > +' > + > test_done > -- > 2.44.0.rc2.424.gbdbf4d014b > >
Attachment:
signature.asc
Description: PGP signature
