From 63ae215071f94569d910964bdee866d91d6e3a10 Mon Sep 17 00:00:00 2001 From: Casey Lee Date: Mon, 16 Jan 2023 13:01:54 -0800 Subject: [PATCH] fix: update artifact server to address GHSL-2023-004 (#1565) --- .vscode/settings.json | 1 + pkg/artifacts/server.go | 82 +++++---- pkg/artifacts/server_test.go | 161 +++++++++++++----- .../testdata/GHSL-2023-004/artifacts.yml | 43 +++++ 4 files changed, 208 insertions(+), 79 deletions(-) create mode 100644 pkg/artifacts/testdata/GHSL-2023-004/artifacts.yml diff --git a/.vscode/settings.json b/.vscode/settings.json index 8f4495dd..3e305ae5 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,7 @@ { "go.lintTool": "golangci-lint", "go.lintFlags": ["--fix"], + "go.testTimeout": "300s", "[json]": { "editor.defaultFormatter": "esbenp.prettier-vscode" }, diff --git a/pkg/artifacts/server.go b/pkg/artifacts/server.go index 9b114573..7c3df56c 100644 --- a/pkg/artifacts/server.go +++ b/pkg/artifacts/server.go @@ -9,7 +9,6 @@ import ( "io/fs" "net/http" "os" - "path" "path/filepath" "strings" "time" @@ -46,28 +45,34 @@ type ResponseMessage struct { Message string `json:"message"` } -type MkdirFS interface { - fs.FS - MkdirAll(path string, perm fs.FileMode) error - Open(name string) (fs.File, error) - OpenAtEnd(name string) (fs.File, error) +type WritableFile interface { + io.WriteCloser } -type MkdirFsImpl struct { - dir string - fs.FS +type WriteFS interface { + OpenWritable(name string) (WritableFile, error) + OpenAppendable(name string) (WritableFile, error) } -func (fsys MkdirFsImpl) MkdirAll(path string, perm fs.FileMode) error { - return os.MkdirAll(fsys.dir+"/"+path, perm) +type readWriteFSImpl struct { } -func (fsys MkdirFsImpl) Open(name string) (fs.File, error) { - return os.OpenFile(fsys.dir+"/"+name, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0644) +func (fwfs readWriteFSImpl) Open(name string) (fs.File, error) { + return os.Open(name) } -func (fsys MkdirFsImpl) OpenAtEnd(name string) (fs.File, error) { - file, err := os.OpenFile(fsys.dir+"/"+name, os.O_CREATE|os.O_RDWR, 0644) +func (fwfs readWriteFSImpl) OpenWritable(name string) (WritableFile, error) { + if err := os.MkdirAll(filepath.Dir(name), os.ModePerm); err != nil { + return nil, err + } + return os.OpenFile(name, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0644) +} + +func (fwfs readWriteFSImpl) OpenAppendable(name string) (WritableFile, error) { + if err := os.MkdirAll(filepath.Dir(name), os.ModePerm); err != nil { + return nil, err + } + file, err := os.OpenFile(name, os.O_CREATE|os.O_RDWR, 0644) if err != nil { return nil, err @@ -77,13 +82,16 @@ func (fsys MkdirFsImpl) OpenAtEnd(name string) (fs.File, error) { if err != nil { return nil, err } - return file, nil } var gzipExtension = ".gz__" -func uploads(router *httprouter.Router, fsys MkdirFS) { +func safeResolve(baseDir string, relPath string) string { + return filepath.Join(baseDir, filepath.Clean(filepath.Join(string(os.PathSeparator), relPath))) +} + +func uploads(router *httprouter.Router, baseDir string, fsys WriteFS) { router.POST("/_apis/pipelines/workflows/:runId/artifacts", func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { runID := params.ByName("runId") @@ -108,19 +116,15 @@ func uploads(router *httprouter.Router, fsys MkdirFS) { itemPath += gzipExtension } - filePath := fmt.Sprintf("%s/%s", runID, itemPath) + safeRunPath := safeResolve(baseDir, runID) + safePath := safeResolve(safeRunPath, itemPath) - err := fsys.MkdirAll(path.Dir(filePath), os.ModePerm) - if err != nil { - panic(err) - } - - file, err := func() (fs.File, error) { + file, err := func() (WritableFile, error) { contentRange := req.Header.Get("Content-Range") if contentRange != "" && !strings.HasPrefix(contentRange, "bytes 0-") { - return fsys.OpenAtEnd(filePath) + return fsys.OpenAppendable(safePath) } - return fsys.Open(filePath) + return fsys.OpenWritable(safePath) }() if err != nil { @@ -170,11 +174,13 @@ func uploads(router *httprouter.Router, fsys MkdirFS) { }) } -func downloads(router *httprouter.Router, fsys fs.FS) { +func downloads(router *httprouter.Router, baseDir string, fsys fs.FS) { router.GET("/_apis/pipelines/workflows/:runId/artifacts", func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { runID := params.ByName("runId") - entries, err := fs.ReadDir(fsys, runID) + safePath := safeResolve(baseDir, runID) + + entries, err := fs.ReadDir(fsys, safePath) if err != nil { panic(err) } @@ -204,12 +210,12 @@ func downloads(router *httprouter.Router, fsys fs.FS) { router.GET("/download/:container", func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { container := params.ByName("container") itemPath := req.URL.Query().Get("itemPath") - dirPath := fmt.Sprintf("%s/%s", container, itemPath) + safePath := safeResolve(baseDir, filepath.Join(container, itemPath)) var files []ContainerItem - err := fs.WalkDir(fsys, dirPath, func(path string, entry fs.DirEntry, err error) error { + err := fs.WalkDir(fsys, safePath, func(path string, entry fs.DirEntry, err error) error { if !entry.IsDir() { - rel, err := filepath.Rel(dirPath, path) + rel, err := filepath.Rel(safePath, path) if err != nil { panic(err) } @@ -218,7 +224,7 @@ func downloads(router *httprouter.Router, fsys fs.FS) { rel = strings.TrimSuffix(rel, gzipExtension) files = append(files, ContainerItem{ - Path: fmt.Sprintf("%s/%s", itemPath, rel), + Path: filepath.Join(itemPath, rel), ItemType: "file", ContentLocation: fmt.Sprintf("http://%s/artifact/%s/%s/%s", req.Host, container, itemPath, rel), }) @@ -245,10 +251,12 @@ func downloads(router *httprouter.Router, fsys fs.FS) { router.GET("/artifact/*path", func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { path := params.ByName("path")[1:] - file, err := fsys.Open(path) + safePath := safeResolve(baseDir, path) + + file, err := fsys.Open(safePath) if err != nil { // try gzip file - file, err = fsys.Open(path + gzipExtension) + file, err = fsys.Open(safePath + gzipExtension) if err != nil { panic(err) } @@ -273,9 +281,9 @@ func Serve(ctx context.Context, artifactPath string, addr string, port string) c router := httprouter.New() logger.Debugf("Artifacts base path '%s'", artifactPath) - fs := os.DirFS(artifactPath) - uploads(router, MkdirFsImpl{artifactPath, fs}) - downloads(router, fs) + fsys := readWriteFSImpl{} + uploads(router, artifactPath, fsys) + downloads(router, artifactPath, fsys) server := &http.Server{ Addr: fmt.Sprintf("%s:%s", addr, port), diff --git a/pkg/artifacts/server_test.go b/pkg/artifacts/server_test.go index 6f865dc2..259c9542 100644 --- a/pkg/artifacts/server_test.go +++ b/pkg/artifacts/server_test.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "io/fs" "net/http" "net/http/httptest" "os" @@ -21,44 +20,43 @@ import ( "github.com/stretchr/testify/assert" ) -type MapFsImpl struct { - fstest.MapFS +type writableMapFile struct { + fstest.MapFile } -func (fsys MapFsImpl) MkdirAll(path string, perm fs.FileMode) error { - // mocked no-op - return nil -} - -type WritableFile struct { - fs.File - fsys fstest.MapFS - path string -} - -func (file WritableFile) Write(data []byte) (int, error) { - file.fsys[file.path].Data = data +func (f *writableMapFile) Write(data []byte) (int, error) { + f.Data = data return len(data), nil } -func (fsys MapFsImpl) Open(path string) (fs.File, error) { - var file = fstest.MapFile{ - Data: []byte("content2"), - } - fsys.MapFS[path] = &file - - result, err := fsys.MapFS.Open(path) - return WritableFile{result, fsys.MapFS, path}, err +func (f *writableMapFile) Close() error { + return nil } -func (fsys MapFsImpl) OpenAtEnd(path string) (fs.File, error) { - var file = fstest.MapFile{ - Data: []byte("content2"), - } - fsys.MapFS[path] = &file +type writeMapFS struct { + fstest.MapFS +} - result, err := fsys.MapFS.Open(path) - return WritableFile{result, fsys.MapFS, path}, err +func (fsys writeMapFS) OpenWritable(name string) (WritableFile, error) { + var file = &writableMapFile{ + MapFile: fstest.MapFile{ + Data: []byte("content2"), + }, + } + fsys.MapFS[name] = &file.MapFile + + return file, nil +} + +func (fsys writeMapFS) OpenAppendable(name string) (WritableFile, error) { + var file = &writableMapFile{ + MapFile: fstest.MapFile{ + Data: []byte("content2"), + }, + } + fsys.MapFS[name] = &file.MapFile + + return file, nil } func TestNewArtifactUploadPrepare(t *testing.T) { @@ -67,7 +65,7 @@ func TestNewArtifactUploadPrepare(t *testing.T) { var memfs = fstest.MapFS(map[string]*fstest.MapFile{}) router := httprouter.New() - uploads(router, MapFsImpl{memfs}) + uploads(router, "artifact/server/path", writeMapFS{memfs}) req, _ := http.NewRequest("POST", "http://localhost/_apis/pipelines/workflows/1/artifacts", nil) rr := httptest.NewRecorder() @@ -93,7 +91,7 @@ func TestArtifactUploadBlob(t *testing.T) { var memfs = fstest.MapFS(map[string]*fstest.MapFile{}) router := httprouter.New() - uploads(router, MapFsImpl{memfs}) + uploads(router, "artifact/server/path", writeMapFS{memfs}) req, _ := http.NewRequest("PUT", "http://localhost/upload/1?itemPath=some/file", strings.NewReader("content")) rr := httptest.NewRecorder() @@ -111,7 +109,7 @@ func TestArtifactUploadBlob(t *testing.T) { } assert.Equal("success", response.Message) - assert.Equal("content", string(memfs["1/some/file"].Data)) + assert.Equal("content", string(memfs["artifact/server/path/1/some/file"].Data)) } func TestFinalizeArtifactUpload(t *testing.T) { @@ -120,7 +118,7 @@ func TestFinalizeArtifactUpload(t *testing.T) { var memfs = fstest.MapFS(map[string]*fstest.MapFile{}) router := httprouter.New() - uploads(router, MapFsImpl{memfs}) + uploads(router, "artifact/server/path", writeMapFS{memfs}) req, _ := http.NewRequest("PATCH", "http://localhost/_apis/pipelines/workflows/1/artifacts", nil) rr := httptest.NewRecorder() @@ -144,13 +142,13 @@ func TestListArtifacts(t *testing.T) { assert := assert.New(t) var memfs = fstest.MapFS(map[string]*fstest.MapFile{ - "1/file.txt": { + "artifact/server/path/1/file.txt": { Data: []byte(""), }, }) router := httprouter.New() - downloads(router, memfs) + downloads(router, "artifact/server/path", memfs) req, _ := http.NewRequest("GET", "http://localhost/_apis/pipelines/workflows/1/artifacts", nil) rr := httptest.NewRecorder() @@ -176,13 +174,13 @@ func TestListArtifactContainer(t *testing.T) { assert := assert.New(t) var memfs = fstest.MapFS(map[string]*fstest.MapFile{ - "1/some/file": { + "artifact/server/path/1/some/file": { Data: []byte(""), }, }) router := httprouter.New() - downloads(router, memfs) + downloads(router, "artifact/server/path", memfs) req, _ := http.NewRequest("GET", "http://localhost/download/1?itemPath=some/file", nil) rr := httptest.NewRecorder() @@ -200,7 +198,7 @@ func TestListArtifactContainer(t *testing.T) { } assert.Equal(1, len(response.Value)) - assert.Equal("some/file/.", response.Value[0].Path) + assert.Equal("some/file", response.Value[0].Path) assert.Equal("file", response.Value[0].ItemType) assert.Equal("http://localhost/artifact/1/some/file/.", response.Value[0].ContentLocation) } @@ -209,13 +207,13 @@ func TestDownloadArtifactFile(t *testing.T) { assert := assert.New(t) var memfs = fstest.MapFS(map[string]*fstest.MapFile{ - "1/some/file": { + "artifact/server/path/1/some/file": { Data: []byte("content"), }, }) router := httprouter.New() - downloads(router, memfs) + downloads(router, "artifact/server/path", memfs) req, _ := http.NewRequest("GET", "http://localhost/artifact/1/some/file", nil) rr := httptest.NewRecorder() @@ -260,6 +258,7 @@ func TestArtifactFlow(t *testing.T) { tables := []TestJobFileInfo{ {"testdata", "upload-and-download", "push", "", platforms, ""}, + {"testdata", "GHSL-2023-004", "push", "", platforms, ""}, } log.SetLevel(log.DebugLevel) @@ -310,3 +309,81 @@ func runTestJobFile(ctx context.Context, t *testing.T, tjfi TestJobFileInfo) { fmt.Println("::endgroup::") }) } + +func TestMkdirFsImplSafeResolve(t *testing.T) { + assert := assert.New(t) + + baseDir := "/foo/bar" + + tests := map[string]struct { + input string + want string + }{ + "simple": {input: "baz", want: "/foo/bar/baz"}, + "nested": {input: "baz/blue", want: "/foo/bar/baz/blue"}, + "dots in middle": {input: "baz/../../blue", want: "/foo/bar/blue"}, + "leading dots": {input: "../../parent", want: "/foo/bar/parent"}, + "root path": {input: "/root", want: "/foo/bar/root"}, + "root": {input: "/", want: "/foo/bar"}, + "empty": {input: "", want: "/foo/bar"}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(tc.want, safeResolve(baseDir, tc.input)) + }) + } +} + +func TestDownloadArtifactFileUnsafePath(t *testing.T) { + assert := assert.New(t) + + var memfs = fstest.MapFS(map[string]*fstest.MapFile{ + "artifact/server/path/some/file": { + Data: []byte("content"), + }, + }) + + router := httprouter.New() + downloads(router, "artifact/server/path", memfs) + + req, _ := http.NewRequest("GET", "http://localhost/artifact/2/../../some/file", nil) + rr := httptest.NewRecorder() + + router.ServeHTTP(rr, req) + + if status := rr.Code; status != http.StatusOK { + assert.FailNow(fmt.Sprintf("Wrong status: %d", status)) + } + + data := rr.Body.Bytes() + + assert.Equal("content", string(data)) +} + +func TestArtifactUploadBlobUnsafePath(t *testing.T) { + assert := assert.New(t) + + var memfs = fstest.MapFS(map[string]*fstest.MapFile{}) + + router := httprouter.New() + uploads(router, "artifact/server/path", writeMapFS{memfs}) + + req, _ := http.NewRequest("PUT", "http://localhost/upload/1?itemPath=../../some/file", strings.NewReader("content")) + rr := httptest.NewRecorder() + + router.ServeHTTP(rr, req) + + if status := rr.Code; status != http.StatusOK { + assert.Fail("Wrong status") + } + + response := ResponseMessage{} + err := json.Unmarshal(rr.Body.Bytes(), &response) + if err != nil { + panic(err) + } + + assert.Equal("success", response.Message) + assert.Equal("content", string(memfs["artifact/server/path/1/some/file"].Data)) +} diff --git a/pkg/artifacts/testdata/GHSL-2023-004/artifacts.yml b/pkg/artifacts/testdata/GHSL-2023-004/artifacts.yml new file mode 100644 index 00000000..e717f141 --- /dev/null +++ b/pkg/artifacts/testdata/GHSL-2023-004/artifacts.yml @@ -0,0 +1,43 @@ + +name: "GHSL-2023-0004" +on: push + +jobs: + test-artifacts: + runs-on: ubuntu-latest + steps: + - run: echo "hello world" > test.txt + - name: curl upload + uses: wei/curl@v1 + with: + args: -s --fail ${ACTIONS_RUNTIME_URL}upload/1?itemPath=../../my-artifact/secret.txt --upload-file test.txt + - uses: actions/download-artifact@v2 + with: + name: my-artifact + path: test-artifacts + - name: 'Verify Artifact #1' + run: | + file="test-artifacts/secret.txt" + if [ ! -f $file ] ; then + echo "Expected file does not exist" + exit 1 + fi + if [ "$(cat $file)" != "hello world" ] ; then + echo "File contents of downloaded artifact are incorrect" + exit 1 + fi + - name: Verify download should work by clean extra dots + uses: wei/curl@v1 + with: + args: --path-as-is -s -o out.txt --fail ${ACTIONS_RUNTIME_URL}artifact/1/../../../1/my-artifact/secret.txt + - name: 'Verify download content' + run: | + file="out.txt" + if [ ! -f $file ] ; then + echo "Expected file does not exist" + exit 1 + fi + if [ "$(cat $file)" != "hello world" ] ; then + echo "File contents of downloaded artifact are incorrect" + exit 1 + fi