From 159486c769b5b1adec8b1aa87810f06aa27a5eea Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Fri, 19 Jan 2024 14:27:40 +0100 Subject: [PATCH 1/3] fix(pkg,internal): multiple fixes related to cos and driver build. Signed-off-by: Federico Di Pierro --- internal/utils/extract.go | 20 +++++++++++++++++--- pkg/driver/distro/cos.go | 5 +++-- pkg/driver/distro/distro.go | 11 ++++++++--- pkg/driver/type/bpf.go | 1 + 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/internal/utils/extract.go b/internal/utils/extract.go index 92e31421..27cd7120 100644 --- a/internal/utils/extract.go +++ b/internal/utils/extract.go @@ -58,7 +58,7 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int) switch header.Typeflag { case tar.TypeDir: d := filepath.Join(destDir, strippedName) - if err = os.Mkdir(filepath.Clean(d), 0o750); err != nil { + if err = os.MkdirAll(filepath.Clean(d), 0o750); err != nil { return nil, err } files = append(files, d) @@ -76,8 +76,22 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int) if err = outFile.Close(); err != nil { return nil, err } + if err = os.Chmod(filepath.Clean(f), header.FileInfo().Mode()); err != nil { + return nil, err + } files = append(files, f) - + case tar.TypeLink, tar.TypeSymlink: + strippedSrcName := stripComponents(header.Linkname, stripPathComponents) + fDst := filepath.Join(destDir, strippedName) + if header.Typeflag == tar.TypeSymlink { + err = os.Symlink(filepath.Clean(strippedSrcName), filepath.Clean(fDst)) + } else { + err = os.Link(filepath.Clean(strippedSrcName), filepath.Clean(fDst)) + } + if err != nil { + return nil, err + } + files = append(files, fDst) default: return nil, fmt.Errorf("extractTarGz: uknown type: %b in %s", header.Typeflag, header.Name) } @@ -96,5 +110,5 @@ func stripComponents(headerName string, stripComponents int) string { if len(names) < stripComponents { return headerName } - return filepath.Clean(strings.Join(names[stripComponents:], "/")) + return filepath.Clean(strings.Join(names[stripComponents:], string(os.PathSeparator))) } diff --git a/pkg/driver/distro/cos.go b/pkg/driver/distro/cos.go index 1e99de23..3201d961 100644 --- a/pkg/driver/distro/cos.go +++ b/pkg/driver/distro/cos.go @@ -18,6 +18,7 @@ package driverdistro import ( "fmt" "os" + "path/filepath" "github.com/blang/semver" "github.com/falcosecurity/driverkit/pkg/kernelrelease" @@ -76,7 +77,7 @@ func (c *cos) customizeBuild(ctx context.Context, currKernelDir := env[kernelDirEnv] - cosKernelDir := currKernelDir + "usr/src/" + cosKernelDir := filepath.Join(currKernelDir, "usr", "src") entries, err := os.ReadDir(cosKernelDir) if err != nil { return nil, err @@ -84,7 +85,7 @@ func (c *cos) customizeBuild(ctx context.Context, if len(entries) == 0 { return nil, fmt.Errorf("no COS kernel src found") } - cosKernelDir = entries[0].Name() + cosKernelDir = filepath.Join(cosKernelDir, entries[0].Name()) // Override env key env[kernelDirEnv] = cosKernelDir diff --git a/pkg/driver/distro/distro.go b/pkg/driver/distro/distro.go index 17ac5bf8..d11f7203 100644 --- a/pkg/driver/distro/distro.go +++ b/pkg/driver/distro/distro.go @@ -17,7 +17,7 @@ package driverdistro import ( - "archive/tar" + "compress/gzip" "errors" "fmt" "io" @@ -336,12 +336,17 @@ func downloadKernelSrc(ctx context.Context, if err != nil { return nil, err } - var src io.Reader + var src io.ReadCloser if strings.HasSuffix(kernelConfigPath, ".gz") { - src = tar.NewReader(f) + src, err = gzip.NewReader(f) + if err != nil { + return env, err + } } else { src = f } + defer src.Close() + fStat, err := f.Stat() if err != nil { return nil, err diff --git a/pkg/driver/type/bpf.go b/pkg/driver/type/bpf.go index 5d976aba..e78ebe4b 100644 --- a/pkg/driver/type/bpf.go +++ b/pkg/driver/type/bpf.go @@ -87,6 +87,7 @@ func (b *bpf) Build(ctx context.Context, makeCmdArgs := fmt.Sprintf(`make -C %q`, filepath.Clean(srcPath)) makeCmd := exec.CommandContext(ctx, "bash", "-c", makeCmdArgs) //nolint:gosec // false positive // Append requested env variables to the command env + makeCmd.Env = os.Environ() for key, val := range env { makeCmd.Env = append(makeCmd.Env, fmt.Sprintf("%s=%s", key, val)) } From 26b8e785cd9058a43e3914802763aa5f97e440a3 Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Fri, 19 Jan 2024 16:36:28 +0100 Subject: [PATCH 2/3] new(internal): added some extractFromTarGz tests. Signed-off-by: Federico Di Pierro --- internal/utils/extract_test.go | 203 +++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 internal/utils/extract_test.go diff --git a/internal/utils/extract_test.go b/internal/utils/extract_test.go new file mode 100644 index 00000000..411df7ba --- /dev/null +++ b/internal/utils/extract_test.go @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2024 The Falco Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package utils + +import ( + "archive/tar" + "compress/gzip" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +const ( + srcDir = "./foo" +) + +var ( + files = []string{srcDir + "/example.txt", srcDir + "/test.txt", srcDir + "/bar/baz.txt"} +) + +func createTarball(t *testing.T, tarballFilePath, srcDir string) { + file, err := os.Create(tarballFilePath) + assert.NoError(t, err) + defer file.Close() + + gzipWriter := gzip.NewWriter(file) + defer gzipWriter.Close() + + tarWriter := tar.NewWriter(gzipWriter) + defer tarWriter.Close() + + err = filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error { + return addToArchive(tarWriter, path, info) + }) + assert.NoError(t, err) +} + +func addToArchive(tw *tar.Writer, fullName string, info os.FileInfo) error { + // Open the file which will be written into the archive + file, err := os.Open(fullName) + if err != nil { + return err + } + defer file.Close() + + // Create a tar Header from the FileInfo data + header, err := tar.FileInfoHeader(info, info.Name()) + if err != nil { + return err + } + + // Use full path as name (FileInfoHeader only takes the basename) + // If we don't do this the directory strucuture would + // not be preserved + // https://golang.org/src/archive/tar/common.go?#L626 + header.Name = fullName + + // Write file header to the tar archive + err = tw.WriteHeader(header) + if err != nil { + return err + } + + if !info.IsDir() { + // Copy file content to tar archive + _, err = io.Copy(tw, file) + if err != nil { + return err + } + } + + return nil +} + +func TestExtractTarGz(t *testing.T) { + // Create src dir + err := os.MkdirAll(srcDir, 0o750) + assert.NoError(t, err) + t.Cleanup(func() { + _ = os.RemoveAll(srcDir) + }) + + // Generate files to be tarballed + for _, f := range files { + err := os.MkdirAll(filepath.Dir(f), 0o755) + assert.NoError(t, err) + _, err = os.Create(f) + assert.NoError(t, err) + } + + // create tarball + createTarball(t, "./test.tgz", srcDir) + t.Cleanup(func() { + _ = os.RemoveAll("./test.tgz") + }) + + // Create dest folder + destDir := "./test/" + err = os.MkdirAll(destDir, 0o750) + assert.NoError(t, err) + t.Cleanup(func() { + _ = os.RemoveAll(destDir) + }) + + // Extract tarball + f, err := os.Open("./test.tgz") + assert.NoError(t, err) + t.Cleanup(func() { + f.Close() + }) + + list, err := ExtractTarGz(f, destDir, 0) + assert.NoError(t, err) + + // Final checks + + // All extracted files are ok + for _, f := range list { + _, err := os.Stat(f) + assert.NoError(t, err) + } + + // Extracted folder contains all source files (plus folders) + for _, f := range files { + path := filepath.Join(destDir, f) + assert.Contains(t, list, path) + } +} + +func TestExtractTarGzStripComponents(t *testing.T) { + // Create src dir + srcDir := "./foo" + err := os.MkdirAll(srcDir, 0o750) + assert.NoError(t, err) + t.Cleanup(func() { + _ = os.RemoveAll(srcDir) + }) + + // Generate files to be tarballed + for _, f := range files { + err := os.MkdirAll(filepath.Dir(f), 0o755) + assert.NoError(t, err) + _, err = os.Create(f) + assert.NoError(t, err) + } + + // create tarball + createTarball(t, "./test.tgz", srcDir) + t.Cleanup(func() { + _ = os.RemoveAll("./test.tgz") + }) + + // Create dest folder + destdirStrip := "./test_strip/" + err = os.MkdirAll(destdirStrip, 0o750) + assert.NoError(t, err) + t.Cleanup(func() { + _ = os.RemoveAll(destdirStrip) + }) + + // Extract tarball + f, err := os.Open("./test.tgz") + assert.NoError(t, err) + t.Cleanup(func() { + f.Close() + }) + // NOTE that here we strip first component + list, err := ExtractTarGz(f, destdirStrip, 1) + assert.NoError(t, err) + + // Final checks + + // All extracted files are ok + for _, f := range list { + _, err := os.Stat(f) + assert.NoError(t, err) + } + + // Extracted folder contains all source files (plus folders) + for _, f := range files { + // We stripped first component (ie: srcDir) + ff := strings.TrimPrefix(f, srcDir) + path := filepath.Join(destdirStrip, ff) + assert.Contains(t, list, path) + } +} From a40ea451350fea0a3616422f9065109a5cff1eb0 Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Mon, 22 Jan 2024 17:53:45 +0100 Subject: [PATCH 3/3] chore(cmd,internal,pkg): improve ExtractTarGz algorithm making it more robust and safe. Signed-off-by: Federico Di Pierro --- cmd/artifact/install/install.go | 2 +- internal/follower/follower.go | 2 +- internal/utils/extract.go | 120 ++++++++++++++++++++++++-------- internal/utils/extract_test.go | 19 +++-- pkg/driver/distro/distro.go | 2 +- 5 files changed, 106 insertions(+), 39 deletions(-) diff --git a/cmd/artifact/install/install.go b/cmd/artifact/install/install.go index b8f49a26..60a90669 100644 --- a/cmd/artifact/install/install.go +++ b/cmd/artifact/install/install.go @@ -337,7 +337,7 @@ func (o *artifactInstallOptions) RunArtifactInstall(ctx context.Context, args [] return err } // Extract artifact and move it to its destination directory - _, err = utils.ExtractTarGz(f, destDir, 0) + _, err = utils.ExtractTarGz(ctx, f, destDir, 0) if err != nil { return fmt.Errorf("cannot extract %q to %q: %w", result.Filename, destDir, err) } diff --git a/internal/follower/follower.go b/internal/follower/follower.go index 398c6fcf..36060d38 100644 --- a/internal/follower/follower.go +++ b/internal/follower/follower.go @@ -291,7 +291,7 @@ func (f *Follower) pull(ctx context.Context) (filePaths []string, res *oci.Regis } // Extract artifact and move it to its destination directory - filePaths, err = utils.ExtractTarGz(file, f.tmpDir, 0) + filePaths, err = utils.ExtractTarGz(ctx, file, f.tmpDir, 0) if err != nil { return filePaths, res, fmt.Errorf("unable to extract %q to %q: %w", res.Filename, f.tmpDir, err) } diff --git a/internal/utils/extract.go b/internal/utils/extract.go index 27cd7120..72ec6571 100644 --- a/internal/utils/extract.go +++ b/internal/utils/extract.go @@ -24,12 +24,30 @@ import ( "os" "path/filepath" "strings" + + "golang.org/x/net/context" ) +type link struct { + Name string + Path string +} + // ExtractTarGz extracts a *.tar.gz compressed archive and moves its content to destDir. // Returns a slice containing the full path of the extracted files. -func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int) ([]string, error) { - var files []string +func ExtractTarGz(ctx context.Context, gzipStream io.Reader, destDir string, stripPathComponents int) ([]string, error) { + var ( + files []string + links []link + symlinks []link + err error + ) + + // We need an absolute path + destDir, err = filepath.Abs(destDir) + if err != nil { + return nil, err + } uncompressedStream, err := gzip.NewReader(gzipStream) if err != nil { @@ -37,34 +55,46 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int) } tarReader := tar.NewReader(uncompressedStream) - for { - header, err := tarReader.Next() + select { + case <-ctx.Done(): + return nil, errors.New("interrupted") + default: + } + header, err := tarReader.Next() if errors.Is(err, io.EOF) { break } - if err != nil { return nil, err } - if strings.Contains(header.Name, "..") { return nil, fmt.Errorf("not allowed relative path in tar archive") } - strippedName := stripComponents(header.Name, stripPathComponents) + path := header.Name + if stripPathComponents > 0 { + path = stripComponents(path, stripPathComponents) + } + if path == "" { + continue + } + + if path, err = safeConcat(destDir, filepath.Clean(path)); err != nil { + // Skip paths that would escape destDir + continue + } + info := header.FileInfo() + files = append(files, path) switch header.Typeflag { case tar.TypeDir: - d := filepath.Join(destDir, strippedName) - if err = os.MkdirAll(filepath.Clean(d), 0o750); err != nil { + if err = os.MkdirAll(path, info.Mode()); err != nil { return nil, err } - files = append(files, d) case tar.TypeReg: - f := filepath.Join(destDir, strippedName) - outFile, err := os.Create(filepath.Clean(f)) + outFile, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, info.Mode()) if err != nil { return nil, err } @@ -76,27 +106,46 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int) if err = outFile.Close(); err != nil { return nil, err } - if err = os.Chmod(filepath.Clean(f), header.FileInfo().Mode()); err != nil { - return nil, err - } - files = append(files, f) - case tar.TypeLink, tar.TypeSymlink: - strippedSrcName := stripComponents(header.Linkname, stripPathComponents) - fDst := filepath.Join(destDir, strippedName) - if header.Typeflag == tar.TypeSymlink { - err = os.Symlink(filepath.Clean(strippedSrcName), filepath.Clean(fDst)) - } else { - err = os.Link(filepath.Clean(strippedSrcName), filepath.Clean(fDst)) + case tar.TypeLink: + name := header.Linkname + if stripPathComponents > 0 { + name = stripComponents(name, stripPathComponents) } - if err != nil { - return nil, err + if name == "" { + continue } - files = append(files, fDst) + + name = filepath.Join(destDir, filepath.Clean(name)) + links = append(links, link{Path: path, Name: name}) + case tar.TypeSymlink: + symlinks = append(symlinks, link{Path: path, Name: header.Linkname}) default: return nil, fmt.Errorf("extractTarGz: uknown type: %b in %s", header.Typeflag, header.Name) } } + // Now we make another pass creating the links + for i := range links { + select { + case <-ctx.Done(): + return nil, errors.New("interrupted") + default: + } + if err = os.Link(links[i].Name, links[i].Path); err != nil { + return nil, err + } + } + + for i := range symlinks { + select { + case <-ctx.Done(): + return nil, errors.New("interrupted") + default: + } + if err = os.Symlink(symlinks[i].Name, symlinks[i].Path); err != nil { + return nil, err + } + } return files, nil } @@ -104,11 +153,22 @@ func stripComponents(headerName string, stripComponents int) string { if stripComponents == 0 { return headerName } - names := strings.FieldsFunc(headerName, func(r rune) bool { - return r == os.PathSeparator - }) + names := strings.Split(headerName, string(filepath.Separator)) if len(names) < stripComponents { return headerName } - return filepath.Clean(strings.Join(names[stripComponents:], string(os.PathSeparator))) + return filepath.Clean(strings.Join(names[stripComponents:], string(filepath.Separator))) +} + +// safeConcat concatenates destDir and name +// but returns an error if the resulting path points outside 'destDir'. +func safeConcat(destDir, name string) (string, error) { + res := filepath.Join(destDir, name) + if !strings.HasSuffix(destDir, string(os.PathSeparator)) { + destDir += string(os.PathSeparator) + } + if !strings.HasPrefix(res, destDir) { + return res, fmt.Errorf("unsafe path concatenation: '%s' with '%s'", destDir, name) + } + return res, nil } diff --git a/internal/utils/extract_test.go b/internal/utils/extract_test.go index 411df7ba..c70916c1 100644 --- a/internal/utils/extract_test.go +++ b/internal/utils/extract_test.go @@ -25,6 +25,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "golang.org/x/net/context" ) const ( @@ -112,7 +113,7 @@ func TestExtractTarGz(t *testing.T) { }) // Create dest folder - destDir := "./test/" + destDir := "./test" err = os.MkdirAll(destDir, 0o750) assert.NoError(t, err) t.Cleanup(func() { @@ -126,10 +127,11 @@ func TestExtractTarGz(t *testing.T) { f.Close() }) - list, err := ExtractTarGz(f, destDir, 0) + list, err := ExtractTarGz(context.TODO(), f, destDir, 0) assert.NoError(t, err) // Final checks + assert.NotEmpty(t, list) // All extracted files are ok for _, f := range list { @@ -138,8 +140,10 @@ func TestExtractTarGz(t *testing.T) { } // Extracted folder contains all source files (plus folders) + absDestDir, err := filepath.Abs(destDir) + assert.NoError(t, err) for _, f := range files { - path := filepath.Join(destDir, f) + path := filepath.Join(absDestDir, f) assert.Contains(t, list, path) } } @@ -168,7 +172,7 @@ func TestExtractTarGzStripComponents(t *testing.T) { }) // Create dest folder - destdirStrip := "./test_strip/" + destdirStrip := "./test_strip" err = os.MkdirAll(destdirStrip, 0o750) assert.NoError(t, err) t.Cleanup(func() { @@ -182,10 +186,11 @@ func TestExtractTarGzStripComponents(t *testing.T) { f.Close() }) // NOTE that here we strip first component - list, err := ExtractTarGz(f, destdirStrip, 1) + list, err := ExtractTarGz(context.TODO(), f, destdirStrip, 1) assert.NoError(t, err) // Final checks + assert.NotEmpty(t, list) // All extracted files are ok for _, f := range list { @@ -194,10 +199,12 @@ func TestExtractTarGzStripComponents(t *testing.T) { } // Extracted folder contains all source files (plus folders) + absDestDirStrip, err := filepath.Abs(destdirStrip) + assert.NoError(t, err) for _, f := range files { // We stripped first component (ie: srcDir) ff := strings.TrimPrefix(f, srcDir) - path := filepath.Join(destdirStrip, ff) + path := filepath.Join(absDestDirStrip, ff) assert.Contains(t, list, path) } } diff --git a/pkg/driver/distro/distro.go b/pkg/driver/distro/distro.go index d11f7203..2fb348c0 100644 --- a/pkg/driver/distro/distro.go +++ b/pkg/driver/distro/distro.go @@ -319,7 +319,7 @@ func downloadKernelSrc(ctx context.Context, return env, err } - _, err = utils.ExtractTarGz(resp.Body, fullKernelDir, stripComponents) + _, err = utils.ExtractTarGz(ctx, resp.Body, fullKernelDir, stripComponents) if err != nil { return env, err }