From: Patrick Steinhardt <ps@pks.im>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Benjamin Flesch <benjaminflesch@icloud.com>
Subject: Re: [PATCH 6/9] upload-pack: disallow object-info capability by default
Date: Mon, 4 Mar 2024 09:34:30 +0100 [thread overview]
Message-ID: <ZeWHlknuWMvRiFtC@tanuki> (raw)
In-Reply-To: <20240228223858.GF1158131@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 6335 bytes --]
On Wed, Feb 28, 2024 at 05:38:58PM -0500, Jeff King wrote:
> From: Taylor Blau <me@ttaylorr.com>
>
> 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@google.com/
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-03-04 8:34 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-28 22:37 [PATCH 0/9] bound upload-pack memory allocations Jeff King
2024-02-28 22:37 ` [PATCH 1/9] upload-pack: drop separate v2 "haves" array Jeff King
2024-02-28 22:37 ` [PATCH 2/9] upload-pack: switch deepen-not list to an oid_array Jeff King
2024-02-28 22:37 ` [PATCH 3/9] upload-pack: use oidset for deepen_not list Jeff King
2024-02-28 22:38 ` [PATCH 4/9] upload-pack: use a strmap for want-ref lines Jeff King
2024-02-28 22:38 ` [PATCH 5/9] upload-pack: accept only a single packfile-uri line Jeff King
2024-02-28 22:38 ` [PATCH 6/9] upload-pack: disallow object-info capability by default Jeff King
2024-03-04 8:34 ` Patrick Steinhardt [this message]
2024-03-04 9:59 ` Jeff King
2024-04-29 20:49 ` Taylor Blau
2024-02-28 22:39 ` [PATCH 7/9] upload-pack: always turn off save_commit_buffer Jeff King
2024-02-28 22:39 ` [PATCH 8/9] upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places Jeff King
2024-02-28 22:39 ` [PATCH 9/9] upload-pack: free tree buffers after parsing Jeff King
2024-03-04 8:33 ` Patrick Steinhardt
2024-03-04 9:57 ` Jeff King
2024-03-04 10:00 ` Patrick Steinhardt
2024-02-28 22:47 ` [PATCH 0/9] bound upload-pack memory allocations Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZeWHlknuWMvRiFtC@tanuki \
--to=ps@pks.im \
--cc=benjaminflesch@icloud.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).