From 9e0a3401f6c9c1beb559b775a7c25f6330a75e73 Mon Sep 17 00:00:00 2001 From: Denis Ollier Date: Wed, 22 Apr 2026 21:54:41 +0200 Subject: [PATCH] Include symbolic links during local source build upload Until now, the `shp build upload /path` command would silently discard symbolic links when walking through a local source directory. This is not backward compatible with the `oc start-build --from-dir=/path` command which supports symbolic links. This change adds support for symbolic links to Shipwright cli to match the legacy OpenShift build commands. To prevent the type of vulnerability described by CWE-61 [1], an additional security measure is implemented: an error is raised upon detecting a symbolic link that points to a file outside the provided directory. [1]: https://cwe.mitre.org/data/definitions/61.html Signed-off-by: Denis Ollier --- docs/shp_build_upload.md | 4 +- pkg/shp/cmd/build/upload.go | 6 +-- pkg/shp/streamer/tar.go | 14 ++++--- pkg/shp/streamer/tar_test.go | 39 +++++++++++++++++-- pkg/shp/streamer/util.go | 73 ++++++++++++++++++++++++++++++++---- 5 files changed, 114 insertions(+), 22 deletions(-) diff --git a/docs/shp_build_upload.md b/docs/shp_build_upload.md index 19fd2b13d..721c5d651 100644 --- a/docs/shp_build_upload.md +++ b/docs/shp_build_upload.md @@ -17,8 +17,8 @@ In case a source bundle image is defined, the bundling feature is used, which wi source code into a bundle container and upload it to the specified container registry. Instead of executing using Git in the source step, it will use the container registry to obtain the source code. - $ shp buildrun upload - $ shp buildrun upload /path/to/repository + $ shp build upload + $ shp build upload /path/to/repository ``` diff --git a/pkg/shp/cmd/build/upload.go b/pkg/shp/cmd/build/upload.go index f75bea9b1..030838235 100644 --- a/pkg/shp/cmd/build/upload.go +++ b/pkg/shp/cmd/build/upload.go @@ -56,8 +56,8 @@ In case a source bundle image is defined, the bundling feature is used, which wi source code into a bundle container and upload it to the specified container registry. Instead of executing using Git in the source step, it will use the container registry to obtain the source code. - $ shp buildrun upload - $ shp buildrun upload /path/to/repository + $ shp build upload + $ shp build upload /path/to/repository ` // targetBaseDir directory where data will be uploaded. @@ -211,7 +211,7 @@ func (u *UploadCommand) performDataStreaming(target *streamer.Target) error { fmt.Fprintf(u.ioStreams.Out, "Streaming %q to the Build POD %q ...\n", u.sourceDir, target.Pod) // creates an in-memory tarball with source directory data, and ready to start data streaming - tarball, err := streamer.NewTar(u.sourceDir) + tarball, err := streamer.NewTar(u.sourceDir, u.ioStreams) if err != nil { return err } diff --git a/pkg/shp/streamer/tar.go b/pkg/shp/streamer/tar.go index e2255f7c7..98a9ad230 100644 --- a/pkg/shp/streamer/tar.go +++ b/pkg/shp/streamer/tar.go @@ -10,18 +10,20 @@ import ( "strings" ignore "github.com/sabhiram/go-gitignore" + "k8s.io/cli-runtime/pkg/genericclioptions" ) // Tar helper to create a tar instance based on a source directory, skipping entries that are not // desired like `.git` directory and entries in `.gitignore` file. type Tar struct { - src string // base directory - gitIgnore *ignore.GitIgnore // matcher for git ignored files + src string // base directory + gitIgnore *ignore.GitIgnore // matcher for git ignored files + ioStreams *genericclioptions.IOStreams // io streams for user-facing output } // skipPath inspect each path and makes sure it skips files the tar helper can't handle. func (t *Tar) skipPath(fpath string, stat fs.FileInfo) bool { - if !stat.Mode().IsRegular() { + if !stat.Mode().IsRegular() && stat.Mode()&fs.ModeSymlink == 0 { return true } if strings.HasPrefix(fpath, path.Join(t.src, ".git")) { @@ -43,7 +45,7 @@ func (t *Tar) Create(w io.Writer) error { if t.skipPath(fpath, stat) { return nil } - return writeFileToTar(tw, t.src, fpath, stat) + return writeFileToTar(tw, t, fpath, stat) }); err != nil { return err } @@ -63,8 +65,8 @@ func (t *Tar) bootstrap() error { } // NewTar instantiate a tar helper based on the source directory path informed. -func NewTar(src string) (*Tar, error) { - t := &Tar{src: src} +func NewTar(src string, ioStreams *genericclioptions.IOStreams) (*Tar, error) { + t := &Tar{src: src, ioStreams: ioStreams} return t, t.bootstrap() } diff --git a/pkg/shp/streamer/tar_test.go b/pkg/shp/streamer/tar_test.go index 810e6e4f1..11801b151 100644 --- a/pkg/shp/streamer/tar_test.go +++ b/pkg/shp/streamer/tar_test.go @@ -3,29 +3,57 @@ package streamer import ( "archive/tar" "io" + "os" "strings" "testing" o "github.com/onsi/gomega" + "k8s.io/cli-runtime/pkg/genericclioptions" ) func Test_Tar(t *testing.T) { g := o.NewGomegaWithT(t) - tarHelper, err := NewTar("../../..") + baseDir := "../../.." + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() + + tarHelper, err := NewTar(baseDir, &ioStreams) g.Expect(err).To(o.BeNil()) reader, writer := io.Pipe() defer reader.Close() defer writer.Close() + // Ensure symlinks with a target outside the source tree are rejected + symlink := baseDir + "/test/" + "symlink_for_test_tar" + if err := os.Symlink("../../path/outside/source/tree", symlink); err != nil { + t.Fatalf("Failed to setup test symlink pointing outside the source tree: %v", err) + } + t.Cleanup(func() { + cleanupErr := os.Remove(symlink) + if cleanupErr != nil { + t.Logf("Failed to cleanup test symlink %q: %v", symlink, cleanupErr) + } + }) + + err = tarHelper.Create(io.Discard) + g.Expect(err.Error()).To(o.ContainSubstring("points outside the source directory")) + + // Override the symlink with a target inside the source tree and retest + if err := os.Remove(symlink); err != nil { + t.Fatalf("Failed to cleanup test symlink %q: %v", symlink, err) + } + if err := os.Symlink("../path/inside/source/tree", symlink); err != nil { + t.Fatalf("Failed to setup test symlink pointing inside the source tree: %v", err) + } + go func() { err := tarHelper.Create(writer) g.Expect(err).To(o.BeNil()) }() tarReader := tar.NewReader(reader) - counter := 0 + counter, symlinks := 0, 0 for { header, err := tarReader.Next() if err != nil { @@ -37,10 +65,15 @@ func Test_Tar(t *testing.T) { counter++ name := header.Name - // making sure that undesired entries are not present on the list of files caputured by the + if header.Linkname != "" { + symlinks++ + } + + // making sure that undesired entries are not present on the list of files captured by the // tar helper g.Expect(strings.HasPrefix(name, ".git/")).To(o.BeFalse()) g.Expect(strings.HasPrefix(name, "_output/")).To(o.BeFalse()) } g.Expect(counter > 10).To(o.BeTrue()) + g.Expect(symlinks > 0).To(o.BeTrue()) } diff --git a/pkg/shp/streamer/util.go b/pkg/shp/streamer/util.go index 4fa42b724..5959f3a5c 100644 --- a/pkg/shp/streamer/util.go +++ b/pkg/shp/streamer/util.go @@ -2,6 +2,7 @@ package streamer import ( "archive/tar" + "fmt" "io" "io/fs" "os" @@ -21,24 +22,80 @@ func trimPrefix(prefix, fpath string) string { return strings.TrimPrefix(strings.ReplaceAll(fpath, prefix, ""), string(filepath.Separator)) } -func writeFileToTar(tw *tar.Writer, src, fpath string, stat fs.FileInfo) error { +func writeFileToTar(tw *tar.Writer, t *Tar, fpath string, stat fs.FileInfo) error { header, err := tar.FileInfoHeader(stat, stat.Name()) if err != nil { return err } - header.Name = trimPrefix(src, fpath) + header.Name = trimPrefix(t.src, fpath) + + // Symlink + if stat.Mode()&fs.ModeSymlink != 0 { + target, err := os.Readlink(fpath) + if err != nil { + return err + } + + // resolving target to absolute path, to determine if it is pointing + // outside the source directory. + var absSrc, absTarget string + + if absSrc, err = filepath.Abs(t.src); err != nil { + return err + } + + if filepath.IsAbs(target) { + absTarget = target + } else { + fullTarget := filepath.Join(filepath.Dir(fpath), target) + if absTarget, err = filepath.Abs(fullTarget); err != nil { + return err + } + } + + if outside, _ := isSymlinkTargetOutsideOfDir(absSrc, absTarget); outside { + relPath, _ := filepath.Rel(t.src, fpath) + return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)", relPath, absSrc, target) + } + + header.Linkname = target + } + if err := tw.WriteHeader(header); err != nil { return err } - // #nosec G304 intentionally opening file from variable - f, err := os.Open(fpath) + // Copy regular file content + if stat.Mode().IsRegular() { + // #nosec G304 intentionally opening file from variable + f, err := os.Open(fpath) + if err != nil { + return err + } + if _, err := io.Copy(tw, f); err != nil { + return err + } + return f.Close() + } + + return nil +} + +// isSymlinkTargetOutsideOfDir checks if the provided link target resolves to an absolute path outside +// of the provided directory. Symlinks to absolute paths outside of a given "root" directory is a common +// attack vector, as these can potentially bypass operating system permission checks that block access to +// sensitive data. See [CWE-61](https://cwe.mitre.org/data/definitions/61.html). +func isSymlinkTargetOutsideOfDir(dir, target string) (bool, error) { + realTarget, err := filepath.EvalSymlinks(target) if err != nil { - return err + realTarget = filepath.Clean(target) } - if _, err := io.Copy(tw, f); err != nil { - return err + + rel, err := filepath.Rel(dir, realTarget) + if err != nil { + return false, err } - return f.Close() + + return !filepath.IsLocal(rel), nil }