[0/2] upload-pack: handle unexpected v2 delim packets

Message

Jeff King March 27, 2020, 8:02 a.m. UTC

We saw an upload-pack segfault in the wild today at GitHub. It's caused
by a client sending bogus v2 protocol lines (a "delim" packet instead of
a "flush"). So the client is broken and our only option is to break the
network connection, but we shouldn't segfault while doing so. :)

I don't think the broken client was Git. It didn't send an "agent"
capability at all, which makes me suspect it was somebody generating the
request manually (nor was there anything interesting in the transport
layer agent; it was just an openssh client).

The fix was simple enough, and is in the second patch. The first one is
just a small cleanup / refactor in preparation.

  [1/2]: test-lib-functions: make packetize() more efficient
  [2/2]: upload-pack: handle unexpected delim packets

 ls-refs.c                              |  5 +++-
 t/t5562-http-backend-content-length.sh | 19 +++++++++------
 t/t5704-protocol-violations.sh         | 33 ++++++++++++++++++++++++++
 t/test-lib-functions.sh                | 23 ++++++++++++------
 upload-pack.c                          |  5 +++-
 5 files changed, 69 insertions(+), 16 deletions(-)
 create mode 100755 t/t5704-protocol-violations.sh

-Peff

Comments

Taylor Blau March 27, 2020, 3:18 p.m. UTC | #1

Hi Peff,

On Fri, Mar 27, 2020 at 04:02:10AM -0400, Jeff King wrote:
> We saw an upload-pack segfault in the wild today at GitHub. It's caused
> by a client sending bogus v2 protocol lines (a "delim" packet instead of
> a "flush"). So the client is broken and our only option is to break the
> network connection, but we shouldn't segfault while doing so. :)
>
> I don't think the broken client was Git. It didn't send an "agent"
> capability at all, which makes me suspect it was somebody generating the
> request manually (nor was there anything interesting in the transport
> layer agent; it was just an openssh client).
>
> The fix was simple enough, and is in the second patch. The first one is
> just a small cleanup / refactor in preparation.
>
>   [1/2]: test-lib-functions: make packetize() more efficient
>   [2/2]: upload-pack: handle unexpected delim packets
>
>  ls-refs.c                              |  5 +++-
>  t/t5562-http-backend-content-length.sh | 19 +++++++++------
>  t/t5704-protocol-violations.sh         | 33 ++++++++++++++++++++++++++
>  t/test-lib-functions.sh                | 23 ++++++++++++------
>  upload-pack.c                          |  5 +++-
>  5 files changed, 69 insertions(+), 16 deletions(-)
>  create mode 100755 t/t5704-protocol-violations.sh

Thanks. This series looks good to me, and is certainly improving things.

  Reviewed-by: Taylor Blau <[e-mail]>

> -Peff

Thanks,
Taylor