Skip to content

Commit ce75ddf

Browse files
dmcgowandmcgowan
committed
archive: check whiteout path before removal
Ensure whiteout is not pointing to the current directory or parent directory before removing. Protects against invalid removal of the parent or current directory. Add whiteout related tar tests using manufactured tar conditions. Signed-off-by: Derek McGowan <[email protected]>
1 parent c59bc7e commit ce75ddf

2 files changed

Lines changed: 177 additions & 4 deletions

File tree

‎archive/tar.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ var bufferPool = &sync.Pool{
2626
},
2727
}
2828

29+
var errInvalidArchive = errors.New("invalid archive")
30+
2931
// Diff returns a tar stream of the computed filesystem
3032
// difference between the provided directories.
3133
//
@@ -220,6 +222,15 @@ func Apply(ctx context.Context, root string, r io.Reader) (int64, error) {
220222

221223
originalBase := base[len(whiteoutPrefix):]
222224
originalPath := filepath.Join(dir, originalBase)
225+
226+
// Ensure originalPath is under dir
227+
if dir[len(dir)-1] != filepath.Separator {
228+
dir += string(filepath.Separator)
229+
}
230+
if !strings.HasPrefix(originalPath, dir) {
231+
return 0, errors.Wrapf(errInvalidArchive, "invalid whiteout name: %v", base)
232+
}
233+
223234
if err := os.RemoveAll(originalPath); err != nil {
224235
return 0, err
225236
}

‎archive/tar_test.go‎

Lines changed: 166 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,12 +217,47 @@ func TestBreakouts(t *testing.T) {
217217
return nil
218218
}
219219
}
220+
fileValue := func(f1 string, content []byte) func(string) error {
221+
return func(root string) error {
222+
b, err := ioutil.ReadFile(filepath.Join(root, f1))
223+
if err != nil {
224+
return err
225+
}
226+
if bytes.Compare(b, content) != 0 {
227+
return errors.Errorf("content differs: expected %v, got %v", content, b)
228+
}
229+
return nil
230+
}
231+
}
232+
fileNotExists := func(f1 string) func(string) error {
233+
return func(root string) error {
234+
_, err := os.Lstat(filepath.Join(root, f1))
235+
if err == nil {
236+
return errors.New("file exists")
237+
} else if !os.IsNotExist(err) {
238+
return err
239+
}
240+
return nil
241+
}
242+
243+
}
244+
all := func(funcs ...func(string) error) func(string) error {
245+
return func(root string) error {
246+
for _, f := range funcs {
247+
if err := f(root); err != nil {
248+
return err
249+
}
250+
}
251+
return nil
252+
}
253+
}
220254

221255
breakouts := []struct {
222256
name string
223257
w WriterToTar
224258
apply fstest.Applier
225259
validator func(string) error
260+
err error
226261
}{
227262
{
228263
name: "SymlinkAbsolute",
@@ -468,10 +503,129 @@ func TestBreakouts(t *testing.T) {
468503
),
469504
validator: notSameFile("/localetc/localpasswd", "/etc/passwd"),
470505
},
506+
{
507+
name: "WhiteoutRootParent",
508+
apply: fstest.Apply(
509+
fstest.CreateDir("/etc/", 0755),
510+
fstest.CreateFile("/etc/passwd", []byte("inside"), 0644),
511+
),
512+
w: TarAll(
513+
tc.File(".wh...", []byte{}, 0644), // Should wipe out whole directory
514+
),
515+
err: errInvalidArchive,
516+
},
517+
{
518+
name: "WhiteoutParent",
519+
apply: fstest.Apply(
520+
fstest.CreateDir("/etc/", 0755),
521+
fstest.CreateFile("/etc/passwd", []byte("inside"), 0644),
522+
),
523+
w: TarAll(
524+
tc.File("etc/.wh...", []byte{}, 0644),
525+
),
526+
err: errInvalidArchive,
527+
},
528+
{
529+
name: "WhiteoutRoot",
530+
apply: fstest.Apply(
531+
fstest.CreateDir("/etc/", 0755),
532+
fstest.CreateFile("/etc/passwd", []byte("inside"), 0644),
533+
),
534+
w: TarAll(
535+
tc.File(".wh..", []byte{}, 0644),
536+
),
537+
err: errInvalidArchive,
538+
},
539+
{
540+
name: "WhiteoutCurrentDirectory",
541+
apply: fstest.Apply(
542+
fstest.CreateDir("/etc/", 0755),
543+
fstest.CreateFile("/etc/passwd", []byte("inside"), 0644),
544+
),
545+
w: TarAll(
546+
tc.File("etc/.wh..", []byte{}, 0644), // Should wipe out whole directory
547+
),
548+
err: errInvalidArchive,
549+
},
550+
{
551+
name: "WhiteoutSymlink",
552+
apply: fstest.Apply(
553+
fstest.CreateDir("/etc/", 0755),
554+
fstest.CreateFile("/etc/passwd", []byte("all users"), 0644),
555+
fstest.Symlink("/etc", "localetc"),
556+
),
557+
w: TarAll(
558+
tc.File(".wh.localetc", []byte{}, 0644), // Should wipe out whole directory
559+
),
560+
validator: all(
561+
fileValue("etc/passwd", []byte("all users")),
562+
fileNotExists("localetc"),
563+
),
564+
},
565+
{
566+
// TODO: This test should change once archive apply is disallowing
567+
// symlinks as parents in the name
568+
name: "WhiteoutSymlinkPath",
569+
apply: fstest.Apply(
570+
fstest.CreateDir("/etc/", 0755),
571+
fstest.CreateFile("/etc/passwd", []byte("all users"), 0644),
572+
fstest.CreateFile("/etc/whitedout", []byte("ahhhh whiteout"), 0644),
573+
fstest.Symlink("/etc", "localetc"),
574+
),
575+
w: TarAll(
576+
tc.File("localetc/.wh.whitedout", []byte{}, 0644),
577+
),
578+
validator: all(
579+
fileValue("etc/passwd", []byte("all users")),
580+
fileNotExists("etc/whitedout"),
581+
),
582+
},
583+
{
584+
name: "WhiteoutDirectoryName",
585+
apply: fstest.Apply(
586+
fstest.CreateDir("/etc/", 0755),
587+
fstest.CreateFile("/etc/passwd", []byte("all users"), 0644),
588+
fstest.CreateFile("/etc/whitedout", []byte("ahhhh whiteout"), 0644),
589+
fstest.Symlink("/etc", "localetc"),
590+
),
591+
w: TarAll(
592+
tc.File(".wh.etc/somefile", []byte("non-empty"), 0644),
593+
),
594+
validator: all(
595+
fileValue("etc/passwd", []byte("all users")),
596+
fileValue(".wh.etc/somefile", []byte("non-empty")),
597+
),
598+
},
599+
{
600+
name: "WhiteoutDeadSymlinkParent",
601+
apply: fstest.Apply(
602+
fstest.CreateDir("/etc/", 0755),
603+
fstest.CreateFile("/etc/passwd", []byte("all users"), 0644),
604+
fstest.Symlink("/dne", "localetc"),
605+
),
606+
w: TarAll(
607+
tc.File("localetc/.wh.etc", []byte{}, 0644),
608+
),
609+
// no-op, remove does not
610+
validator: fileValue("etc/passwd", []byte("all users")),
611+
},
612+
{
613+
name: "WhiteoutRelativePath",
614+
apply: fstest.Apply(
615+
fstest.CreateDir("/etc/", 0755),
616+
fstest.CreateFile("/etc/passwd", []byte("all users"), 0644),
617+
fstest.Symlink("/dne", "localetc"),
618+
),
619+
w: TarAll(
620+
tc.File("dne/../.wh.etc", []byte{}, 0644),
621+
),
622+
// resolution ends up just removing etc
623+
validator: fileNotExists("etc/passwd"),
624+
},
471625
}
472626

473627
for _, bo := range breakouts {
474-
t.Run(bo.name, makeWriterToTarTest(bo.w, bo.apply, bo.validator))
628+
t.Run(bo.name, makeWriterToTarTest(bo.w, bo.apply, bo.validator, bo.err))
475629
}
476630
}
477631

@@ -501,6 +655,7 @@ func TestApplyTar(t *testing.T) {
501655
w WriterToTar
502656
apply fstest.Applier
503657
validator func(string) error
658+
err error
504659
}{
505660
{
506661
name: "DirectoryCreation",
@@ -525,7 +680,7 @@ func TestApplyTar(t *testing.T) {
525680
}
526681

527682
for _, at := range tests {
528-
t.Run(at.name, makeWriterToTarTest(at.w, at.apply, at.validator))
683+
t.Run(at.name, makeWriterToTarTest(at.w, at.apply, at.validator, at.err))
529684
}
530685
}
531686

@@ -636,7 +791,7 @@ func testDiffApply(appliers ...fstest.Applier) error {
636791
return fstest.CheckDirectoryEqual(td, dest)
637792
}
638793

639-
func makeWriterToTarTest(wt WriterToTar, a fstest.Applier, validate func(string) error) func(*testing.T) {
794+
func makeWriterToTarTest(wt WriterToTar, a fstest.Applier, validate func(string) error, applyErr error) func(*testing.T) {
640795
return func(t *testing.T) {
641796
td, err := ioutil.TempDir("", "test-writer-to-tar-")
642797
if err != nil {
@@ -653,7 +808,14 @@ func makeWriterToTarTest(wt WriterToTar, a fstest.Applier, validate func(string)
653808
tr := TarFromWriterTo(wt)
654809

655810
if _, err := Apply(context.Background(), td, tr); err != nil {
656-
t.Fatalf("Failed to apply tar: %v", err)
811+
if applyErr == nil {
812+
t.Fatalf("Failed to apply tar: %v", err)
813+
} else if errors.Cause(err) != applyErr {
814+
t.Fatalf("Unexpected apply error: %v, expected %v", err, applyErr)
815+
}
816+
return
817+
} else if applyErr != nil {
818+
t.Fatalf("Expected apply error, got none: %v", applyErr)
657819
}
658820

659821
if validate != nil {

0 commit comments

Comments
 (0)