git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 09/11] serve: reject bogus v2 "command=ls-refs=foo"
Date: Wed, 15 Sep 2021 14:36:33 -0400	[thread overview]
Message-ID: <YUI9MRXNAFxzVoA3@coredump.intra.peff.net> (raw)
In-Reply-To: <YUI8z5SiyvgrDBas@coredump.intra.peff.net>

When we see a line from the client like "command=ls-refs", we parse
everything after the equals sign as a capability, which we check against
our capabilities table. If we don't recognize the command (e.g.,
"command=foo"), we'll reject it.

But in parse_command(), we use the same get_capability() parser for
parsing non-command lines. So if we see "command=ls-refs=foo", we will
feed "ls-refs=foo" to get_capability(), which will say "OK, that's
ls-refs, with value 'foo'". But then we simply ignore the value
entirely.

The client is violating the spec here, which says:

      command = PKT-LINE("command=" key LF)
      key = 1*(ALPHA | DIGIT | "-_")

I.e., the key is not even allowed to have an equals sign in it. Whereas
a real non-command capability does allow a value:

      capability = PKT-LINE(key[=value] LF)

So by reusing the same get_capability() parser, we are mixing up the
"key" and "capability" tokens. However, since that parser tells us
whether it saw an "=", we can still use it; we just need to reject any
input that produces a non-NULL value field.

The current behavior isn't really hurting anything (the client should
never send such a request, and if it does, we just ignore the "value"
part). But since it does violate the spec, let's tighten it up to
prevent any surprising behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c              |  2 +-
 t/t5701-git-serve.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/serve.c b/serve.c
index 1a7c8a118f..db5ecfed2d 100644
--- a/serve.c
+++ b/serve.c
@@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command)
 		if (*command)
 			die("command '%s' requested after already requesting command '%s'",
 			    out, (*command)->name);
-		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)
+		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value)
 			die("invalid command '%s'", out);
 
 		*command = cmd;
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 520672f842..2e51886def 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -72,6 +72,16 @@ test_expect_success 'request invalid command' '
 	test_i18ngrep "invalid command" err
 '
 
+test_expect_success 'requested command is command=value' '
+	test-tool pkt-line pack >in <<-EOF &&
+	command=ls-refs=whatever
+	object-format=$(test_oid algo)
+	0000
+	EOF
+	test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&
+	grep invalid.command.*ls-refs=whatever err
+'
+
 test_expect_success 'wrong object-format' '
 	test-tool pkt-line pack >in <<-EOF &&
 	command=fetch
-- 
2.33.0.917.g33ebf6a5f6


  parent reply	other threads:[~2021-09-15 18:36 UTC|newest]

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 15:29 [PATCH 0/9] reducing memory allocations for v2 servers Jeff King
2021-09-14 15:30 ` [PATCH 1/9] serve: rename is_command() to parse_command() Jeff King
2021-09-14 15:30 ` [PATCH 2/9] serve: return capability "value" from get_capability() Jeff King
2021-09-14 15:31 ` [PATCH 3/9] serve: add "receive" method for v2 capabilities table Jeff King
2021-09-14 15:31 ` [PATCH 4/9] serve: provide "receive" function for object-format capability Jeff King
2021-09-14 18:59   ` Martin Ågren
2021-09-14 15:33 ` [PATCH 5/9] serve: provide "receive" function for session-id capability Jeff King
2021-09-14 16:55   ` Taylor Blau
2021-09-14 17:06     ` Jeff King
2021-09-14 17:12       ` Taylor Blau
2021-09-14 19:02   ` Martin Ågren
2021-09-14 19:14     ` Jeff King
2021-09-14 15:33 ` [PATCH 6/9] serve: drop "keys" strvec Jeff King
2021-09-14 16:59   ` Taylor Blau
2021-09-14 17:16     ` Jeff King
2021-09-14 15:37 ` [PATCH 7/9] ls-refs: ignore very long ref-prefix counts Jeff King
2021-09-14 17:18   ` Taylor Blau
2021-09-14 17:27     ` Jeff King
2021-09-14 17:23   ` Jeff King
2021-09-14 19:06   ` Martin Ågren
2021-09-14 19:22     ` Jeff King
2021-09-14 22:09   ` Jeff King
2021-09-14 22:11     ` Taylor Blau
2021-09-14 22:15       ` Jeff King
2021-09-14 15:37 ` [PATCH 8/9] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
2021-09-14 17:21   ` Taylor Blau
2021-09-14 15:37 ` [PATCH 9/9] serve: reject commands used as capabilities Jeff King
2021-09-14 17:30 ` [PATCH 0/9] reducing memory allocations for v2 servers Taylor Blau
2021-09-14 18:00 ` Junio C Hamano
2021-09-14 18:38   ` Jeff King
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
2021-09-14 23:51   ` [PATCH v2 01/11] serve: rename is_command() to parse_command() Jeff King
2021-09-14 23:51   ` [PATCH v2 02/11] serve: return capability "value" from get_capability() Jeff King
2021-09-14 23:51   ` [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table Jeff King
2021-09-15  0:31     ` Ævar Arnfjörð Bjarmason
2021-09-15 16:35       ` Jeff King
2021-09-15 16:41     ` Junio C Hamano
2021-09-15 16:57       ` Jeff King
2021-09-14 23:51   ` [PATCH v2 04/11] serve: provide "receive" function for object-format capability Jeff King
2021-09-15 16:54     ` Junio C Hamano
2021-09-14 23:51   ` [PATCH v2 05/11] serve: provide "receive" function for session-id capability Jeff King
2021-09-15 16:56     ` Junio C Hamano
2021-09-14 23:51   ` [PATCH v2 06/11] serve: drop "keys" strvec Jeff King
2021-09-15 17:01     ` Junio C Hamano
2021-09-14 23:51   ` [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
2021-09-15  4:16     ` Taylor Blau
2021-09-15 16:39       ` Jeff King
2021-09-15  5:00     ` Eric Sunshine
2021-09-15 16:40       ` Jeff King
2021-09-14 23:52   ` [PATCH v2 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
2021-09-14 23:52   ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
2021-09-15  0:27     ` Ævar Arnfjörð Bjarmason
2021-09-15 16:28       ` Jeff King
2021-09-15  5:09     ` Eric Sunshine
2021-09-15 16:32       ` Jeff King
2021-09-15 17:33     ` Junio C Hamano
2021-09-15 17:39       ` Jeff King
2021-09-14 23:52   ` [PATCH v2 10/11] serve: reject commands used as capabilities Jeff King
2021-09-14 23:54   ` [PATCH v2 11/11] ls-refs: reject unknown arguments Jeff King
2021-09-15  0:09     ` Ævar Arnfjörð Bjarmason
2021-09-15 16:25       ` Jeff King
2021-09-15  4:17   ` [PATCH v2 0/11] limit memory allocations for v2 servers Taylor Blau
2021-09-15 18:33   ` Jeff King
2021-09-15 18:34     ` [PATCH v3 " Jeff King
2021-09-15 18:35       ` [PATCH v3 01/11] serve: rename is_command() to parse_command() Jeff King
2021-09-15 18:35       ` [PATCH v3 02/11] serve: return capability "value" from get_capability() Jeff King
2021-09-15 18:35       ` [PATCH v3 03/11] serve: add "receive" method for v2 capabilities table Jeff King
2021-09-15 18:35       ` [PATCH v3 04/11] serve: provide "receive" function for object-format capability Jeff King
2021-09-15 18:35       ` [PATCH v3 05/11] serve: provide "receive" function for session-id capability Jeff King
2021-09-15 18:35       ` [PATCH v3 06/11] serve: drop "keys" strvec Jeff King
2021-09-15 18:35       ` [PATCH v3 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
2021-09-15 18:35       ` [PATCH v3 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
2021-09-15 18:36       ` Jeff King [this message]
2021-09-15 18:36       ` [PATCH v3 10/11] serve: reject commands used as capabilities Jeff King
2021-09-15 18:36       ` [PATCH v3 11/11] ls-refs: reject unknown arguments Jeff King
2021-09-15  0:25 ` [PATCH 0/9] reducing memory allocations for v2 servers Ævar Arnfjörð Bjarmason
2021-09-15 16:41   ` Jeff King

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=YUI9MRXNAFxzVoA3@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=sunshine@sunshineco.com \
    /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 NNTP newsgroup(s).