Skip to content

Commit adab2f2

Browse files
authored
Merge commit from fork
The file store unpack path (pushDir -> extractTarDirectory) validated symlink targets only lexically and skipped the parent-symlink check for root-level entries, then wrote regular files with O_CREATE|O_TRUNC and no O_NOFOLLOW. A malicious unpack layer could plant a chain of symlinks whose lexical target stays inside the extraction root but whose kernel-resolved target is an arbitrary absolute path, then write through it with a same-named regular-file entry, creating or overwriting files outside the store working directory under the default AllowPathTraversalOnWrite=false. Fix: - writeFile removes a pre-existing terminal symlink before opening, so a regular-file entry can never be written through a link left by an earlier entry. - extractTarDirectory re-verifies containment with symlinks resolved via the existing checkSymlinkEscape helper, matching the pushFile path. Adds a regression test reproducing the symlink-chain escape. Signed-off-by: Terry Howe <terrylhowe@gmail.com>
1 parent 948a1dc commit adab2f2

2 files changed

Lines changed: 127 additions & 0 deletions

File tree

content/file/file_unix_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,16 @@ limitations under the License.
1818
package file
1919

2020
import (
21+
"archive/tar"
2122
"bytes"
23+
"compress/gzip"
2224
"context"
2325
"os"
2426
"path/filepath"
27+
"strings"
2528
"testing"
2629

30+
"github.com/opencontainers/go-digest"
2731
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
2832
"oras.land/oras-go/v2"
2933
)
@@ -472,3 +476,107 @@ func TestStore_Dir_OverwriteSymlink_RemovalFailed(t *testing.T) {
472476
t.Fatal("error calling Chmod(), error =", err)
473477
}
474478
}
479+
480+
// TestStore_Dir_SymlinkChainEscape reproduces GHSA-m37j-52j7-pjw7: a malicious
481+
// unpack layer plants a chain of symlinks whose lexical target stays inside the
482+
// extraction root but whose kernel-resolved target is an absolute path outside
483+
// it, then writes a regular file with the same name to write through the link.
484+
// The write must be contained inside the store's working directory.
485+
func TestStore_Dir_SymlinkChainEscape(t *testing.T) {
486+
workDir := t.TempDir()
487+
// attacker's target lives outside the store working directory
488+
outsideDir := t.TempDir()
489+
outsidePath := filepath.Join(outsideDir, "PWNED")
490+
491+
const title = "out"
492+
baseAbs := filepath.Join(workDir, title)
493+
depth := len(strings.Split(strings.Trim(filepath.ToSlash(baseAbs), "/"), "/"))
494+
495+
// build the malicious tar.gz
496+
var buf bytes.Buffer
497+
gzw := gzip.NewWriter(&buf)
498+
tw := tar.NewWriter(gzw)
499+
500+
dirs := make([]string, depth)
501+
for i := range dirs {
502+
dirs[i] = "d"
503+
}
504+
for i := 1; i <= depth; i++ {
505+
if err := tw.WriteHeader(&tar.Header{
506+
Typeflag: tar.TypeDir,
507+
Name: title + "/" + strings.Join(dirs[:i], "/"),
508+
Mode: 0o755,
509+
}); err != nil {
510+
t.Fatal(err)
511+
}
512+
}
513+
// "up" symlink at the bottom, resolving back to baseAbs
514+
if err := tw.WriteHeader(&tar.Header{
515+
Typeflag: tar.TypeSymlink,
516+
Name: title + "/" + strings.Join(dirs, "/") + "/up",
517+
Linkname: strings.Repeat("../", depth-1) + "..",
518+
Mode: 0o777,
519+
}); err != nil {
520+
t.Fatal(err)
521+
}
522+
// "escape" symlink whose lexical target stays in-bounds but whose
523+
// kernel-resolved target is outsidePath
524+
escapeTarget := strings.Join(dirs, "/") + "/up/" + strings.Repeat("../", depth-1) + ".." + outsidePath
525+
if err := tw.WriteHeader(&tar.Header{
526+
Typeflag: tar.TypeSymlink,
527+
Name: title + "/escape",
528+
Linkname: escapeTarget,
529+
Mode: 0o777,
530+
}); err != nil {
531+
t.Fatal(err)
532+
}
533+
// regular file with the same name, written through the symlink
534+
payload := []byte("PWNED-BY-ORAS-TARSLIP")
535+
if err := tw.WriteHeader(&tar.Header{
536+
Typeflag: tar.TypeReg,
537+
Name: title + "/escape",
538+
Mode: 0o644,
539+
Size: int64(len(payload)),
540+
}); err != nil {
541+
t.Fatal(err)
542+
}
543+
if _, err := tw.Write(payload); err != nil {
544+
t.Fatal(err)
545+
}
546+
if err := tw.Close(); err != nil {
547+
t.Fatal(err)
548+
}
549+
if err := gzw.Close(); err != nil {
550+
t.Fatal(err)
551+
}
552+
blob := buf.Bytes()
553+
554+
desc := ocispec.Descriptor{
555+
MediaType: ocispec.MediaTypeImageLayerGzip,
556+
Digest: digest.FromBytes(blob),
557+
Size: int64(len(blob)),
558+
Annotations: map[string]string{
559+
ocispec.AnnotationTitle: title,
560+
AnnotationUnpack: "true",
561+
},
562+
}
563+
564+
s, err := New(workDir)
565+
if err != nil {
566+
t.Fatal("Store.New() error =", err)
567+
}
568+
defer s.Close()
569+
if s.AllowPathTraversalOnWrite {
570+
t.Fatal("expected AllowPathTraversalOnWrite to default to false")
571+
}
572+
573+
// Push may succeed (write contained inside the store) or fail, but under no
574+
// circumstances may it write outside the working directory.
575+
_ = s.Push(context.Background(), desc, bytes.NewReader(blob))
576+
577+
if _, err := os.Stat(outsidePath); err == nil {
578+
t.Fatalf("path traversal: file written outside working dir at %s", outsidePath)
579+
} else if !os.IsNotExist(err) {
580+
t.Fatalf("unexpected error stat-ing %s: %v", outsidePath, err)
581+
}
582+
}

content/file/utils.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,15 @@ func extractTarDirectory(dirPath, dirName string, r io.Reader, buf []byte, prese
175175
}
176176
filePath := filepath.Join(dirPath, filePathRel)
177177

178+
// resolveRelToBase only performs lexical and per-component Lstat checks,
179+
// which a chain of previously-extracted symlinks can bypass. Re-verify
180+
// containment with symlinks fully resolved before mutating the
181+
// filesystem, matching the check on the pushFile path.
182+
// (GHSA-m37j-52j7-pjw7)
183+
if err := checkSymlinkEscape(dirPath, filePath); err != nil {
184+
return err
185+
}
186+
178187
// Create content
179188
switch header.Typeflag {
180189
case tar.TypeReg:
@@ -281,6 +290,16 @@ func ensureLinkPath(baseAbs, baseRel, link, target string) (string, error) {
281290

282291
// writeFile writes content to the file specified by the `path` parameter.
283292
func writeFile(path string, r io.Reader, perm os.FileMode, buf []byte) (err error) {
293+
// os.OpenFile follows a terminal symlink, so a regular-file entry whose
294+
// path was already created as a symlink by an earlier archive entry would
295+
// be written through that link, landing outside the extraction root
296+
// (GHSA-m37j-52j7-pjw7). Remove any such symlink first so the content is
297+
// written to a regular file at path itself.
298+
if fi, err := os.Lstat(path); err == nil && fi.Mode()&os.ModeSymlink != 0 {
299+
if err := os.Remove(path); err != nil {
300+
return err
301+
}
302+
}
284303
file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
285304
if err != nil {
286305
return err

0 commit comments

Comments
 (0)