Set a time limit on git upload-pack requests

When a client does a git fetch over HTTP, workhorse performs an access
check based on the HTTP request header, then reads the entire request
body into a temporary file before handing off to Gitaly to service it.
However, the client has control over how long it takes to read the
request body. Since the Gitaly RPC only happens once the request body
is read, people can set up a connection before their access is revoked
and use it to gain access to code committed days or weeks later.

To resolve this, we place an overall limit of 10 minutes on receiving
the `upload-pack` request body. Since this is over HTTP, the client is
using the `--stateless-rpc` mode, and there is no negotiation between
client and server. The time limit is chosen fairly arbitrarily, but it
fits well with the existing 10MiB limit on request body size, implying
a transfer speed of just 17KiB/sec to be able to fill that buffer and
get a "request too large" error instead of "request too slow".

Workhorse does not expose the `upload-archive` endpoint directly to the
user; the client in that case is always gitlab-rails, so there is no
vulnerability there.

The `receive-pack` endpoint is theoretically vulnerable, but Gitaly
performs a second access check in the pre-receive hook which defeats
the attack, so no changes are needed.

The SSH endpoints are similarly vulnerable, but since those RPCs are
bidirectional, a different approach is needed.

internal/git/upload-pack.go

@@ -5,19 +5,40 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"time"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
+var (
+	uploadPackTimeout = 10 * time.Minute
+)
+
 // Will not return a non-nil error after the response body has been
 // written to.
 func handleUploadPack(w *HttpResponseWriter, r *http.Request, a *api.Response) error {
+	ctx := r.Context()
+
 	// The body will consist almost entirely of 'have XXX' and 'want XXX'
-	// lines; these are about 50 bytes long. With a limit of 10MB the client
-	// can send over 200,000 have/want lines.
-	buffer, err := helper.ReadAllTempfile(io.LimitReader(r.Body, 10*1024*1024))
+	// lines; these are about 50 bytes long. With a size limit of 10MiB, the
+	// client can send over 200,000 have/want lines.
+	sizeLimited := io.LimitReader(r.Body, 10*1024*1024)
+
+	// Prevent the client from holding the connection open indefinitely. A
+	// transfer rate of 17KiB/sec is sufficient to fill the 10MiB buffer in
+	// ten minutes, which seems adequate. Most requests will be much smaller.
+	// This mitigates a use-after-check issue.
+	//
+	// We can't reliably interrupt the read from a http handler, but we can
+	// ensure the request will (eventually) fail: https://github.com/golang/go/issues/16100
+	readerCtx, cancel := context.WithTimeout(ctx, uploadPackTimeout)
+	defer cancel()
+
+	limited := helper.NewContextReader(readerCtx, sizeLimited)
+	buffer, err := helper.ReadAllTempfile(limited)
+
 	if err != nil {
 		return fmt.Errorf("ReadAllTempfile: %v", err)
 	}
@@ -29,7 +50,7 @@ func handleUploadPack(w *HttpResponseWriter, r *http.Request, a *api.Response) e
 
 	gitProtocol := r.Header.Get("Git-Protocol")
 
-	return handleUploadPackWithGitaly(r.Context(), a, buffer, w, gitProtocol)
+	return handleUploadPackWithGitaly(ctx, a, buffer, w, gitProtocol)
 }
 
 func handleUploadPackWithGitaly(ctx context.Context, a *api.Response, clientRequest io.Reader, clientResponse io.Writer, gitProtocol string) error {

internal/git/upload-pack_test.go

+package git
+
+import (
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
+)
+
+var (
+	originalUploadPackTimeout = uploadPackTimeout
+)
+
+type fakeReader struct {
+	n   int
+	err error
+}
+
+func (f *fakeReader) Read(b []byte) (int, error) {
+	return f.n, f.err
+}
+
+func TestUploadPackTimesOut(t *testing.T) {
+	uploadPackTimeout = time.Millisecond
+	defer func() { uploadPackTimeout = originalUploadPackTimeout }()
+
+	body := &fakeReader{n: 0, err: nil}
+
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest("GET", "/", body)
+	a := &api.Response{}
+
+	err := handleUploadPack(NewHttpResponseWriter(w), r, a)
+	require.EqualError(t, err, "ReadAllTempfile: context deadline exceeded")
+
+}

internal/helper/context_reader.go

+package helper
+
+import (
+	"context"
+	"io"
+)
+
+type ContextReader struct {
+	ctx              context.Context
+	underlyingReader io.Reader
+}
+
+func NewContextReader(ctx context.Context, underlyingReader io.Reader) *ContextReader {
+	return &ContextReader{
+		ctx:              ctx,
+		underlyingReader: underlyingReader,
+	}
+}
+
+func (r *ContextReader) Read(b []byte) (int, error) {
+	if r.canceled() {
+		return 0, r.err()
+	}
+
+	n, err := r.underlyingReader.Read(b)
+
+	if r.canceled() {
+		err = r.err()
+	}
+
+	return n, err
+}
+
+func (r *ContextReader) canceled() bool {
+	return r.err() != nil
+}
+
+func (r *ContextReader) err() error {
+	return r.ctx.Err()
+}

internal/helper/context_reader_test.go

+package helper
+
+import (
+	"context"
+	"io"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+type fakeReader struct {
+	n   int
+	err error
+}
+
+func (f *fakeReader) Read(b []byte) (int, error) {
+	return f.n, f.err
+}
+
+type fakeContextWithTimeout struct {
+	n         int
+	threshold int
+}
+
+func (*fakeContextWithTimeout) Deadline() (deadline time.Time, ok bool) {
+	return
+}
+
+func (*fakeContextWithTimeout) Done() <-chan struct{} {
+	return nil
+}
+
+func (*fakeContextWithTimeout) Value(key interface{}) interface{} {
+	return nil
+}
+
+func (f *fakeContextWithTimeout) Err() error {
+	f.n++
+	if f.n > f.threshold {
+		return context.DeadlineExceeded
+	}
+
+	return nil
+}
+
+func TestContextReaderRead(t *testing.T) {
+	underlyingReader := &fakeReader{n: 1, err: io.EOF}
+
+	for _, tc := range []struct {
+		desc        string
+		ctx         *fakeContextWithTimeout
+		expectedN   int
+		expectedErr error
+	}{
+		{
+			desc:        "Before and after read deadline checks are fine",
+			ctx:         &fakeContextWithTimeout{n: 0, threshold: 2},
+			expectedN:   underlyingReader.n,
+			expectedErr: underlyingReader.err,
+		},
+		{
+			desc:        "Before read deadline check fails",
+			ctx:         &fakeContextWithTimeout{n: 0, threshold: 0},
+			expectedN:   0,
+			expectedErr: context.DeadlineExceeded,
+		},
+		{
+			desc:        "After read deadline check fails",
+			ctx:         &fakeContextWithTimeout{n: 0, threshold: 1},
+			expectedN:   underlyingReader.n,
+			expectedErr: context.DeadlineExceeded,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			cr := NewContextReader(tc.ctx, underlyingReader)
+
+			n, err := cr.Read(nil)
+			require.Equal(t, tc.expectedN, n)
+			require.Equal(t, tc.expectedErr, err)
+		})
+	}
+}