patch, 9 partsreducing memory allocations for v2 servers

While looking at [1], I noticed that v2 servers will read a few bits of client input into strvecs. Even though we expect these to be small-ish, there's nothing preventing a client from sending us a bunch of junk and wasting memory.

This series changes that, putting a cap on how much data we'll receive. The two spots are the "capabilities" list we receive (before we even dispatch to a particular command like ls-refs or fetch), and the ref-prefix list we receive for ls-refs.

To deal with the capabilities issue, we'll just handle each capability line as it comes in, rather than storing a list. This requires a bit of cleanup, which is why it takes up the first 6 patches. It also needs a few other cleanups from ab/serve-cleanup (and so this series is based on that). The dependencies there are both textual (e.g., using designated initializers in the capabilities table) and functional (dropping the "keys" parameter from v2 command() functions).

Patch 7 fixes the ls-refs issue by degrading when we see too many prefixes (which works because the capability is optional in the first place).

The final two patches aren't strictly necessary. They're a tightening of the protocol against some bogus input that I noticed while doing the earlier cleanups. That bogus input isn't really hurting anything, but I think it makes sense to reject nonsense from the client rather than ignoring it. The obvious risk is that some client for some reason would send us nonsense, but I don't think Git ever has.

Once again, these are based on ab/serve-cleanup, which is currently in next.

  [1/9]: serve: rename is_command() to parse_command()
  [2/9]: serve: return capability "value" from get_capability()
  [3/9]: serve: add "receive" method for v2 capabilities table
  [4/9]: serve: provide "receive" function for object-format capability
  [5/9]: serve: provide "receive" function for session-id capability
  [6/9]: serve: drop "keys" strvec
  [7/9]: ls-refs: ignore very long ref-prefix counts
  [8/9]: serve: reject bogus v2 "command=ls-refs=foo"
  [9/9]: serve: reject commands used as capabilities

 ls-refs.c            |  19 ++++++-
 serve.c              | 120 +++++++++++++++++++++++--------------------
 t/t5701-git-serve.sh |  60 ++++++++++++++++++++++
 3 files changed, 142 insertions(+), 57 deletions(-)

-Peff

[1] https://lore.kernel.org/git/YT54CNYgtGcqexwq@coredump.intra.peff.net/

The is_command() function not only tells us whether the pktline is a valid command string, but it also parses out the command (and complains if we see a duplicate). Let's rename it to make those extra functions a bit more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not strictly necessary, but the name caused me a fair bit of
confusion at first while touching this code.

 serve.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

When the client sends v2 capabilities, they may be simple, like:

  foo

or have a value like:

  foo=bar

(all of the current capabilities actually expect a value, but the protocol allows for boolean ones).

We use get_capability() to make sure the client's pktline matches a capability. In doing so, we parse enough to see the "=" and the value (if any), but we immediately forget it. Nobody cares for now, because they end up parsing the values out later using has_capability(). But in preparation for changing that, let's pass back a pointer so the callers know what we found.

Note that unlike has_capability(), we'll return NULL for a "simple" capability. Distinguishing these will be useful for some future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
We get rid of has_capability() later, so the inconsistency in return
types will go away.

 serve.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

We have a capabilities table that tells us what we should tell the client we are capable of, and what to do when a client gives us a particular command (e.g., "command=ls-refs"). But it doesn't tell us what to do when the client sends us back a capability (e.g., "object-format=sha256"). We just collect them all in a strvec and hope somebody can use them later.

Instead, let's provide a function pointer in the table to act on these. This will eventually help us avoid collecting the strings, which will be more efficient and less prone to mischief.

Using the new method is optional, which helps in two ways:

  - we can move existing capabilities over to this new system gradually
    in individual commits

  - some capabilities we don't actually do anything with anyway. For
    example, the client is free to say "agent=git/1.2.3" to us, but we
    do not act on the information in any way.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

We get any "object-format" specified by the client by searching for it in the collected list of capabilities the client sent. We can instead just handle it as soon as they send it. This is slightly more efficient, and gets us one step closer to dropping that collected list.

Note that we do still have to do our final hash check after receiving all capabilities (because they might not have sent an object-format line at all, and we still have to check that the default matches our repository algorithm). Since the check_algorithm() function would now be done to a single if() statement, I've just inlined it in its only caller.

There should be no change of behavior here, except for two broken-protocol cases:

  - if the client sends multiple conflicting object-format capabilities
    (which they should not), we'll now choose the last one rather than
    the first. We could also detect and complain about the duplicates
    quite easily now, which we could not before, but I didn't do so
    here.

  - if the client sends a bogus "object-format" with no equals sign,
    we'll now say so, rather than "unknown object format: ''"

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

On Tue, 14 Sept 2021 at 17:33, Jeff King <peff@peff.net> wrote:

s/done/down/, I believe.

FWIW, I agree with this.

Martin

Rather than pulling the session-id string from the list of collected capabilities, we can handle it as soon as we receive it. This gets us closer to dropping the collected list entirely.

The behavior should be the same, with one exception. Previously if the client sent us multiple session-id lines, we'd report only the first. Now we'll pass each one along to trace2. This shouldn't matter in practice, since clients shouldn't do that (and if they do, it's probably sensible to log them all).

As this removes the last caller of the static has_capability(), we can remove it, as well (and in fact we must to avoid -Wunused-function complaining).

Signed-off-by: Jeff King <peff@peff.net>
---
I had originally dropped has_capability() in a separate patch, to keep
this one more readable. That breaks bisectability, but only with
-Werror. I'm not sure where we should fall on that spectrum (I generally
bisect with -Wno-error just because warnings may come and go when
working with different compilers than what was normal at the time).

Not that big a deal either way for this patch, but I wonder if people have opinions in general.

 serve.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

On Tue, Sep 14, 2021 at 11:33:09AM -0400, Jeff King wrote:

I tend to fall the same way, especially when bisecting things in ancient (to me) versions of Git where my current compiler complains. So I think the approach that you took here is just fine.

(This patch, as with all leading up to it, looks good to me.)

Thanks, Taylor

On Tue, Sep 14, 2021 at 12:55:01PM -0400, Taylor Blau wrote:

To be clear, the approach here is conservative: it will bisect even with -Werror. I think what you're saying is I _could_ have done the other approach, and put the removal into its own commit?

-Peff

On Tue, Sep 14, 2021 at 01:06:42PM -0400, Jeff King wrote:

Yes. I would have been fine with dropping has_capability() in its own patch, since the result would have been more readable (and since I never bisect with `-Werror`). But this (the conservative approach of persevering bisect-ability even with `-Werror` is fine with me, too).

Thanks, Taylor

On Tue, 14 Sept 2021 at 17:34, Jeff King <peff@peff.net> wrote:

Looking good.

First of all, agreed about the "not that big a deal" part. Just a random thought: You could do the opposite of what Elijah sometimes does by first adding a "MAYBE_UNUSED" function, then actually using it. You'd add "MAYBE_UNUSED" here, then the next commit would drop the whole thing. It could be worth it if you're removing many many lines so that the "actual" change gets lost in the noise. But this patch isn't near any such threshold, IMHO (if there even is such a "threshold").

Handling NULL. Nice. :)

Martin

On Tue, Sep 14, 2021 at 09:02:10PM +0200, Martin Ågren wrote:

Yeah, I considered that (because I had seen Elijah do it; I didn't think of it myself). I don't love it, if only because now the extra MAYBE_UNUSED is a head-scratcher for somebody reading the patch. I think it makes sense if the code will exist in that maybe-unused state for a while, but here it's just going away immediately anyway. I dunno.

Otherwise segfault if the client just says "session-id". :)

To be clear, the old code behaved the same way. It's just that has_capability() returned the empty string for this case instead of NULL. I changed get_capability() to distinguish the two so that the later fixes for "command=ls-refs=whatever" could treat them differently.

I didn't add tests for this case (nor for "object-format" without a value), but we could do that if anybody cares.

-Peff

We collect the set of capabilities the client sends us in a strvec. While this is usually small, there's no limit to the number of capabilities the client can send us (e.g., they could just send us "agent" pkt-lines over and over, and we'd keep adding them to the list).

Since all code has been converted away from using this list, let's get rid of it. This avoids a potential attack where clients waste our memory.

Note that we do have to replace it with a flag, because some of the flush-packet logic checks whether we've seen any valid commands or keys.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:

...because now we only bother to tell capabilities about information the client sent as it happened, instead of accumulating an unbounded set of strings together into a single strvec.

Makes sense.

Nit; unnecessary whitespace change (but obviously not worth a re-roll on its own).

The rest of this change looks obviously good to me.

Thanks, Taylor

On Tue, Sep 14, 2021 at 12:59:18PM -0400, Taylor Blau wrote:

Right. By the way, one thing I noticed while working on this (that is unchanged by my series) is that some capabilities are ignored. That make sense for "agent", where the primary goal is telling the client about our version (and letting them speak their version, which might be helpful for debug logs but doesn't impact the protocol itself). But we do nothing at all with "server-option".

The client wouldn't send one by default, but we do support "git fetch --server-option". So I assume that it would be a sort of "I'll tell you this string, and you might feed it to hooks, etc" function, like push-options. But we just throw it away (well, before my series we stuffed it into a strvec that nobody looked at).

So possibly we could stop advertising it, but I wonder if any clients would get unhappy.

I also wonder if jgit uses it. Grepping in jgit.git shows that it is recorded but I don't see anybody doing anything useful with it; but maybe some user of the library does.

Thanks. While this section ended up quite simple, it got rebased a whole lot of times as I figured out all of quirks that led to the final protocol-tightening patches. :)

I don't know how I missed the extra line on my final read-through.

-Peff

Because each "ref-prefix" capability from the client comes in its own pkt-line, there's no limit to the number of them that a misbehaving client may send. We read them all into a strvec, which means the client can waste arbitrary amounts of our memory by just sending us "ref-prefix foo" over and over.

One possible solution is to just drop the connection when the limit is reached. If we set it high enough, then only misbehaving or malicious clients would hit it. But "high enough" is vague, and it's unfriendly if we guess wrong and a legitimate client hits this.

But we can do better. Since supporting the ref-prefix capability is optional anyway, the client has to further cull the response based on their own patterns. So we can simply ignore the patterns once we cross a certain threshold. Note that we have to ignore _all_ patterns, not just the ones past our limit (since otherwise we'd send too little data).

The limit here is fairly arbitrary, and probably much higher than anyone would need in practice. It might be worth limiting it further, if only because we check it linearly (so with "m" local refs and "n" patterns, we do "m * n" string comparisons). But if we care about optimizing this, an even better solution may be a more advanced data structure anyway.

I didn't bother making the limit configurable, since it's so high and since Git should behave correctly in either case. It wouldn't be too hard to do, but it makes both the code and documentation more complex.

Signed-off-by: Jeff King <peff@peff.net>
---
We're perhaps bending "optional" a little here. The client does know if
we said "yes, we support ref-prefix" and until now, that meant they
could trust us to cull. But no version of Git has ever relied on that
(we tell the transport code "if you can limit by these prefixes, go for
it" but then just post-process the result).

The other option is that we could just say "no, you're sending too many prefixes" and hangup. This seemed friendlier to me (though either way, I really find it quite unlikely anybody would legitimately hit this limit).

 ls-refs.c            | 19 +++++++++++++++++--
 t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:

Right, because each ref-prefix line *adds* references to the advertised set instead of culling them out. So as soon as we start ignoring even a single ref-prefix line, we have to ignore all of them. Makes sense.

Agreed. I don't think it's worth making it configurable because the limit is so absurdly high that probably nobody will ever want to tweak it.

FWIW, either (dropping the connection or the approach you took here) would have been fine with me, but I find it unlikely that any real users will notice ;).

The order of this if-statement is a little odd to me, but obviously correct. I might have wrote:

    if (too_many_prefixes)
      continue;

    if (data.prefixes.nr < MAX_ALLOWED_PREFIXES) {
      strvec_push(&data.prefixes, out);
    } else {
      too_many_prefixes = 1;
      strvec_clear(&data.prefixes);
    }

But certainly what you wrote here works just fine (so this is a cosmetic comment, and not a functional one).

You could have written this as a loop over the unmatched prefixes, but I vastly prefer the result you came up with, which is much more explicit and doesn't require readers to parse out what the loop does.

So this part looks very good to me.

Thanks, Taylor

On Tue, Sep 14, 2021 at 01:18:06PM -0400, Taylor Blau wrote:

My view of it was: check every case that may avoid us pushing a prefix, and then finally push one. But that may have been related to my goal in writing the patch. :)

I actually think that:

  git for-each-ref --format='%(objectname) %(refname)' >expect

is pretty readable (and is much more efficient, and nicely avoids the master/main brittle-ness, which I ran into while backporting this). But:

  - this matches what the rest of the script does

  - for-each-ref doesn't report on HEAD, so we have to add that in
    separately

  - the "pkt-line unpack" will include the flush packet, so we'd have to
    add that in, too.

-Peff

On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:

The limit I picked is 65536, because it seemed round and high. But note that somebody can put up to almost-64k in a single ref-prefix line, which means ultimately you can allocate 4GB. I do wonder if dropping this to something like 1024 might be reasonable.

In practice I'd expect it to be a handful in most cases (refs/heads/*, refs/tags/*, HEAD). But if you do something like:

  git fetch $remote 1 2 3 4 5 6 7 ...

then we'll prefix-expand those names with the usual lookup rules into refs/1, refs/heads/1, refs/2, refs/heads/2, and so on.

At some point it becomes silly and works counter to the purpose of the optimization (you send more prefix constraints than the actual ref advertisement, not to mention that client bandwidth may not be symmetric). I'm not sure what we want to declare as a reasonable limit.

And this is just about protecting the server; probably it makes sense for the client to realize it's going to send a ridiculous number of prefixes and just skip the feature entirely (since that's what actually saves the bandwidth).

-Peff

On Tue, 14 Sept 2021 at 17:38, Jeff King <peff@peff.net> wrote:

This all makes sense to me. At some point, we should be able to go "I don't know what you're trying to do, but let me just ignore all this craziness and instead try to give you a useful result sooner rather than later".

I do wonder if we should document that the client can't trust us to actually do all this culling. In general, I find that it's a matter of hygiene for the client to do its own checks, but with this change they actually *need* to do them. (Unless they know our limit and that they're on the right side of it, but that kind of magic is even less hygienic.)

Is it easier to reason about with something like this (whitespace-damaged) on top?

On Tue, Sep 14, 2021 at 09:06:55PM +0200, Martin Ågren wrote:

Perhaps we could say so more explicitly in the v2 protocol spec. I'll take a look.

You're the second person to complain about this if-else chain. I'll take the hint. ;)

Hmm. At first I liked this, because it reduces the number of cases (and variables!). But there's something really subtle going on here. I thought at first it should be "<", but you are intentionally over-allocating by one entry to indicate the overflow. I.e., you've essentially stuffed the too_many_prefixes boolean into the count.

This is far from the parser, but I think that's OK. I'd probably couple it with a comment explaining why we need to clear rather than using what we got.

At first I thought this was just being cute, but it's an attempt to compensate for the off-by-one subtlety in the early check. I'll give it some thought. I kind of like it, but the fact that it took me a minute or three to be sure the code is correct makes me worried it's being too clever.

-Peff

On Tue, Sep 14, 2021 at 11:37:06AM -0400, Jeff King wrote:

Yuck. While double-checking some refactoring, I realized this test does not actually generate the correct input!

It omits the "ref-prefix" header. _And_ it accidentally expands $_ in the shell rather than in perl.

The test does work once corrected. And in fact I had originally written it correctly as a "test_seq | sed" pipeline, but generating 64k+ lines in the shell seemed a bit much (we do avoid sub-processes, but the "set -x" output is unwieldy).

I'll fix it in a re-roll.

-Peff

On Tue, Sep 14, 2021 at 06:09:45PM -0400, Jeff King wrote:

Hah, nice find. You'd think that one of us would have caught it earlier given that we both discussed it.

Thanks, Taylor

On Tue, Sep 14, 2021 at 06:11:32PM -0400, Taylor Blau wrote:

Really, I'd have thought that ls-refs would complain about a totally bogus capability. I'll see if I can fix that, as well.

-Peff

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 we use the same parser that checks for regular capabilities like "object-format=sha256". And so we'll accept "ls-refs=foo", even though everything after the equals is bogus, and simply ignored.

This isn't really hurting anything, but the request 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(-)

On Tue, Sep 14, 2021 at 11:37:21AM -0400, Jeff King wrote:

Yeah. I seriously doubt that anybody is relying on this obviously broken behavior, and it's definitely a spec violation. So I have no trouble rejecting 'command=ls-refs=foo' over the wire.

Thanks, Taylor

Our table of v2 "capabilities" contains everything we might tell the client we support. But there are differences in how we expect the client to respond. Some of the entries are true capabilities (i.e., we expect the client to say "yes, I support this"), and some are ones we expect them to send as commands (with "command=ls-refs" or similar).

When we receive a capability used as a command, we complain about that. But when we receive a command used as a capability (e.g., just "ls-refs" in a pkt-line by itself), we silently ignore it.

This isn't really hurting anything (clients shouldn't send it, and we'll ignore it), but we can tighten up the protocol to match what we expect to happen.

There are two new tests here. The first one checks a capability used as a command, which already passes. The second tests a command as a capability, which this patch fixes.

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

On Tue, Sep 14, 2021 at 11:29:46AM -0400, Jeff King wrote:

Thanks. I reviewed this series carefully and all of my comments were either of the form "you could have written it this way, but I'm equally happy with what you wrote here" or "this behavior change won't affect real users and so I'm OK with it".

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Jeff King <peff@peff.net> writes:

The title of the topic says "reducing", but it smells more to me like this is about "limiting" allocations to protect ourselves. Am I reading the series correctly?

Your cover letters (as proposed log messages for individual commits) are always to the point and very pleasant to read. Very much appreciated.

Thanks.

On Tue, Sep 14, 2021 at 11:00:56AM -0700, Junio C Hamano wrote:

Yeah, I think "limiting" is probably a better word. We will still allocate for ref-prefix, of course, but we'll limit the number of allocations. We did "reduce" the number of spots that allocate, since capabilities no longer do so. :)

But I think your understanding of the series is correct.

-Peff

Here's a re-roll of my series to limit the memory a v2 server is willing to allocate on behalf of a client. See v1:

  https://lore.kernel.org/git/YUC%2F6n1hhUbMJiLw@coredump.intra.peff.net/

for an overview. The existing patches are mostly small fixups pointed out by reviewers (thanks!), but I did take Martin's TOO_MANY_PREFIXES suggestion in patch 7 (without the change to the name of the constant it seemed too clever to me, but with it it seems just the right amount of clever).

There are two new patches here:

 - patch 8 explicitly documents the v2 ref-prefix "best effort" behavior

 - patch 11 teaches ls-refs to reject bogus arguments, which violates
   the spec (the current behavior caused a broken test to go unnoticed)

Full range-diff is below the diffstat.

  [01/11]: serve: rename is_command() to parse_command()
  [02/11]: serve: return capability "value" from get_capability()
  [03/11]: serve: add "receive" method for v2 capabilities table
  [04/11]: serve: provide "receive" function for object-format capability
  [05/11]: serve: provide "receive" function for session-id capability
  [06/11]: serve: drop "keys" strvec
  [07/11]: ls-refs: ignore very long ref-prefix counts
  [08/11]: docs/protocol-v2: clarify some ls-refs ref-prefix details
  [09/11]: serve: reject bogus v2 "command=ls-refs=foo"
  [10/11]: serve: reject commands used as capabilities
  [11/11]: ls-refs: reject unknown arguments

 Documentation/technical/protocol-v2.txt |   6 +-
 ls-refs.c                               |  22 ++++-
 serve.c                                 | 119 +++++++++++++-----------
 t/t5701-git-serve.sh                    |  73 +++++++++++++++
 4 files changed, 162 insertions(+), 58 deletions(-)

 1:  eb8e7b21a1 =  1:  eb8e7b21a1 serve: rename is_command() to parse_command()
 2:  8cc66cae41 =  2:  8cc66cae41 serve: return capability "value" from get_capability()
 3:  3343f9bb0a =  3:  3343f9bb0a serve: add "receive" method for v2 capabilities table
 4:  c4cc80fe7a !  4:  0319b69881 serve: provide "receive" function for object-format capability
    @@ Commit message
         all capabilities (because they might not have sent an object-format line
         at all, and we still have to check that the default matches our
         repository algorithm). Since the check_algorithm() function would now be
    -    done to a single if() statement, I've just inlined it in its only
    +    down to a single if() statement, I've just inlined it in its only
         caller.
     
         There should be no change of behavior here, except for two
 5:  c8527ca5a7 =  5:  efe207c35c serve: provide "receive" function for session-id capability
 6:  250e4723ba !  6:  463aa7faa3 serve: drop "keys" strvec
    @@ serve.c: static int process_request(void)
      			else
      				die("unknown capability '%s'", reader.line);
      
    -+
    - 			/* Consume the peeked line */
    - 			packet_reader_read(&reader);
    - 			break;
     @@ serve.c: static int process_request(void)
      			 * If no command and no keys were given then the client
      			 * wanted to terminate the connection.
 7:  1218d62247 !  7:  da2043f42f ls-refs: ignore very long ref-prefix counts
    @@ ls-refs.c: static void ensure_config_read(void)
      }
      
     +/*
    -+ * The maximum number of "ref-prefix" lines we'll allow the client to send.
    -+ * If they go beyond this, we'll avoid using the prefix feature entirely.
    ++ * If we see this many or more "ref-prefix" lines from the client, we consider
    ++ * it "too many" and will avoid using the prefix feature entirely.
     + */
    -+#define MAX_ALLOWED_PREFIXES 65536
    ++#define TOO_MANY_PREFIXES 65536
     +
      /*
       * Check if one of the prefixes is a prefix of the ref.
       * If no prefixes were provided, all refs match.
    -@@ ls-refs.c: static int ls_refs_config(const char *var, const char *value, void *data)
    - int ls_refs(struct repository *r, struct packet_reader *request)
    - {
    - 	struct ls_refs_data data;
    -+	int too_many_prefixes = 0;
    - 
    - 	memset(&data, 0, sizeof(data));
    - 	strvec_init(&data.prefixes);
     @@ ls-refs.c: int ls_refs(struct repository *r, struct packet_reader *request)
      			data.peel = 1;
      		else if (!strcmp("symrefs", arg))
      			data.symrefs = 1;
     -		else if (skip_prefix(arg, "ref-prefix ", &out))
     -			strvec_push(&data.prefixes, out);
     +		else if (skip_prefix(arg, "ref-prefix ", &out)) {
    -+			if (too_many_prefixes) {
    -+				/* ignore any further ones */
    -+			} else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {
    -+				strvec_clear(&data.prefixes);
    -+				too_many_prefixes = 1;
    -+			} else {
    ++			if (data.prefixes.nr < TOO_MANY_PREFIXES)
     +				strvec_push(&data.prefixes, out);
    -+			}
     +		}
      		else if (!strcmp("unborn", arg))
      			data.unborn = allow_unborn;
      	}
    + 
    + 	if (request->status != PACKET_READ_FLUSH)
    + 		die(_("expected flush after ls-refs arguments"));
    + 
    ++	/*
    ++	 * If we saw too many prefixes, we must avoid using them at all; as
    ++	 * soon as we have any prefix, they are meant to form a comprehensive
    ++	 * list.
    ++	 */
    ++	if (data.prefixes.nr >= TOO_MANY_PREFIXES)
    ++		strvec_clear(&data.prefixes);
    ++
    + 	send_possibly_unborn_head(&data);
    + 	if (!data.prefixes.nr)
    + 		strvec_push(&data.prefixes, "");
     
      ## t/t5701-git-serve.sh ##
     @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '
    @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '
      
     +test_expect_success 'ignore very large set of prefixes' '
     +	# generate a large number of ref-prefixes that we expect
    -+	# to match nothing; the value here exceeds MAX_ALLOWED_PREFIXES
    ++	# to match nothing; the value here exceeds TOO_MANY_PREFIXES
     +	# from ls-refs.c.
     +	{
     +		echo command=ls-refs &&
     +		echo object-format=$(test_oid algo)
     +		echo 0001 &&
    -+		perl -le "print \"refs/heads/$_\" for (1..65536+1)" &&
    ++		perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
     +		echo 0000
     +	} |
     +	test-tool pkt-line pack >in &&
 -:  ---------- >  8:  ee540a4ef7 docs/protocol-v2: clarify some ls-refs ref-prefix details
 8:  b1567fdc82 =  9:  481c07cfac serve: reject bogus v2 "command=ls-refs=foo"
 9:  9786b9a11f = 10:  dff965c1d2 serve: reject commands used as capabilities
 -:  ---------- > 11:  f7339f924b ls-refs: reject unknown arguments

The is_command() function not only tells us whether the pktline is a valid command string, but it also parses out the command (and complains if we see a duplicate). Let's rename it to make those extra functions a bit more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

When the client sends v2 capabilities, they may be simple, like:

  foo

or have a value like:

  foo=bar

(all of the current capabilities actually expect a value, but the protocol allows for boolean ones).

We use get_capability() to make sure the client's pktline matches a capability. In doing so, we parse enough to see the "=" and the value (if any), but we immediately forget it. Nobody cares for now, because they end up parsing the values out later using has_capability(). But in preparation for changing that, let's pass back a pointer so the callers know what we found.

Note that unlike has_capability(), we'll return NULL for a "simple" capability. Distinguishing these will be useful for some future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

We have a capabilities table that tells us what we should tell the client we are capable of, and what to do when a client gives us a particular command (e.g., "command=ls-refs"). But it doesn't tell us what to do when the client sends us back a capability (e.g., "object-format=sha256"). We just collect them all in a strvec and hope somebody can use them later.

Instead, let's provide a function pointer in the table to act on these. This will eventually help us avoid collecting the strings, which will be more efficient and less prone to mischief.

Using the new method is optional, which helps in two ways:

  - we can move existing capabilities over to this new system gradually
    in individual commits

  - some capabilities we don't actually do anything with anyway. For
    example, the client is free to say "agent=git/1.2.3" to us, but we
    do not act on the information in any way.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

On Tue, Sep 14 2021, Jeff King wrote:

I haven't actually run this yet (and need to zZzZ soon), but AFAICT at the end of this series you leave the existing advertise semantics of advertise() be (which is fine). I have this unsubmitted patch locally which you may or may not want to work into this.

I considered splitting up the advertise() method as well, i.e. we could have a new "is_advertised" boolean callback, and then a "capability_string" or whatever. "server-option" and "object-info" never add anything, so they could leave that as NULL.

But it's probably not worth it, just food for thought...

-- >8 -- serve: document that "advertise" is called in two modes

If we're being called with a non-NULL argument we're sending out the advertisement line, but if it's NULL we're actually in the middle of a request.

So you can use the check for NULL to emit your own "die" on "return 0", like "wtf, I said I don't support command xyz, why are you calling it?", as opposed to the default "invalid command '%s'".

Maybe nobody cares, and we can't assume that we're going from an advertisement to a command for the same request anyway (can we?). I.e. are we serving multiple clients?

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Wed, Sep 15, 2021 at 02:31:28AM +0200, Ævar Arnfjörð Bjarmason wrote:

Yes, I noticed the advertise() function is really doing double-duty here. Nothing changes with respect to it in my series, but I agree it's a bit confusing.

The functions are simple enough that splitting up the two functions of advertise() might make sense. Likewise, your documentation seems reasonable. I'd prefer to punt on it for now, though, as my series isn't otherwise touching this function.

I'm sure there are textual conflicts with what I'm doing here, since it's nearby, but I'd prefer to just build more cleanups on top later.

-Peff

Jeff King <peff@peff.net> writes:

What does "as a non-command" mean? To put it another way, when a client requests the capability as a command, what does the receive method do differently?

The comment tentatively gets slightly stale here, but that will be corrected at the end, so it would be fine ;-)

On Wed, Sep 15, 2021 at 09:41:28AM -0700, Junio C Hamano wrote:

For each entry in this capability table, clients can say:

  command=foo

or just:

  foo

The latter is a non-command. The "receive" function is not called at all if it is a "command". I think this is a bit more clear when read together with the existing function just above (which you can't see in the diff context):

        /*
         * Function called when a client requests the capability as a command.
         * Will be provided a struct packet_reader 'request' which it should
         * use to read the command specific part of the request.  Every command
         * MUST read until a flush packet is seen before sending a response.
         *
         * This field should be NULL for capabilities which are not commands.
         */
        int (*command)(struct repository *r, struct packet_reader *request);

I guess these could define "as a command", but I think it's pretty clear in the context.

Hmm. I think it is not stale here, as we are still collecting the "keys" strvec. But it _does_ get stale by the end, when we stop doing so.

I'm preparing a re-roll for the test issues Eric noted, so I'll drop that comment in the appropriate patch.

-Peff

We get any "object-format" specified by the client by searching for it in the collected list of capabilities the client sent. We can instead just handle it as soon as they send it. This is slightly more efficient, and gets us one step closer to dropping that collected list.

Note that we do still have to do our final hash check after receiving all capabilities (because they might not have sent an object-format line at all, and we still have to check that the default matches our repository algorithm). Since the check_algorithm() function would now be down to a single if() statement, I've just inlined it in its only caller.

There should be no change of behavior here, except for two broken-protocol cases:

  - if the client sends multiple conflicting object-format capabilities
    (which they should not), we'll now choose the last one rather than
    the first. We could also detect and complain about the duplicates
    quite easily now, which we could not before, but I didn't do so
    here.

  - if the client sends a bogus "object-format" with no equals sign,
    we'll now say so, rather than "unknown object format: ''"

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Jeff King <peff@peff.net> writes:

I'd imagine that it would have been easy to retain the "use the first one" behaviour if we wanted to; I do not think this behaviour change really matters in practice, so it's OK.

OK.

OK.

Rather than pulling the session-id string from the list of collected capabilities, we can handle it as soon as we receive it. This gets us closer to dropping the collected list entirely.

The behavior should be the same, with one exception. Previously if the client sent us multiple session-id lines, we'd report only the first. Now we'll pass each one along to trace2. This shouldn't matter in practice, since clients shouldn't do that (and if they do, it's probably sensible to log them all).

As this removes the last caller of the static has_capability(), we can remove it, as well (and in fact we must to avoid -Wunused-function complaining).

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

Jeff King <peff@peff.net> writes:

Nice.

Because each "ref-prefix" capability from the client comes in its own pkt-line, there's no limit to the number of them that a misbehaving client may send. We read them all into a strvec, which means the client can waste arbitrary amounts of our memory by just sending us "ref-prefix foo" over and over.

One possible solution is to just drop the connection when the limit is reached. If we set it high enough, then only misbehaving or malicious clients would hit it. But "high enough" is vague, and it's unfriendly if we guess wrong and a legitimate client hits this.

But we can do better. Since supporting the ref-prefix capability is optional anyway, the client has to further cull the response based on their own patterns. So we can simply ignore the patterns once we cross a certain threshold. Note that we have to ignore _all_ patterns, not just the ones past our limit (since otherwise we'd send too little data).

The limit here is fairly arbitrary, and probably much higher than anyone would need in practice. It might be worth limiting it further, if only because we check it linearly (so with "m" local refs and "n" patterns, we do "m * n" string comparisons). But if we care about optimizing this, an even better solution may be a more advanced data structure anyway.

I didn't bother making the limit configurable, since it's so high and since Git should behave correctly in either case. It wouldn't be too hard to do, but it makes both the code and documentation more complex.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            | 20 ++++++++++++++++++--
 t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

On Tue, Sep 14, 2021 at 07:51:35PM -0400, Jeff King wrote:

Great, I find this version even better than what I suggested where the case where the list already has too many prefixes `continue`s through the loop before calling strvec_add().

Just noting aloud, the `>` part of this conditional will never happen (since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but never larger than it).

Obviously not wrong, but I figured it'd be worth mentioning in case it caught the attention of other reviewers.

Thanks, Taylor

On Wed, Sep 15, 2021 at 12:16:04AM -0400, Taylor Blau wrote:

Yeah, your analysis is right. Long ago I read some advice (I think from K&R, either the C book or a related article) that suggested always using bounding checks like this rather than equality, because they do the right thing even if the variable ends up with a surprising value.

I can certainly see an argument against it (like that if you're so worried about it, maybe you ought to detect it and figure out who is setting the variable to be unexpectedly high). But it seems like a reasonable defensive measure to me (against an off-by-one mis-analysis, or against the earlier code changing in the future).

-Peff

On Tue, Sep 14, 2021 at 7:51 PM Jeff King <peff@peff.net> wrote:

Broken &&-chain in {...}.

(Granted, it doesn't matter in this case since the exit code gets lost down the pipe, but better to be consistent about it.)

On Wed, Sep 15, 2021 at 01:00:01AM -0400, Eric Sunshine wrote:

Thanks, will fix.

Yep. I think what happened is that I started to convert this to a here-doc to match the nearby tests, but then realized that expanding the multi-line bit was weird (even more so when it was a shell loop with a pipe, before I switched it to perl).

-Peff

We collect the set of capabilities the client sends us in a strvec. While this is usually small, there's no limit to the number of capabilities the client can send us (e.g., they could just send us "agent" pkt-lines over and over, and we'd keep adding them to the list).

Since all code has been converted away from using this list, let's get rid of it. This avoids a potential attack where clients waste our memory.

Note that we do have to replace it with a flag, because some of the flush-packet logic checks whether we've seen any valid commands or keys.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

OK, we no longer "collect" request in the keys strvec, but I guess what receive_client_capability() does still counts as "collecting", so the "tentatively stale" comment is not wrong after all at the end (we have tentatively been collecting in two different places and one of them is dropped here).

Nice.

We've never documented the fact that a client can provide multiple ref-prefix capabilities. Let's describe the behavior.

We also never discussed the "best effort" nature of the prefixes. The client side of git.git has always treated them this way, filtering the result with local patterns. And indeed any client must do this, because the prefix patterns are not sufficient to express the usual refspecs (and so for "foo" we ask for "refs/heads/foo", "refs/tags/foo", and so on).

So this may be considered a change in the spec with respect to client expectations / requirements, but it's mostly codifying existing behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/protocol-v2.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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 we use the same parser that checks for regular capabilities like "object-format=sha256". And so we'll accept "ls-refs=foo", even though everything after the equals is bogus, and simply ignored.

This isn't really hurting anything, but the request 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(-)

On Tue, Sep 14 2021, Jeff King wrote:

Doesn't need a re-roll, but just for my own sanity:

By violating the spec you mean it doesn't coform to the "key" in the "Capability Advertisement" section of protocol-v2.txt, one might skim this and think values with "=" in them are OK, but a command=WHATEVER has a WHATEVER as a "key", not a "value", that's for other parts of the dialog.

But you could also have meant that it violates the spec because there's no such command as "ls-refs=whatever", just like there isn't "foobar", but I don't think that's what you mean...

On Wed, Sep 15, 2021 at 02:27:35AM +0200, Ævar Arnfjörð Bjarmason wrote:

Both, I think. :) The line "command=foo=bar" is obvious nonsense by the spec, no matter which way you interpret it:

  - "foo" is a command name, but adding a value does not make sense when
    it is used in this context, so it's invalid

  - "foo=bar" is not a command name

Our parser does pick out "foo", so in terms of the code it is more like the former. But since it is invalid either way, I'm not sure the distinction matters.

-Peff

On Tue, Sep 14, 2021 at 7:52 PM Jeff King <peff@peff.net> wrote:

This here-doc uses <<-\EOF yet (presumably) expects $(test_oid algo) to be expanded. I'm guessing that you meant <<-EOF but never noticed...

... because of this test_must_fail().

On Wed, Sep 15, 2021 at 01:09:19AM -0400, Eric Sunshine wrote:

Ah, thanks. It's muscle memory to inhibit expansion in my here-docs. :)

Actually, it's a little more complicated. The "grep" here would notice if we failed for the wrong reason. But we never actually look at the object-format line at all, because we barf as soon as we see the bogus command line.

So the object-format line is superfluous in this test. I added it mostly for consistency with the other tests (including the existing unknown-command test, which is in the same boat). But it also makes the test slightly more robust, in that the command is more likely to succeed (and thus fail the test) if the code accidentally did not notice the wrong command line.

But obviously it's worth fixing. Thanks for noticing.

-Peff

Jeff King <peff@peff.net> writes:

Maybe I am slow but I had to read the above a few times and finally look at the implementation of parse_command() to realize that what the last sentence describes is:

    When parse_command() is fed "command=ls-refs=foo", it strips
    "command=", feeds "ls-refs=foo" to get_capability(), and because
    we do not ensure value is NULL, we silently ignore "=foo" that
    is bogus.

And it makes sense. It would probably have helped if I peeked the updated test ;-)

Good.

On Wed, Sep 15, 2021 at 10:33:32AM -0700, Junio C Hamano wrote:

Since I'm re-rolling anyway, I'll expand it a bit (and also cover Ævar's "what exactly does violate mean here" question).

-Peff

Our table of v2 "capabilities" contains everything we might tell the client we support. But there are differences in how we expect the client to respond. Some of the entries are true capabilities (i.e., we expect the client to say "yes, I support this"), and some are ones we expect them to send as commands (with "command=ls-refs" or similar).

When we receive a capability used as a command, we complain about that. But when we receive a command used as a capability (e.g., just "ls-refs" in a pkt-line by itself), we silently ignore it.

This isn't really hurting anything (clients shouldn't send it, and we'll ignore it), but we can tighten up the protocol to match what we expect to happen.

There are two new tests here. The first one checks a capability used as a command, which already passes. The second tests a command as a capability, which this patch fixes.

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

The v2 ls-refs command may receive extra arguments from the client, one per pkt-line. The spec is pretty clear that the arguments must come from a specified set, but we silently ignore any unknown entries. For a well-behaved client this doesn't matter, but it makes testing and debugging more confusing. Let's tighten this up to match the spec.

In theory this liberal behavior _could_ be useful for extending the protocol. But:

  - every other part of the protocol requires that the server first
    indicate that it supports the argument; this includes the fetch and
    object-info commands, plus the "unborn" capability added to ls-refs
    itself

  - it's not a very good extension mechanism anyway; without the server
    advertising support, clients would have no idea if the argument was
    silently ignored, or accepted and simply had no effect

So we're not really losing anything by tightening this.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            |  2 ++
 t/t5701-git-serve.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+)

On Tue, Sep 14 2021, Jeff King wrote:

Looks good. For what it's worth some of this series is stuff I've been meaning to submit after my current in-flight series, so I guess we'll be playing some rebase ping-pong in this area.

In my version of this I'd led with a change to do changes like these for
all the protocol verbs:
	
	diff --git a/connect.c b/connect.c
	index 70b13389ba5..5e991a92279 100644
	--- a/connect.c
	+++ b/connect.c
	@@ -514,6 +514,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
	        packet_write_fmt(fd_out, "symrefs\n");
	        if (server_supports_feature("ls-refs", "unborn", 0))
	                packet_write_fmt(fd_out, "unborn\n");
	+       if (git_env_bool("GIT_TEST_PROTOCOL_BAD_LS_REFS", 0))
	+               packet_write_fmt(fd_out, "test-bad-client\n");
	        for (i = 0; ref_prefixes && i < ref_prefixes->nr; i++) {
	                packet_write_fmt(fd_out, "ref-prefix %s\n",
	                                 ref_prefixes->v[i]);

So the below patch isn't anywhere near applying, but you can see the test coverage (those tests are new) & what changes if we instrument a client to actually send this bad data.

That packet_client_error() function is new, part of what I'm doing there is converting all of this error emitting from die() to properly sending ERR packets.

I think a bug in your versio is that you're using _() here, if your server program happens to be started in Chinese you probably still want to emit errors to clients in LANG=C.

That's why I'm using N_() in my version, it knows to emit the die() localized, but the ERR packet in LANG=C, no you'll get the i18n message in the server terminal, but the client gets LANG=C.

Of course that actually working is subject to various races that I may or may not be able to untangle...

-- >8 -- protocol v2: correct & adjust "ls-refs" ERR behavior

Change the protocol v2 "ls-refs" to error out on unknown arguments, before this we'd silently ignore unknown arguments.

This brings us in line with the behavior we've had in "fetch" (i.e. upload-pack.c) since 3145ea957d2 (upload-pack: introduce fetch server command, 2018-03-15) (see the "/* ignore unknown lines maybe? */" comment in "process_args()".

The looser behavior in "ls-refs" seems to have been unintentionally added in 72d0ea0056b (ls-refs: introduce ls-refs server command, 2018-03-15).

I've also changed the wording of the "expected flush" message in "ls-refs" to be consistent with "fetch" and "object-info".

In the changes leading up to this we had to grep out the racy "fatal: the remote end hung up unexpectedly" messages. Now we need to guard the full test_cmp with a test_expect_failure. I could also write a looser test, but this makes subsequent changes smaller, and is more accurate.

Most of the time the test_expect_failure() succeeds, but in some cases the two messages will be out-of-order, or e.g. only the STDERR one will make it, and not the ERR message, or the other way around. I.e. now that we emit both an ERR line and die() on the remote, we might see ERR before die(), or die() before ERR.

This race does not happen in the t5701-git-serve.sh and other "test-tool pkt-line" tests, which only use one process.

Ideally we should only emit ERR and not also something on STDERR when we die() on the server. Subsequent commits will address that, but for now let's keep that behavior. I.e. we'll now emit two mostly duplicate errors in some cases from the client, or one where we emitted none (or one that made no sense) before.

The current client implementation doesn't expect the server to be this overly helpful in giving us errors both via pkt-line and STDERR, so we usually get two lines, and in some cases just one (but never zero).

Subsequent commits will address this. I.e. we can assume that the client can format the ERR lines, and that we can just exit(128) or disconnect on the server, not die().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c                      |  6 +++++-
 t/t5703-protocol-v2-file.sh    | 24 +++++++++++++++++-------
 t/t5704-protocol-v2-git.sh     | 20 +++++++++++++-------
 t/t5705-protocol-v2-http.sh    | 25 +++++++++++++++++++------
 t/t5710-protocol-violations.sh |  2 +-
 5 files changed, 55 insertions(+), 22 deletions(-)

On Wed, Sep 15, 2021 at 02:09:16AM +0200, Ævar Arnfjörð Bjarmason wrote:

Right, I noticed that none of this is likely to make it to a client (unless they are using file:// or an ssh channel which passes back stderr).

I agree that we should probably be passing these back via ERR packets, but note that the client is racy here. If the server reports an error and dies while the client is still writing, they'll see SIGPIPE and exit without showing the user the ERR packet. The solution may be something along these lines:

  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/

Alternatively, the server can pump the client data to /dev/null until we hit a flush, and _then_ write the ERR. But that doesn't work for some protocol-level errors (like "oops, I'm having trouble reading your pkt-lines).

I'm not sure it's a bug. If you set LANG=zh on your server, that might be harmful (if you serve a multi-lingual international audience), or it might be helpful (if it's a company server where everybody speaks the language). Likewise, for a file:// or ssh:// operation, your local LANG would kick in.

I don't really have a strong opinion either way on whether it's helpful or hindering overall. But I did follow the convention of nearby code in this series, so I think we don't need to worry about it now (it also seemed inconsistent to me; serve.c does not mark for translation, but ls-refs.c does).

Oh good, so you know about them. :)

-Peff

On Tue, Sep 14, 2021 at 07:51:06PM -0400, Jeff King wrote:

Thanks for this. The two new patches look good to me, and I took a light skim over the ones which were modified since v1. Everything I saw seemed very reasonable to me.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

OK, hopefully third time's the charm. The changes from v2 are pretty minor:

 - test typos noticed by Eric (none affected the outcome of the tests)

 - I added "object-format=$(test_oid algo)" to the input of a few of the
   new tests even where it doesn't change the outcome, for consistency
   with the existing tests

 - dropped out-dated "collect" comment noticed by Junio

 - explained the command=ls-refs=foo case a little further, including
   specific references in how it violates the spec

Range-diff is below.

  [01/11]: serve: rename is_command() to parse_command()
  [02/11]: serve: return capability "value" from get_capability()
  [03/11]: serve: add "receive" method for v2 capabilities table
  [04/11]: serve: provide "receive" function for object-format capability
  [05/11]: serve: provide "receive" function for session-id capability
  [06/11]: serve: drop "keys" strvec
  [07/11]: ls-refs: ignore very long ref-prefix counts
  [08/11]: docs/protocol-v2: clarify some ls-refs ref-prefix details
  [09/11]: serve: reject bogus v2 "command=ls-refs=foo"
  [10/11]: serve: reject commands used as capabilities
  [11/11]: ls-refs: reject unknown arguments

 Documentation/technical/protocol-v2.txt |   6 +-
 ls-refs.c                               |  22 ++++-
 serve.c                                 | 120 +++++++++++++-----------
 t/t5701-git-serve.sh                    |  75 +++++++++++++++
 4 files changed, 164 insertions(+), 59 deletions(-)

 1:  e642ced1e8 =  1:  71003eb21a serve: rename is_command() to parse_command()
 2:  75d119ae49 =  2:  1e86c31477 serve: return capability "value" from get_capability()
 3:  9fb21cab1d =  3:  12a159d5c8 serve: add "receive" method for v2 capabilities table
 4:  5b661c9ed5 =  4:  f5c29b5cdf serve: provide "receive" function for object-format capability
 5:  34e0ce5c12 =  5:  063cb60d1e serve: provide "receive" function for session-id capability
 6:  8e42fa9aec !  6:  0ed0b946ea serve: drop "keys" strvec
    @@ serve.c: static int process_request(void)
      
      	packet_reader_init(&reader, 0, NULL, 0,
     @@ serve.c: static int process_request(void)
    - 			/* collect request; a sequence of keys and values */
    + 		case PACKET_READ_EOF:
    + 			BUG("Should have already died when seeing EOF");
    + 		case PACKET_READ_NORMAL:
    +-			/* collect request; a sequence of keys and values */
      			if (parse_command(reader.line, &command) ||
      			    receive_client_capability(reader.line))
     -				strvec_push(&keys, reader.line);
 7:  de7ac11ad3 !  7:  a9392d0e68 ls-refs: ignore very long ref-prefix counts
    @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '
     +	# from ls-refs.c.
     +	{
     +		echo command=ls-refs &&
    -+		echo object-format=$(test_oid algo)
    ++		echo object-format=$(test_oid algo) &&
     +		echo 0001 &&
     +		perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
     +		echo 0000
 8:  3f78422fd3 =  8:  0a982f676a docs/protocol-v2: clarify some ls-refs ref-prefix details
 9:  6c55a7412d !  9:  553db6f83e serve: reject bogus v2 "command=ls-refs=foo"
    @@ Commit message
         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 we use the same parser that checks
    -    for regular capabilities like "object-format=sha256". And so we'll
    -    accept "ls-refs=foo", even though everything after the equals is bogus,
    -    and simply ignored.
    +    "command=foo"), we'll reject it.
     
    -    This isn't really hurting anything, but the request does violate the
    -    spec. Let's tighten it up to prevent any surprising behavior.
    +    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 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>
     
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
      '
      
     +test_expect_success 'requested command is command=value' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=ls-refs=whatever
     +	object-format=$(test_oid algo)
     +	0000
10:  bbb12669b9 ! 10:  93216b9eaa serve: reject commands used as capabilities
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
      '
      
     +test_expect_success 'request capability as command' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=agent
    ++	object-format=$(test_oid algo)
     +	0000
     +	EOF
     +	test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&
     +	grep invalid.command.*agent err
     +'
     +
     +test_expect_success 'request command as capability' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=ls-refs
    ++	object-format=$(test_oid algo)
     +	fetch
     +	0000
     +	EOF
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
     +'
     +
      test_expect_success 'requested command is command=value' '
    - 	test-tool pkt-line pack >in <<-\EOF &&
    + 	test-tool pkt-line pack >in <<-EOF &&
      	command=ls-refs=whatever
11:  375a85b9a6 = 11:  30802eeb54 ls-refs: reject unknown arguments

On Wed, Sep 15, 2021 at 02:33:53PM -0400, Jeff King wrote:

Er, that should have been "v3" in the subject, as this one now is.

-Peff

The is_command() function not only tells us whether the pktline is a valid command string, but it also parses out the command (and complains if we see a duplicate). Let's rename it to make those extra functions a bit more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

When the client sends v2 capabilities, they may be simple, like:

  foo

or have a value like:

  foo=bar

(all of the current capabilities actually expect a value, but the protocol allows for boolean ones).

We use get_capability() to make sure the client's pktline matches a capability. In doing so, we parse enough to see the "=" and the value (if any), but we immediately forget it. Nobody cares for now, because they end up parsing the values out later using has_capability(). But in preparation for changing that, let's pass back a pointer so the callers know what we found.

Note that unlike has_capability(), we'll return NULL for a "simple" capability. Distinguishing these will be useful for some future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

We have a capabilities table that tells us what we should tell the client we are capable of, and what to do when a client gives us a particular command (e.g., "command=ls-refs"). But it doesn't tell us what to do when the client sends us back a capability (e.g., "object-format=sha256"). We just collect them all in a strvec and hope somebody can use them later.

Instead, let's provide a function pointer in the table to act on these. This will eventually help us avoid collecting the strings, which will be more efficient and less prone to mischief.

Using the new method is optional, which helps in two ways:

  - we can move existing capabilities over to this new system gradually
    in individual commits

  - some capabilities we don't actually do anything with anyway. For
    example, the client is free to say "agent=git/1.2.3" to us, but we
    do not act on the information in any way.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

We get any "object-format" specified by the client by searching for it in the collected list of capabilities the client sent. We can instead just handle it as soon as they send it. This is slightly more efficient, and gets us one step closer to dropping that collected list.

Note that we do still have to do our final hash check after receiving all capabilities (because they might not have sent an object-format line at all, and we still have to check that the default matches our repository algorithm). Since the check_algorithm() function would now be down to a single if() statement, I've just inlined it in its only caller.

There should be no change of behavior here, except for two broken-protocol cases:

  - if the client sends multiple conflicting object-format capabilities
    (which they should not), we'll now choose the last one rather than
    the first. We could also detect and complain about the duplicates
    quite easily now, which we could not before, but I didn't do so
    here.

  - if the client sends a bogus "object-format" with no equals sign,
    we'll now say so, rather than "unknown object format: ''"

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Rather than pulling the session-id string from the list of collected capabilities, we can handle it as soon as we receive it. This gets us closer to dropping the collected list entirely.

The behavior should be the same, with one exception. Previously if the client sent us multiple session-id lines, we'd report only the first. Now we'll pass each one along to trace2. This shouldn't matter in practice, since clients shouldn't do that (and if they do, it's probably sensible to log them all).

As this removes the last caller of the static has_capability(), we can remove it, as well (and in fact we must to avoid -Wunused-function complaining).

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

We collect the set of capabilities the client sends us in a strvec. While this is usually small, there's no limit to the number of capabilities the client can send us (e.g., they could just send us "agent" pkt-lines over and over, and we'd keep adding them to the list).

Since all code has been converted away from using this list, let's get rid of it. This avoids a potential attack where clients waste our memory.

Note that we do have to replace it with a flag, because some of the flush-packet logic checks whether we've seen any valid commands or keys.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Because each "ref-prefix" capability from the client comes in its own pkt-line, there's no limit to the number of them that a misbehaving client may send. We read them all into a strvec, which means the client can waste arbitrary amounts of our memory by just sending us "ref-prefix foo" over and over.

One possible solution is to just drop the connection when the limit is reached. If we set it high enough, then only misbehaving or malicious clients would hit it. But "high enough" is vague, and it's unfriendly if we guess wrong and a legitimate client hits this.

But we can do better. Since supporting the ref-prefix capability is optional anyway, the client has to further cull the response based on their own patterns. So we can simply ignore the patterns once we cross a certain threshold. Note that we have to ignore _all_ patterns, not just the ones past our limit (since otherwise we'd send too little data).

The limit here is fairly arbitrary, and probably much higher than anyone would need in practice. It might be worth limiting it further, if only because we check it linearly (so with "m" local refs and "n" patterns, we do "m * n" string comparisons). But if we care about optimizing this, an even better solution may be a more advanced data structure anyway.

I didn't bother making the limit configurable, since it's so high and since Git should behave correctly in either case. It wouldn't be too hard to do, but it makes both the code and documentation more complex.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            | 20 ++++++++++++++++++--
 t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

We've never documented the fact that a client can provide multiple ref-prefix capabilities. Let's describe the behavior.

We also never discussed the "best effort" nature of the prefixes. The client side of git.git has always treated them this way, filtering the result with local patterns. And indeed any client must do this, because the prefix patterns are not sufficient to express the usual refspecs (and so for "foo" we ask for "refs/heads/foo", "refs/tags/foo", and so on).

So this may be considered a change in the spec with respect to client expectations / requirements, but it's mostly codifying existing behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/protocol-v2.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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(-)

Our table of v2 "capabilities" contains everything we might tell the client we support. But there are differences in how we expect the client to respond. Some of the entries are true capabilities (i.e., we expect the client to say "yes, I support this"), and some are ones we expect them to send as commands (with "command=ls-refs" or similar).

When we receive a capability used as a command, we complain about that. But when we receive a command used as a capability (e.g., just "ls-refs" in a pkt-line by itself), we silently ignore it.

This isn't really hurting anything (clients shouldn't send it, and we'll ignore it), but we can tighten up the protocol to match what we expect to happen.

There are two new tests here. The first one checks a capability used as a command, which already passes. The second tests a command as a capability, which this patch fixes.

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

The v2 ls-refs command may receive extra arguments from the client, one per pkt-line. The spec is pretty clear that the arguments must come from a specified set, but we silently ignore any unknown entries. For a well-behaved client this doesn't matter, but it makes testing and debugging more confusing. Let's tighten this up to match the spec.

In theory this liberal behavior _could_ be useful for extending the protocol. But:

  - every other part of the protocol requires that the server first
    indicate that it supports the argument; this includes the fetch and
    object-info commands, plus the "unborn" capability added to ls-refs
    itself

  - it's not a very good extension mechanism anyway; without the server
    advertising support, clients would have no idea if the argument was
    silently ignored, or accepted and simply had no effect

So we're not really losing anything by tightening this.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            |  2 ++
 t/t5701-git-serve.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+)

On Tue, Sep 14 2021, Jeff King wrote:

[...]

When I first read this I expected it to be a link to https://lore.kernel.org/git/YPCsDLoiiAG%2FC%2Fft@coredump.intra.peff.net/; i.e. that the object-info leak discussion back in July had encouraged you to work on this ... :)

On Wed, Sep 15, 2021 at 02:25:49AM +0200, Ævar Arnfjörð Bjarmason wrote:

Nope, I got terrified by the "yes | upload-pack" example I showed earlier. :)

I was really worried it could turn into a heap overflow, but it turns out that it cannot. But I still think tightening things up (and avoiding any memory-consumption attacks) is worth doing.

-Peff