From 68c6cbacbb3c6172db3c314655bbd8bae8f60730 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 5 Oct 2022 16:29:44 +0200 Subject: [PATCH 1/3] feat: error when TAR has files outside of root --- tarwriter.go | 40 ++++++++++++++++++++++++++++- tarwriter_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/tarwriter.go b/tarwriter.go index 4f4ee4e..649b2fa 100644 --- a/tarwriter.go +++ b/tarwriter.go @@ -2,14 +2,22 @@ package files import ( "archive/tar" + "errors" "fmt" "io" "path" + "strings" "time" ) +var ( + ErrUnixFSPathOutsideRoot = errors.New("relative UnixFS paths outside the root are now allowed, use CAR instead") +) + type TarWriter struct { - TarW *tar.Writer + TarW *tar.Writer + baseDirSet bool + baseDir string } // NewTarWriter wraps given io.Writer into a new tar writer @@ -50,8 +58,38 @@ func (w *TarWriter) writeFile(f File, fpath string) error { return nil } +func validateTarFilePath(baseDir, fpath string) bool { + // Ensure the filepath has no ".", "..", etc within the known filepath. + fpath = path.Clean(fpath) + + // If we have a non-empty baseDir, check if the filepath starts with baseDir. + // If not, we can exclude it immediately. For 'ipfs get' and for the gateway, + // the baseDir would be '{cid}.tar'. + if baseDir != "" && !strings.HasPrefix(path.Clean(fpath), baseDir) { + return false + } + + // If there is no defined baseDir, i.e., if there's multiple files in the + // root, check if paths contain elements that would otherwise make them fall + // outside the root. + if strings.Contains(fpath, "..") { + return false + } + + return true +} + // WriteNode adds a node to the archive. func (w *TarWriter) WriteFile(nd Node, fpath string) error { + if !w.baseDirSet { + w.baseDirSet = true // Use a variable for this as baseDir may be an empty string. + w.baseDir = fpath + } + + if !validateTarFilePath(w.baseDir, fpath) { + return ErrUnixFSPathOutsideRoot + } + switch nd := nd.(type) { case *Symlink: return writeSymlinkHeader(w.TarW, nd.Target, fpath) diff --git a/tarwriter_test.go b/tarwriter_test.go index f66d035..1485d56 100644 --- a/tarwriter_test.go +++ b/tarwriter_test.go @@ -2,6 +2,7 @@ package files import ( "archive/tar" + "errors" "io" "testing" "time" @@ -83,3 +84,66 @@ func TestTarWriter(t *testing.T) { t.Fatal(err) } } + +func TestTarWriterRelativePathInsideRoot(t *testing.T) { + tf := NewMapDirectory(map[string]Node{ + "file.txt": NewBytesFile([]byte(text)), + "boop": NewMapDirectory(map[string]Node{ + "../a.txt": NewBytesFile([]byte("bleep")), + "b.txt": NewBytesFile([]byte("bloop")), + }), + "beep.txt": NewBytesFile([]byte("beep")), + }) + + tw, err := NewTarWriter(io.Discard) + if err != nil { + t.Fatal(err) + } + + defer tw.Close() + if err := tw.WriteFile(tf, ""); err != nil { + t.Error(err) + } +} + +func TestTarWriterFailsFileOutsideRoot(t *testing.T) { + tf := NewMapDirectory(map[string]Node{ + "file.txt": NewBytesFile([]byte(text)), + "boop": NewMapDirectory(map[string]Node{ + "../../a.txt": NewBytesFile([]byte("bleep")), + "b.txt": NewBytesFile([]byte("bloop")), + }), + "beep.txt": NewBytesFile([]byte("beep")), + }) + + tw, err := NewTarWriter(io.Discard) + if err != nil { + t.Fatal(err) + } + + defer tw.Close() + if err := tw.WriteFile(tf, ""); !errors.Is(err, ErrUnixFSPathOutsideRoot) { + t.Error(err) + } +} + +func TestTarWriterFailsFileOutsideRootWithBaseDir(t *testing.T) { + tf := NewMapDirectory(map[string]Node{ + "../file.txt": NewBytesFile([]byte(text)), + "boop": NewMapDirectory(map[string]Node{ + "a.txt": NewBytesFile([]byte("bleep")), + "b.txt": NewBytesFile([]byte("bloop")), + }), + "beep.txt": NewBytesFile([]byte("beep")), + }) + + tw, err := NewTarWriter(io.Discard) + if err != nil { + t.Fatal(err) + } + + defer tw.Close() + if err := tw.WriteFile(tf, "test.tar"); !errors.Is(err, ErrUnixFSPathOutsideRoot) { + t.Error(err) + } +} From 2acf76c5d599957feb4c6806e5b00aded61ac848 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 5 Oct 2022 16:34:22 +0200 Subject: [PATCH 2/3] docs: improve text --- tarwriter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tarwriter.go b/tarwriter.go index 649b2fa..f90b2b9 100644 --- a/tarwriter.go +++ b/tarwriter.go @@ -59,7 +59,7 @@ func (w *TarWriter) writeFile(f File, fpath string) error { } func validateTarFilePath(baseDir, fpath string) bool { - // Ensure the filepath has no ".", "..", etc within the known filepath. + // Ensure the filepath has no ".", "..", etc within the known root directory. fpath = path.Clean(fpath) // If we have a non-empty baseDir, check if the filepath starts with baseDir. From 619bbe4ac790771b691f7af47fe09cdbfe62b121 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Mon, 7 Nov 2022 14:48:41 +0100 Subject: [PATCH 3/3] address comments --- tarwriter.go | 7 +++---- tarwriter_test.go | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/tarwriter.go b/tarwriter.go index f90b2b9..cecbcae 100644 --- a/tarwriter.go +++ b/tarwriter.go @@ -69,10 +69,9 @@ func validateTarFilePath(baseDir, fpath string) bool { return false } - // If there is no defined baseDir, i.e., if there's multiple files in the - // root, check if paths contain elements that would otherwise make them fall - // outside the root. - if strings.Contains(fpath, "..") { + // Otherwise, check if the path starts with '..' which would make it fall + // outside the root path. This works since the path has already been cleaned. + if strings.HasPrefix(fpath, "..") { return false } diff --git a/tarwriter_test.go b/tarwriter_test.go index 1485d56..0e1488e 100644 --- a/tarwriter_test.go +++ b/tarwriter_test.go @@ -123,7 +123,7 @@ func TestTarWriterFailsFileOutsideRoot(t *testing.T) { defer tw.Close() if err := tw.WriteFile(tf, ""); !errors.Is(err, ErrUnixFSPathOutsideRoot) { - t.Error(err) + t.Errorf("unexpected error, wanted: %v; got: %v", ErrUnixFSPathOutsideRoot, err) } } @@ -144,6 +144,6 @@ func TestTarWriterFailsFileOutsideRootWithBaseDir(t *testing.T) { defer tw.Close() if err := tw.WriteFile(tf, "test.tar"); !errors.Is(err, ErrUnixFSPathOutsideRoot) { - t.Error(err) + t.Errorf("unexpected error, wanted: %v; got: %v", ErrUnixFSPathOutsideRoot, err) } }