diff options
author | Paul Okstad <pokstad@gitlab.com> | 2019-08-13 01:18:33 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2019-08-13 01:18:33 +0300 |
commit | 0ec00f843cf878673934877d8a6194d98cfd9318 (patch) | |
tree | ff4b66c656b23b8a873350576214b447f666a255 | |
parent | c6d201ee96ddc917ba0a94bf2fa5cea0e0627155 (diff) |
Fix FindCommits flag injection exploit
37 files changed, 332 insertions, 25 deletions
diff --git a/changelogs/unreleased/security-1799-flag-injection.yml b/changelogs/unreleased/security-1799-flag-injection.yml new file mode 100644 index 000000000..63f6176ba --- /dev/null +++ b/changelogs/unreleased/security-1799-flag-injection.yml @@ -0,0 +1,5 @@ +--- +title: Fix FindCommits flag injection exploit +merge_request: 31 +author: +type: security diff --git a/internal/git/proto.go b/internal/git/proto.go index 5244e6bdb..17323c600 100644 --- a/internal/git/proto.go +++ b/internal/git/proto.go @@ -18,9 +18,8 @@ import ( // See https://gitlab.com/gitlab-org/gitaly/issues/556#note_40289573 var FallbackTimeValue = time.Unix(1<<63-62135596801, 999999999) -// ValidateRevision checks if a revision looks valid -func ValidateRevision(revision []byte) error { - if len(revision) == 0 { +func validateRevision(revision []byte, allowEmpty bool) error { + if !allowEmpty && len(revision) == 0 { return fmt.Errorf("empty revision") } if bytes.HasPrefix(revision, []byte("-")) { @@ -38,6 +37,17 @@ func ValidateRevision(revision []byte) error { return nil } +// ValidateRevisionAllowEmpty checks if a revision looks valid, but allows +// empty strings +func ValidateRevisionAllowEmpty(revision []byte) error { + return validateRevision(revision, true) +} + +// ValidateRevision checks if a revision looks valid +func ValidateRevision(revision []byte) error { + return validateRevision(revision, false) +} + // Version returns the used git version. func Version() (string, error) { ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/service/blob/get_blobs.go b/internal/service/blob/get_blobs.go index 1388609d6..dc1877ad4 100644 --- a/internal/service/blob/get_blobs.go +++ b/internal/service/blob/get_blobs.go @@ -4,6 +4,7 @@ import ( "io" "io/ioutil" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/service/commit" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -105,6 +106,19 @@ func sendGetBlobsResponse(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobSer } func (*server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobService_GetBlobsServer) error { + if err := validateGetBlobsRequest(req); err != nil { + return err + } + + c, err := catfile.New(stream.Context(), req.Repository) + if err != nil { + return err + } + + return sendGetBlobsResponse(req, stream, c) +} + +func validateGetBlobsRequest(req *gitalypb.GetBlobsRequest) error { if req.Repository == nil { return status.Errorf(codes.InvalidArgument, "GetBlobs: empty Repository") } @@ -112,10 +126,12 @@ func (*server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobServi if len(req.RevisionPaths) == 0 { return status.Errorf(codes.InvalidArgument, "GetBlobs: empty RevisionPaths") } - c, err := catfile.New(stream.Context(), req.Repository) - if err != nil { - return err + + for _, rp := range req.RevisionPaths { + if err := git.ValidateRevision([]byte(rp.Revision)); err != nil { + return status.Errorf(codes.InvalidArgument, "GetBlobs: %v", err) + } } - return sendGetBlobsResponse(req, stream, c) + return nil } diff --git a/internal/service/blob/get_blobs_test.go b/internal/service/blob/get_blobs_test.go index b7fd6b9fa..da532e0df 100644 --- a/internal/service/blob/get_blobs_test.go +++ b/internal/service/blob/get_blobs_test.go @@ -151,6 +151,19 @@ func TestFailedGetBlobsRequestDueToValidation(t *testing.T) { }, code: codes.InvalidArgument, }, + { + desc: "invalid Revision", + request: &gitalypb.GetBlobsRequest{ + Repository: testRepo, + RevisionPaths: []*gitalypb.GetBlobsRequest_RevisionPath{ + { + Path: []byte("CHANGELOG"), + Revision: "--output=/meow", + }, + }, + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/commits_by_message.go b/internal/service/commit/commits_by_message.go index 1b4d3b87f..915a186dc 100644 --- a/internal/service/commit/commits_by_message.go +++ b/internal/service/commit/commits_by_message.go @@ -3,6 +3,7 @@ package commit import ( "fmt" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -67,6 +68,10 @@ func commitsByMessage(in *gitalypb.CommitsByMessageRequest, stream gitalypb.Comm } func validateCommitsByMessageRequest(in *gitalypb.CommitsByMessageRequest) error { + if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil { + return err + } + if in.GetQuery() == "" { return fmt.Errorf("empty Query") } diff --git a/internal/service/commit/commits_by_message_test.go b/internal/service/commit/commits_by_message_test.go index e9a7f17c2..0a5986101 100644 --- a/internal/service/commit/commits_by_message_test.go +++ b/internal/service/commit/commits_by_message_test.go @@ -184,6 +184,11 @@ func TestFailedCommitsByMessageRequest(t *testing.T) { request: &gitalypb.CommitsByMessageRequest{Repository: testRepo}, code: codes.InvalidArgument, }, + { + desc: "Revision is invalid", + request: &gitalypb.CommitsByMessageRequest{Repository: testRepo, Revision: []byte("--output=/meow"), Query: "not empty"}, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/count_commits.go b/internal/service/commit/count_commits.go index f96f0dca5..1f96bb2fc 100644 --- a/internal/service/commit/count_commits.go +++ b/internal/service/commit/count_commits.go @@ -72,6 +72,10 @@ func (s *server) CountCommits(ctx context.Context, in *gitalypb.CountCommitsRequ } func validateCountCommitsRequest(in *gitalypb.CountCommitsRequest) error { + if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil { + return err + } + if len(in.GetRevision()) == 0 && !in.GetAll() { return fmt.Errorf("empty Revision and false All") } diff --git a/internal/service/commit/count_commits_test.go b/internal/service/commit/count_commits_test.go index f3ad92740..a4b07611e 100644 --- a/internal/service/commit/count_commits_test.go +++ b/internal/service/commit/count_commits_test.go @@ -177,8 +177,9 @@ func TestFailedCountCommitsRequestDueToValidationError(t *testing.T) { rpcRequests := []gitalypb.CountCommitsRequest{ {Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision}, // Repository doesn't exist - {Repository: nil, Revision: revision}, // Repository is nil - {Repository: testRepo, Revision: nil, All: false}, // Revision is empty and All is false + {Repository: nil, Revision: revision}, // Repository is nil + {Repository: testRepo, Revision: nil, All: false}, // Revision is empty and All is false + {Repository: testRepo, Revision: []byte("--output=/meow"), All: false}, // Revision is invalid } for _, rpcRequest := range rpcRequests { diff --git a/internal/service/commit/find_all_commits.go b/internal/service/commit/find_all_commits.go index 5116dff30..8f717d90e 100644 --- a/internal/service/commit/find_all_commits.go +++ b/internal/service/commit/find_all_commits.go @@ -3,6 +3,7 @@ package commit import ( "fmt" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/internal/service/ref" @@ -26,6 +27,10 @@ func (sender *findAllCommitsSender) Send() error { } func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gitalypb.CommitService_FindAllCommitsServer) error { + if err := validateFindAllCommitsRequest(in); err != nil { + return err + } + var revisions []string if len(in.GetRevision()) == 0 { branchNames, err := _findBranchNamesFunc(stream.Context(), in.Repository) @@ -47,6 +52,14 @@ func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gital return nil } +func validateFindAllCommitsRequest(in *gitalypb.FindAllCommitsRequest) error { + if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil { + return helper.ErrInvalidArgument(err) + } + + return nil +} + func findAllCommits(in *gitalypb.FindAllCommitsRequest, stream gitalypb.CommitService_FindAllCommitsServer, revisions []string) error { sender := &findAllCommitsSender{stream: stream} diff --git a/internal/service/commit/find_all_commits_test.go b/internal/service/commit/find_all_commits_test.go index 7b1f6d028..43dd6fae7 100644 --- a/internal/service/commit/find_all_commits_test.go +++ b/internal/service/commit/find_all_commits_test.go @@ -292,6 +292,9 @@ func TestFailedFindAllCommitsRequest(t *testing.T) { invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"} + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + testCases := []struct { desc string request *gitalypb.FindAllCommitsRequest @@ -307,6 +310,14 @@ func TestFailedFindAllCommitsRequest(t *testing.T) { request: &gitalypb.FindAllCommitsRequest{}, code: codes.InvalidArgument, }, + { + desc: "Revision is invalid", + request: &gitalypb.FindAllCommitsRequest{ + Repository: testRepo, + Revision: []byte("--output=/meow"), + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/find_commits.go b/internal/service/commit/find_commits.go index e5d49a5ad..3e4709353 100644 --- a/internal/service/commit/find_commits.go +++ b/internal/service/commit/find_commits.go @@ -21,6 +21,10 @@ const commitsPerPage int = 20 func (s *server) FindCommits(req *gitalypb.FindCommitsRequest, stream gitalypb.CommitService_FindCommitsServer) error { ctx := stream.Context() + if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil { + return helper.ErrInvalidArgument(err) + } + // Use Gitaly's default branch lookup function because that is already // migrated. if revision := req.Revision; len(revision) == 0 && !req.GetAll() { diff --git a/internal/service/commit/find_commits_test.go b/internal/service/commit/find_commits_test.go index 8e1dc4944..b68ac1def 100644 --- a/internal/service/commit/find_commits_test.go +++ b/internal/service/commit/find_commits_test.go @@ -438,6 +438,15 @@ func TestFailureFindCommitsRequest(t *testing.T) { }, code: codes.InvalidArgument, }, + { + desc: "invalid revision", + request: &gitalypb.FindCommitsRequest{ + Repository: testRepo, + Revision: []byte("--output=/meow"), + Limit: 1, + }, + code: codes.InvalidArgument, + }, } for _, tc := range testCases { diff --git a/internal/service/commit/languages.go b/internal/service/commit/languages.go index 63a104b2d..87c8ab0d5 100644 --- a/internal/service/commit/languages.go +++ b/internal/service/commit/languages.go @@ -18,6 +18,10 @@ import ( func (*server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLanguagesRequest) (*gitalypb.CommitLanguagesResponse, error) { repo := req.Repository + if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil { + return nil, helper.ErrInvalidArgument(err) + } + revision := string(req.Revision) if revision == "" { defaultBranch, err := ref.DefaultBranchName(ctx, req.Repository) diff --git a/internal/service/commit/languages_test.go b/internal/service/commit/languages_test.go index a9faeae28..c23ba8a23 100644 --- a/internal/service/commit/languages_test.go +++ b/internal/service/commit/languages_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc/codes" ) func TestLanguages(t *testing.T) { @@ -91,3 +92,23 @@ func TestLanguagesEmptyRevision(t *testing.T) { } require.True(t, foundRuby, "expected to find Ruby as a language on HEAD") } + +func TestInvalidCommitLanguagesRequestRevision(t *testing.T) { + server, serverSocketPath := startTestServices(t) + defer server.Stop() + + client, conn := newCommitServiceClient(t, serverSocketPath) + defer conn.Close() + + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + ctx, cancel := testhelper.Context() + defer cancel() + + _, err := client.CommitLanguages(ctx, &gitalypb.CommitLanguagesRequest{ + Repository: testRepo, + Revision: []byte("--output=/meow"), + }) + testhelper.RequireGrpcError(t, err, codes.InvalidArgument) +} diff --git a/internal/service/commit/last_commit_for_path.go b/internal/service/commit/last_commit_for_path.go index 60e3c3c7e..350b90b3b 100644 --- a/internal/service/commit/last_commit_for_path.go +++ b/internal/service/commit/last_commit_for_path.go @@ -2,8 +2,8 @@ package commit import ( "context" - "fmt" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -44,8 +44,8 @@ func lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitForPathReques } func validateLastCommitForPathRequest(in *gitalypb.LastCommitForPathRequest) error { - if len(in.Revision) == 0 { - return fmt.Errorf("empty Revision") + if err := git.ValidateRevision(in.Revision); err != nil { + return helper.ErrInvalidArgument(err) } return nil } diff --git a/internal/service/commit/last_commit_for_path_test.go b/internal/service/commit/last_commit_for_path_test.go index 24c5c70a5..f9c58757c 100644 --- a/internal/service/commit/last_commit_for_path_test.go +++ b/internal/service/commit/last_commit_for_path_test.go @@ -121,6 +121,13 @@ func TestFailedLastCommitForPathRequest(t *testing.T) { request: &gitalypb.LastCommitForPathRequest{Repository: testRepo, Path: []byte("foo/bar")}, code: codes.InvalidArgument, }, + { + desc: "Revision is invalid", + request: &gitalypb.LastCommitForPathRequest{ + Repository: testRepo, Path: []byte("foo/bar"), Revision: []byte("--output=/meow"), + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/list_files.go b/internal/service/commit/list_files.go index 615ab1534..547633dc2 100644 --- a/internal/service/commit/list_files.go +++ b/internal/service/commit/list_files.go @@ -19,6 +19,10 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit "Revision": in.GetRevision(), }).Debug("ListFiles") + if err := validateListFilesRequest(in); err != nil { + return err + } + repo := in.Repository if _, err := helper.GetRepoPath(repo); err != nil { return err @@ -45,6 +49,13 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit return nil } +func validateListFilesRequest(in *gitalypb.ListFilesRequest) error { + if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil { + return helper.ErrInvalidArgument(err) + } + return nil +} + func listFiles(repo *gitalypb.Repository, revision string, stream gitalypb.CommitService_ListFilesServer) error { args := []string{"ls-tree", "-z", "-r", "--full-tree", "--full-name", "--", revision} cmd, err := git.Command(stream.Context(), repo, args...) diff --git a/internal/service/commit/list_files_test.go b/internal/service/commit/list_files_test.go index a3d4132c9..8c80a4896 100644 --- a/internal/service/commit/list_files_test.go +++ b/internal/service/commit/list_files_test.go @@ -7,6 +7,7 @@ import ( "io" "testing" + "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/service/ref" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -184,3 +185,26 @@ func drainListFilesResponse(c gitalypb.CommitService_ListFilesClient) error { } return err } + +func TestInvalidListFilesRequestRevision(t *testing.T) { + server, serverSocketPath := startTestServices(t) + defer server.Stop() + + client, conn := newCommitServiceClient(t, serverSocketPath) + defer conn.Close() + + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.ListFiles(ctx, &gitalypb.ListFilesRequest{ + Repository: testRepo, + Revision: []byte("--output=/meow"), + }) + require.NoError(t, err) + + _, err = stream.Recv() + testhelper.RequireGrpcError(t, err, codes.InvalidArgument) +} diff --git a/internal/service/commit/list_last_commits_for_tree.go b/internal/service/commit/list_last_commits_for_tree.go index ada4b2915..cb379e420 100644 --- a/internal/service/commit/list_last_commits_for_tree.go +++ b/internal/service/commit/list_last_commits_for_tree.go @@ -1,7 +1,6 @@ package commit import ( - "fmt" "io" "sort" @@ -144,8 +143,5 @@ func sendCommitsForTree(batch []*gitalypb.ListLastCommitsForTreeResponse_CommitF } func validateListLastCommitsForTreeRequest(in *gitalypb.ListLastCommitsForTreeRequest) error { - if in.Revision == "" { - return fmt.Errorf("empty Revision") - } - return nil + return git.ValidateRevision([]byte(in.Revision)) } diff --git a/internal/service/commit/list_last_commits_for_tree_test.go b/internal/service/commit/list_last_commits_for_tree_test.go index b357c79c9..722964c58 100644 --- a/internal/service/commit/list_last_commits_for_tree_test.go +++ b/internal/service/commit/list_last_commits_for_tree_test.go @@ -278,6 +278,16 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) { }, code: codes.Internal, }, + { + desc: "Invalid revision", + request: &gitalypb.ListLastCommitsForTreeRequest{ + Repository: testRepo, + Revision: "--output=/meow", + Offset: 0, + Limit: 25, + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/raw_blame.go b/internal/service/commit/raw_blame.go index 49be189e6..840d83ac6 100644 --- a/internal/service/commit/raw_blame.go +++ b/internal/service/commit/raw_blame.go @@ -46,8 +46,8 @@ func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitSe } func validateRawBlameRequest(in *gitalypb.RawBlameRequest) error { - if len(in.GetRevision()) == 0 { - return fmt.Errorf("empty Revision") + if err := git.ValidateRevision(in.Revision); err != nil { + return err } if len(in.GetPath()) == 0 { diff --git a/internal/service/commit/raw_blame_test.go b/internal/service/commit/raw_blame_test.go index 8ecdbefcf..cde759ff2 100644 --- a/internal/service/commit/raw_blame_test.go +++ b/internal/service/commit/raw_blame_test.go @@ -113,6 +113,13 @@ func TestFailedRawBlameRequest(t *testing.T) { path: []byte(""), code: codes.InvalidArgument, }, + { + description: "Invalid revision", + repo: testRepo, + revision: []byte("--output=/meow"), + path: []byte("a/b/c"), + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/commit/stats_test.go b/internal/service/commit/stats_test.go index 373326622..2d9236754 100644 --- a/internal/service/commit/stats_test.go +++ b/internal/service/commit/stats_test.go @@ -119,6 +119,12 @@ func TestCommitStatsFailure(t *testing.T) { revision: []byte("non/existing"), err: codes.Internal, }, + { + desc: "invalid revision", + repo: testRepo, + revision: []byte("--outpu=/meow"), + err: codes.InvalidArgument, + }, } for _, tc := range tests { diff --git a/internal/service/commit/tree_entries.go b/internal/service/commit/tree_entries.go index ca99abc52..7e7fc23ee 100644 --- a/internal/service/commit/tree_entries.go +++ b/internal/service/commit/tree_entries.go @@ -5,6 +5,7 @@ import ( grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -15,8 +16,8 @@ import ( var maxTreeEntries = 1000 func validateGetTreeEntriesRequest(in *gitalypb.GetTreeEntriesRequest) error { - if len(in.GetRevision()) == 0 { - return fmt.Errorf("empty Revision") + if err := git.ValidateRevision(in.Revision); err != nil { + return err } if len(in.GetPath()) == 0 { diff --git a/internal/service/commit/tree_entries_test.go b/internal/service/commit/tree_entries_test.go index 4a86f7507..0cca60015 100644 --- a/internal/service/commit/tree_entries_test.go +++ b/internal/service/commit/tree_entries_test.go @@ -432,6 +432,7 @@ func TestFailedGetTreeEntriesRequestDueToValidationError(t *testing.T) { {Repository: nil, Revision: revision, Path: path}, // Repository is nil {Repository: testRepo, Revision: nil, Path: path}, // Revision is empty {Repository: testRepo, Revision: revision}, // Path is empty + {Repository: testRepo, Revision: []byte("--output=/meow"), Path: path}, // Revision is invalid } for _, rpcRequest := range rpcRequests { diff --git a/internal/service/commit/tree_entry.go b/internal/service/commit/tree_entry.go index 527edf654..8498b8436 100644 --- a/internal/service/commit/tree_entry.go +++ b/internal/service/commit/tree_entry.go @@ -5,6 +5,7 @@ import ( "io" "strings" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -122,8 +123,8 @@ func (s *server) TreeEntry(in *gitalypb.TreeEntryRequest, stream gitalypb.Commit } func validateRequest(in *gitalypb.TreeEntryRequest) error { - if len(in.GetRevision()) == 0 { - return fmt.Errorf("empty Revision") + if err := git.ValidateRevision(in.Revision); err != nil { + return err } if len(in.GetPath()) == 0 { diff --git a/internal/service/commit/tree_entry_test.go b/internal/service/commit/tree_entry_test.go index 775e80517..5460cb336 100644 --- a/internal/service/commit/tree_entry_test.go +++ b/internal/service/commit/tree_entry_test.go @@ -178,6 +178,7 @@ func TestFailedTreeEntryRequestDueToValidationError(t *testing.T) { {Repository: nil, Revision: revision, Path: path}, // Repository is nil {Repository: testRepo, Revision: nil, Path: path}, // Revision is empty {Repository: testRepo, Revision: revision}, // Path is empty + {Repository: testRepo, Revision: []byte("--output=/meow"), Path: path}, // Revision is invalid } for _, rpcRequest := range rpcRequests { diff --git a/internal/service/diff/patch.go b/internal/service/diff/patch.go index f8c02be6a..e4d6977fb 100644 --- a/internal/service/diff/patch.go +++ b/internal/service/diff/patch.go @@ -1,11 +1,17 @@ package diff import ( + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) func (s *server) CommitPatch(in *gitalypb.CommitPatchRequest, stream gitalypb.DiffService_CommitPatchServer) error { + if err := git.ValidateRevision(in.Revision); err != nil { + return helper.ErrInvalidArgument(err) + } + ctx := stream.Context() client, err := s.DiffServiceClient(ctx) diff --git a/internal/service/diff/patch_test.go b/internal/service/diff/patch_test.go index d1a3396bf..4927f3083 100644 --- a/internal/service/diff/patch_test.go +++ b/internal/service/diff/patch_test.go @@ -5,8 +5,10 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc/codes" ) func TestSuccessfulCommitPatchRequest(t *testing.T) { @@ -67,3 +69,26 @@ func TestSuccessfulCommitPatchRequest(t *testing.T) { }) } } + +func TestInvalidCommitPatchRequestRevision(t *testing.T) { + server, serverSocketPath := runDiffServer(t) + defer server.Stop() + + client, conn := newDiffClient(t, serverSocketPath) + defer conn.Close() + + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.CommitPatch(ctx, &gitalypb.CommitPatchRequest{ + Repository: testRepo, + Revision: []byte("--output=/meow"), + }) + require.NoError(t, err) + + _, err = stream.Recv() + testhelper.RequireGrpcError(t, err, codes.InvalidArgument) +} diff --git a/internal/service/repository/apply_gitattributes_test.go b/internal/service/repository/apply_gitattributes_test.go index 16485791a..e82968530 100644 --- a/internal/service/repository/apply_gitattributes_test.go +++ b/internal/service/repository/apply_gitattributes_test.go @@ -110,6 +110,11 @@ func TestApplyGitattributesFailure(t *testing.T) { revision: []byte("not-existing-ref"), code: codes.InvalidArgument, }, + { + repo: testRepo, + revision: []byte("--output=/meow"), + code: codes.InvalidArgument, + }, } for _, test := range tests { diff --git a/internal/service/repository/write_ref_test.go b/internal/service/repository/write_ref_test.go index e836e03cc..905515a7e 100644 --- a/internal/service/repository/write_ref_test.go +++ b/internal/service/repository/write_ref_test.go @@ -137,6 +137,14 @@ func TestWriteRefValidationError(t *testing.T) { Revision: []byte("0123012301231243"), }, }, + { + desc: "invalid revision", + req: &gitalypb.WriteRefRequest{ + Repository: testRepo, + Ref: []byte("refs/heads/master"), + Revision: []byte("--output=/meow"), + }, + }, } for _, tc := range testCases { diff --git a/internal/service/wiki/find_file.go b/internal/service/wiki/find_file.go index 6ecb62058..cf936b8de 100644 --- a/internal/service/wiki/find_file.go +++ b/internal/service/wiki/find_file.go @@ -1,6 +1,7 @@ package wiki import ( + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -10,6 +11,10 @@ import ( func (s *server) WikiFindFile(request *gitalypb.WikiFindFileRequest, stream gitalypb.WikiService_WikiFindFileServer) error { ctx := stream.Context() + if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil { + return status.Errorf(codes.InvalidArgument, "WikiFindFile: %s", err) + } + if len(request.GetName()) == 0 { return status.Errorf(codes.InvalidArgument, "WikiFindFile: Empty Name") } diff --git a/internal/service/wiki/find_file_test.go b/internal/service/wiki/find_file_test.go index e4735dfdd..9908686a0 100644 --- a/internal/service/wiki/find_file_test.go +++ b/internal/service/wiki/find_file_test.go @@ -180,6 +180,12 @@ func TestFailedWikiFindFileDueToValidation(t *testing.T) { revision: "deadfacedeadfacedeadfacedeadfacedeadface", code: codes.Unknown, }, + { + desc: "dangerously invalid revision", + name: "image.jpg", + revision: "--output=/meow", + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { diff --git a/internal/service/wiki/find_page.go b/internal/service/wiki/find_page.go index 3ba586544..239781cf8 100644 --- a/internal/service/wiki/find_page.go +++ b/internal/service/wiki/find_page.go @@ -1,6 +1,9 @@ package wiki import ( + "errors" + + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -10,8 +13,8 @@ import ( func (s *server) WikiFindPage(request *gitalypb.WikiFindPageRequest, stream gitalypb.WikiService_WikiFindPageServer) error { ctx := stream.Context() - if len(request.GetTitle()) == 0 { - return status.Errorf(codes.InvalidArgument, "WikiFindPage: Empty Title") + if err := validateWikiFindPage(request); err != nil { + return status.Errorf(codes.InvalidArgument, "WikiFindPage: %v", err) } client, err := s.WikiServiceClient(ctx) @@ -39,3 +42,13 @@ func (s *server) WikiFindPage(request *gitalypb.WikiFindPageRequest, stream gita return stream.Send(resp) }) } + +func validateWikiFindPage(request *gitalypb.WikiFindPageRequest) error { + if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil { + return err + } + if len(request.GetTitle()) == 0 { + return errors.New("empty Title") + } + return nil +} diff --git a/internal/service/wiki/find_page_test.go b/internal/service/wiki/find_page_test.go index 9cb8511a5..fd7e262f6 100644 --- a/internal/service/wiki/find_page_test.go +++ b/internal/service/wiki/find_page_test.go @@ -370,3 +370,27 @@ func readFullWikiPageFromWikiFindPageClient(t *testing.T, c gitalypb.WikiService return wikiPage } + +func TestInvalidWikiFindPageRequestRevision(t *testing.T) { + server, serverSocketPath := runWikiServiceServer(t) + defer server.Stop() + + client, conn := newWikiClient(t, serverSocketPath) + defer conn.Close() + + wikiRepo, _, cleanupFunc := setupWikiRepo(t) + defer cleanupFunc() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.WikiFindPage(ctx, &gitalypb.WikiFindPageRequest{ + Repository: wikiRepo, + Title: []byte("non-empty title"), + Revision: []byte("--output=/meow"), + }) + require.NoError(t, err) + + _, err = stream.Recv() + testhelper.RequireGrpcError(t, err, codes.InvalidArgument) +} diff --git a/internal/service/wiki/formatted_data.go b/internal/service/wiki/formatted_data.go index c4e24b7c3..27a49be6a 100644 --- a/internal/service/wiki/formatted_data.go +++ b/internal/service/wiki/formatted_data.go @@ -1,6 +1,7 @@ package wiki import ( + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -8,6 +9,10 @@ import ( ) func (s *server) WikiGetFormattedData(request *gitalypb.WikiGetFormattedDataRequest, stream gitalypb.WikiService_WikiGetFormattedDataServer) error { + if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil { + return status.Errorf(codes.InvalidArgument, "WikiGetFormattedData: %s", err) + } + ctx := stream.Context() if len(request.GetTitle()) == 0 { diff --git a/internal/service/wiki/formatted_data_test.go b/internal/service/wiki/formatted_data_test.go index 3233c416a..d14513fa1 100644 --- a/internal/service/wiki/formatted_data_test.go +++ b/internal/service/wiki/formatted_data_test.go @@ -157,3 +157,27 @@ func readFullDataFromWikiGetFormattedDataClient(t *testing.T, c gitalypb.WikiSer return } + +func TestInvalidWikiGetFormattedDataRequestRevision(t *testing.T) { + wikiRepo, _, cleanupFunc := setupWikiRepo(t) + defer cleanupFunc() + + server, serverSocketPath := runWikiServiceServer(t) + defer server.Stop() + + client, conn := newWikiClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.WikiGetFormattedData(ctx, &gitalypb.WikiGetFormattedDataRequest{ + Repository: wikiRepo, + Title: []byte("Home Pagé"), + Revision: []byte("--output=/meow"), + }) + require.NoError(t, err) + + _, err = stream.Recv() + testhelper.RequireGrpcError(t, err, codes.InvalidArgument) +} |