Merge branch 'security-29660-time-limit-upload-pack' into 'master'

Set a time limit on git upload-pack requests

See merge request gitlab/gitlab-workhorse!24

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)
+		})
+	}
+}