Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

Jonathan Nieder [off-list ref] writes:

Hi,

Junio C Hamano wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

quoted

quoted

More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?

Sorry to butt into the conversation late, but: I am not yet convinced.

Is the idea that if the push options were very large, this would save
the client from the cost of sending them?  

Not really.  I have no strong opinion on the benefit of limiting
number/size.  Stefan limited the number/size at the receiving end
and made receiving end die with its message.  I was merely trying to
tweak the arrangement so that the sending end can complain with its
own message, possibly in its own language, especially because it was
unclear to me if the die() message on the receiving end would always
go back to the sending end correctly.

But this comes with a
downside: the server doesn't get to send an error message about
where 
the maximum number of push options can come from (e.g., with a link to
a page where the limit can be adjusted, or with an explanation of when
clients tend to run into this problem and what they should do
instead).

Hmm, interesting point.  That would be better told by the receiving
end, as the way to configure it (if offered) would be different from
installation to installation.

So I'd like to propose an alternative. What if the client tells the
server the number of push options early on (and possibly also a cap on
the length of those push options)?  That way, the client doesn't have
to waste bit sending the push options but the server gets an
opportunity to send a helpful error message on sideband 3.

	server> HEAD\0push-options ...
	client> ... commands ...
	client> push-options 2
	client> my-first-option
	client> my-second-option

On Fri, Jul 8, 2016 at 11:39 AM, Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

Hi,

Junio C Hamano wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

quoted

quoted

More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?

Sorry to butt into the conversation late, but: I am not yet convinced.

Is the idea that if the push options were very large, this would save
the client from the cost of sending them?

Not really.  I have no strong opinion on the benefit of limiting
number/size.  Stefan limited the number/size at the receiving end
and made receiving end die with its message.

Jeff claimed we'd need some sort of DoS protection for this feature,
so I considered just die-ing enough for an initial implementation.

I was merely trying to
tweak the arrangement so that the sending end can complain with its
own message, possibly in its own language, especially because it was
unclear to me if the die() message on the receiving end would always
go back to the sending end correctly.

I'm currently implementing Jonathans suggestion as it seems to be a reasonable
trade off (client hasn't sent a lot of data when it is decided it
doesn't go through,
the server can complain with a reasonable error message, only downside: no
i18n localisation support on the client side as the server will
currently report the
error in English).

That method will make heavy use of rp_error that uses the side band for
communicating the actual error message.

quoted

But this comes with a
downside: the server doesn't get to send an error message about
where
the maximum number of push options can come from (e.g., with a link to
a page where the limit can be adjusted, or with an explanation of when
clients tend to run into this problem and what they should do
instead).

Hmm, interesting point.  That would be better told by the receiving
end, as the way to configure it (if offered) would be different from
installation to installation.

quoted

So I'd like to propose an alternative. What if the client tells the
server the number of push options early on (and possibly also a cap on
the length of those push options)?  That way, the client doesn't have
to waste bit sending the push options but the server gets an
opportunity to send a helpful error message on sideband 3.

      server> HEAD\0push-options ...
      client> ... commands ...
      client> push-options 2
      client> my-first-option
      client> my-second-option

Another (slightly offtopic) observation:
If in the future we'll need to transmit push options >64k, instead of
splitting the push option to multiple packets, we could invent "large
packets". The current upper bound for packets is artificailly low, such
that the server is able to interleave sideband information with the actual
data in a fetch and have the client display the progress in a timely manner.
When pushing to the server we'd not need to have progress information
(the server doesn't care, and the client knows the size it is pushing).

As of today a packet consists of 4 bytes (hex characters) to indicate
the length and then the payload follows. So instead we could transmit
"v" (that is not a hex character) followed by a variable length integer for
the length and then the payload which has no upper bound.

In the release notes for 2.3 you wrote:

* "git push" and "git fetch" did not communicate an overlong refname
  correctly.  Now it uses 64kB sideband to accommodate longer ones.

That could also make use of these "large packets" instead.

Thanks,
Stefan

On Fri, Jul 08, 2016 at 11:57:20AM -0700, Stefan Beller wrote:

quoted

quoted

Sorry to butt into the conversation late, but: I am not yet convinced.

Is the idea that if the push options were very large, this would save
the client from the cost of sending them?

Not really.  I have no strong opinion on the benefit of limiting
number/size.  Stefan limited the number/size at the receiving end
and made receiving end die with its message.

Jeff claimed we'd need some sort of DoS protection for this feature,
so I considered just die-ing enough for an initial implementation.

I do not think we need to worry too much about niceties for these
limits. The point is to protect servers from malicious nonsense, like
somebody sending gigabytes of push options, or trying to overflow a
buffer in a hook with a large value. If people are seeing these in
routine use, then the limits are set too low, and this should happen
roughly as often as a BUG assertion, and IMHO should be treated roughly
the same: don't bother with translation, and don't worry about
optimizing wasted bandwidth for this case. It won't happen enough to
matter.

-Peff

On Fri, Jul 8, 2016 at 2:46 PM, Jeff King [off-list ref] wrote:

On Fri, Jul 08, 2016 at 11:57:20AM -0700, Stefan Beller wrote:

quoted

quoted

quoted

Sorry to butt into the conversation late, but: I am not yet convinced.

Is the idea that if the push options were very large, this would save
the client from the cost of sending them?

Not really.  I have no strong opinion on the benefit of limiting
number/size.  Stefan limited the number/size at the receiving end
and made receiving end die with its message.

Jeff claimed we'd need some sort of DoS protection for this feature,
so I considered just die-ing enough for an initial implementation.

I do not think we need to worry too much about niceties for these
limits. The point is to protect servers from malicious nonsense, like
somebody sending gigabytes of push options, or trying to overflow a
buffer in a hook with a large value.

Agreed. This would speak for keeping the implementation as is.

If people are seeing these in
routine use, then the limits are set too low, and this should happen
roughly as often as a BUG assertion, and IMHO should be treated roughly
the same: don't bother with translation, and don't worry about
optimizing wasted bandwidth for this case. It won't happen enough to
matter.

Well the wasted band width is part of the server protection, no?
This would favor the idea Jonathan came up with:

    server: I advertise push options
    client: ok I want to use push options
    client: I'll send you 1000 push options with upper bound of 1000M
    server: It's a bit too much, eh?
    * server quits

So this case only occurs for the (malicious?) corner case, where I
do not bother a translation.

But having the size announcement not in
the capability advertisement, but in the actual push options phase makes
sense to me as we do not want to clutter the capabilities with data that can
come later. We would only waste a little bit of band width, (the
initial ls-remote
and command list of the client).


Speaking of this, I can craft a malicious client that sends the
following command list

0000000000000000000000000000000000000000
0000000000000000000000000000000000000000
refs/heads/loooooooooooooooooong-ref
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000
refs/heads/loooooooooooooooooong-ref
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000
refs/heads/loooooooooooooooooong-ref
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000
refs/heads/loooooooooooooooooong-ref
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000
refs/heads/loooooooooooooooooong-ref
<repeat the above a few times>
0000

IIUC in the receive-pack code we would queue that up and the error checking
(two times null sha1? update of the same ref more than once?), is
done just after we send out the flush packet, i.e. when all commands
are received.

This would also result in sending gigabytes of junk as well as a
memory issue on the server
side?

The new push options design is actually neat in the way that the
client exactly says what it wants
and the server can reject early, but not cluttering the capability
advertisement.

Thanks,
Stefan

On Fri, Jul 08, 2016 at 03:17:13PM -0700, Stefan Beller wrote:

quoted

If people are seeing these in
routine use, then the limits are set too low, and this should happen
roughly as often as a BUG assertion, and IMHO should be treated roughly
the same: don't bother with translation, and don't worry about
optimizing wasted bandwidth for this case. It won't happen enough to
matter.

Well the wasted band width is part of the server protection, no?

Not if you stop receiving as soon as you hit the limits. Then of course
they can send up to the limit each time, but that is not a DoS. That is
things working as advertised.

This would favor the idea Jonathan came up with:

    server: I advertise push options
    client: ok I want to use push options
    client: I'll send you 1000 push options with upper bound of 1000M
    server: It's a bit too much, eh?
    * server quits

So this case only occurs for the (malicious?) corner case, where I
do not bother a translation.

In the malicious case, the client says "I'll send you 10 push option
with an upper bound of 1024K", and then sends gigabytes anyway. Either
way the server has to react to what is sent, not what is promised.

-Peff

On Fri, Jul 8, 2016 at 3:21 PM, Jeff King [off-list ref] wrote:

On Fri, Jul 08, 2016 at 03:17:13PM -0700, Stefan Beller wrote:

quoted

quoted

If people are seeing these in
routine use, then the limits are set too low, and this should happen
roughly as often as a BUG assertion, and IMHO should be treated roughly
the same: don't bother with translation, and don't worry about
optimizing wasted bandwidth for this case. It won't happen enough to
matter.

Well the wasted band width is part of the server protection, no?

Not if you stop receiving as soon as you hit the limits. Then of course
they can send up to the limit each time, but that is not a DoS. That is
things working as advertised.

quoted

This would favor the idea Jonathan came up with:

    server: I advertise push options
    client: ok I want to use push options
    client: I'll send you 1000 push options with upper bound of 1000M
    server: It's a bit too much, eh?
    * server quits

So this case only occurs for the (malicious?) corner case, where I
do not bother a translation.

In the malicious case, the client says "I'll send you 10 push option
with an upper bound of 1024K", and then sends gigabytes anyway. Either
way the server has to react to what is sent, not what is promised.

Well that is what the initial patch did via:

+       for (i = 0; i < max_options; i++) {
+               char *line;
+               int len;
+
+               line = packet_read_line(0, &len);
+
+               if (!line)
+                       break;
+
+               if (len > max_size)
+                       die("protocol error: server configuration allows push "
+                           "options of size up to %d bytes", max_size);
+
+               len = strcspn(line, "\n");
+               line[len] = '\0';
+
+               string_list_append(ret, line);
+       }
+       if (i == max_options)
+               die("protocol error: server configuration only allows up "
+                   "to %d push options", max_options);

I assume the die is an effective way to "stop receiving".

Thinking further about what you said, I think the initial selections of
max_size and max_options is sufficient and we only see those bounds in
the malicious case.

This discussion rather makes me wonder if we want to stick to the initial design
as it was easy and not overcomplicating things as we assume the abort case
doesn't occur often.

-Peff

On Fri, Jul 08, 2016 at 03:29:09PM -0700, Stefan Beller wrote:

quoted

In the malicious case, the client says "I'll send you 10 push option
with an upper bound of 1024K", and then sends gigabytes anyway. Either
way the server has to react to what is sent, not what is promised.

Well that is what the initial patch did via:

+       for (i = 0; i < max_options; i++) {
+               char *line;
+               int len;
+
+               line = packet_read_line(0, &len);
+
+               if (!line)
+                       break;
+
+               if (len > max_size)
+                       die("protocol error: server configuration allows push "
+                           "options of size up to %d bytes", max_size);
+
+               len = strcspn(line, "\n");
+               line[len] = '\0';
+
+               string_list_append(ret, line);
+       }
+       if (i == max_options)
+               die("protocol error: server configuration only allows up "
+                   "to %d push options", max_options);

I assume the die is an effective way to "stop receiving".

Thinking further about what you said, I think the initial selections of
max_size and max_options is sufficient and we only see those bounds in
the malicious case.

This discussion rather makes me wonder if we want to stick to the initial design
as it was easy and not overcomplicating things as we assume the abort case
doesn't occur often.

Yes. I haven't been following the intermediate discussion and patches,
but I don't see anything wrong with the general design above. I think
you do need to use rp_error() to get the die message to the client for
non-ssh cases, though (that is a problem with other protocol-error
messages, too; I wonder if we should install a custom die handler, or
convert them all to some kind of rp_die()).

-Peff

On Fri, Jul 8, 2016 at 3:35 PM, Jeff King [off-list ref] wrote:

Yes. I haven't been following the intermediate discussion and patches,
but I don't see anything wrong with the general design above. I think
you do need to use rp_error() to get the die message to the client for
non-ssh cases, though (that is a problem with other protocol-error
messages, too; I wonder if we should install a custom die handler, or
convert them all to some kind of rp_die()).

Some of the rp_error messages do not want to die(), but most seem to
be ok when the rp_error would die.

I'll look into that.

-Peff

On Fri, Jul 08, 2016 at 03:43:27PM -0700, Stefan Beller wrote:

On Fri, Jul 8, 2016 at 3:35 PM, Jeff King [off-list ref] wrote:

quoted

Yes. I haven't been following the intermediate discussion and patches,
but I don't see anything wrong with the general design above. I think
you do need to use rp_error() to get the die message to the client for
non-ssh cases, though (that is a problem with other protocol-error
messages, too; I wonder if we should install a custom die handler, or
convert them all to some kind of rp_die()).

Some of the rp_error messages do not want to die(), but most seem to
be ok when the rp_error would die.

Sorry, I meant converting die() into:

  rp_error(...);
  die(...);

possibly via an rp_die() helper.  The existing rp_error() cases would
remain untouched.

Installing a die handler could make that work automatically, though I
suspect it would lead to corner cases where we break protocol (e.g., if
we die() in the middle of writing out a packet).

-Peff

On Fri, Jul 8, 2016 at 3:46 PM, Jeff King [off-list ref] wrote:

Sorry, I meant converting die() into:

  rp_error(...);
  die(...);

possibly via an rp_die() helper.  The existing rp_error() cases would
remain untouched.

Oh I see!
That makes a lot of sense.