From e417c19a748cf61427c293fa1ff6b034ce6b69f3 Mon Sep 17 00:00:00 2001 From: Matt Joiner Date: Thu, 2 Sep 2021 10:21:46 +1000 Subject: [PATCH] Add "no name" handling and storage.NewFileOpts This came out of testing against Transmission in https://github.com/anacrolix/torrent/discussions/556#discussioncomment-1263670. --- metainfo/info.go | 15 +++++-- storage/file-deprecated.go | 34 +++++++++++++++ storage/file-paths.go | 38 +++++++++++++++++ storage/file.go | 84 +++++++++++++++++--------------------- storage/mmap.go | 1 + storage/safe-path_test.go | 60 +++++++++++++++++++++------ 6 files changed, 169 insertions(+), 63 deletions(-) create mode 100644 storage/file-deprecated.go create mode 100644 storage/file-paths.go diff --git a/metainfo/info.go b/metainfo/info.go index 6b251884..c907f492 100644 --- a/metainfo/info.go +++ b/metainfo/info.go @@ -23,14 +23,23 @@ type Info struct { Files []FileInfo `bencode:"files,omitempty"` // BEP3, mutually exclusive with Length } -// This is a helper that sets Files and Pieces from a root path and its -// children. +// The Info.Name field is "advisory". For multi-file torrents it's usually a suggested directory +// name. There are situations where we don't want a directory (like using the contents of a torrent +// as the immediate contents of a directory), or the name is invalid. Transmission will inject the +// name of the torrent file if it doesn't like the name, resulting in a different infohash +// (https://github.com/transmission/transmission/issues/1775). To work around these situations, we +// will use a sentinel name for compatibility with Transmission and to signal to our own client that +// we intended to have no directory name. By exposing it in the API we can check for references to +// this behaviour within this implementation. +const NoName = "-" + +// This is a helper that sets Files and Pieces from a root path and its children. func (info *Info) BuildFromFilePath(root string) (err error) { info.Name = func() string { b := filepath.Base(root) switch b { case ".", "..", string(filepath.Separator): - return "" + return NoName default: return b } diff --git a/storage/file-deprecated.go b/storage/file-deprecated.go new file mode 100644 index 00000000..4560b9de --- /dev/null +++ b/storage/file-deprecated.go @@ -0,0 +1,34 @@ +package storage + +import ( + "github.com/anacrolix/torrent/metainfo" +) + +func NewFileWithCompletion(baseDir string, completion PieceCompletion) ClientImplCloser { + return NewFileWithCustomPathMakerAndCompletion(baseDir, nil, completion) +} + +// File storage with data partitioned by infohash. +func NewFileByInfoHash(baseDir string) ClientImplCloser { + return NewFileWithCustomPathMaker(baseDir, infoHashPathMaker) +} + +// Deprecated: Allows passing a function to determine the path for storing torrent data. The +// function is responsible for sanitizing the info if it uses some part of it (for example +// sanitizing info.Name). +func NewFileWithCustomPathMaker(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string) ClientImplCloser { + return NewFileWithCustomPathMakerAndCompletion(baseDir, pathMaker, pieceCompletionForDir(baseDir)) +} + +// Deprecated: Allows passing custom PieceCompletion +func NewFileWithCustomPathMakerAndCompletion( + baseDir string, + pathMaker TorrentDirFilePathMaker, + completion PieceCompletion, +) ClientImplCloser { + return NewFileOpts(NewFileClientOpts{ + ClientBaseDir: baseDir, + TorrentDirMaker: pathMaker, + PieceCompletion: completion, + }) +} diff --git a/storage/file-paths.go b/storage/file-paths.go new file mode 100644 index 00000000..8d338f86 --- /dev/null +++ b/storage/file-paths.go @@ -0,0 +1,38 @@ +package storage + +import ( + "os" + "path/filepath" + "strings" + + "github.com/anacrolix/torrent/metainfo" +) + +// Determines the filepath to be used for each file in a torrent. +type FilePathMaker func(opts FilePathMakerOpts) string + +// Determines the directory for a given torrent within a storage client. +type TorrentDirFilePathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string + +// Info passed to a FilePathMaker. +type FilePathMakerOpts struct { + Info *metainfo.Info + File *metainfo.FileInfo +} + +// defaultPathMaker just returns the storage client's base directory. +func defaultPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string { + return baseDir +} + +func infoHashPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string { + return filepath.Join(baseDir, infoHash.HexString()) +} + +func isSubFilepath(base, sub string) bool { + rel, err := filepath.Rel(base, sub) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) +} diff --git a/storage/file.go b/storage/file.go index 22c1d849..bbfe6d66 100644 --- a/storage/file.go +++ b/storage/file.go @@ -13,73 +13,63 @@ import ( "github.com/anacrolix/torrent/metainfo" ) -// File-based storage for torrents, that isn't yet bound to a particular -// torrent. +// File-based storage for torrents, that isn't yet bound to a particular torrent. type fileClientImpl struct { - baseDir string - pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string - pc PieceCompletion + opts NewFileClientOpts } -// The Default path maker just returns the current path -func defaultPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string { - return baseDir -} - -func infoHashPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string { - return filepath.Join(baseDir, infoHash.HexString()) -} - -// All Torrent data stored in this baseDir +// All Torrent data stored in this baseDir. The info names of each torrent are used as directories. func NewFile(baseDir string) ClientImplCloser { return NewFileWithCompletion(baseDir, pieceCompletionForDir(baseDir)) } -func NewFileWithCompletion(baseDir string, completion PieceCompletion) *fileClientImpl { - return NewFileWithCustomPathMakerAndCompletion(baseDir, nil, completion) +type NewFileClientOpts struct { + // The base directory for all downloads. + ClientBaseDir string + FilePathMaker FilePathMaker + TorrentDirMaker TorrentDirFilePathMaker + PieceCompletion PieceCompletion } -// File storage with data partitioned by infohash. -func NewFileByInfoHash(baseDir string) ClientImplCloser { - return NewFileWithCustomPathMaker(baseDir, infoHashPathMaker) -} - -// Allows passing a function to determine the path for storing torrent data. The function is -// responsible for sanitizing the info if it uses some part of it (for example sanitizing -// info.Name). -func NewFileWithCustomPathMaker(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string) ClientImplCloser { - return NewFileWithCustomPathMakerAndCompletion(baseDir, pathMaker, pieceCompletionForDir(baseDir)) -} - -// Allows passing custom PieceCompletion -func NewFileWithCustomPathMakerAndCompletion(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string, completion PieceCompletion) *fileClientImpl { - if pathMaker == nil { - pathMaker = defaultPathMaker +// NewFileOpts creates a new ClientImplCloser that stores files using the OS native filesystem. +func NewFileOpts(opts NewFileClientOpts) ClientImplCloser { + if opts.TorrentDirMaker == nil { + opts.TorrentDirMaker = defaultPathMaker } - return &fileClientImpl{ - baseDir: baseDir, - pathMaker: pathMaker, - pc: completion, + if opts.FilePathMaker == nil { + opts.FilePathMaker = func(opts FilePathMakerOpts) string { + var parts []string + if opts.Info.Name != metainfo.NoName { + parts = append(parts, opts.Info.Name) + } + return filepath.Join(append(parts, opts.File.Path...)...) + } } + if opts.PieceCompletion == nil { + opts.PieceCompletion = pieceCompletionForDir(opts.ClientBaseDir) + } + return fileClientImpl{opts} } -func (me *fileClientImpl) Close() error { - return me.pc.Close() +func (me fileClientImpl) Close() error { + return me.opts.PieceCompletion.Close() } -func (fs *fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Hash) (_ TorrentImpl, err error) { - dir := fs.pathMaker(fs.baseDir, info, infoHash) +func (fs fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Hash) (_ TorrentImpl, err error) { + dir := fs.opts.TorrentDirMaker(fs.opts.ClientBaseDir, info, infoHash) upvertedFiles := info.UpvertedFiles() files := make([]file, 0, len(upvertedFiles)) for i, fileInfo := range upvertedFiles { - var s string - s, err = ToSafeFilePath(append([]string{info.Name}, fileInfo.Path...)...) - if err != nil { - err = fmt.Errorf("file %v has unsafe path %q: %w", i, fileInfo.Path, err) + filePath := filepath.Join(dir, fs.opts.FilePathMaker(FilePathMakerOpts{ + Info: info, + File: &fileInfo, + })) + if !isSubFilepath(dir, filePath) { + err = fmt.Errorf("file %v: path %q is not sub path of %q", i, filePath, dir) return } f := file{ - path: filepath.Join(dir, s), + path: filePath, length: fileInfo.Length, } if f.length == 0 { @@ -95,7 +85,7 @@ func (fs *fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Has files, segments.NewIndex(common.LengthIterFromUpvertedFiles(upvertedFiles)), infoHash, - fs.pc, + fs.opts.PieceCompletion, } return TorrentImpl{ Piece: t.Piece, diff --git a/storage/mmap.go b/storage/mmap.go index 9c0a27ed..9c9e8471 100644 --- a/storage/mmap.go +++ b/storage/mmap.go @@ -22,6 +22,7 @@ type mmapClientImpl struct { pc PieceCompletion } +// TODO: Support all the same native filepath configuration that NewFileOpts provides. func NewMMap(baseDir string) ClientImplCloser { return NewMMapWithCompletion(baseDir, pieceCompletionForDir(baseDir)) } diff --git a/storage/safe-path_test.go b/storage/safe-path_test.go index bf7e7ba0..e12d3333 100644 --- a/storage/safe-path_test.go +++ b/storage/safe-path_test.go @@ -1,28 +1,38 @@ package storage import ( + "fmt" "log" "path/filepath" "testing" + + "github.com/anacrolix/torrent/metainfo" + qt "github.com/frankban/quicktest" ) func init() { log.SetFlags(log.Flags() | log.Lshortfile) } -func TestSafePath(t *testing.T) { - for _, _case := range []struct { - input []string - expected string - expectErr bool - }{ - {input: []string{"a", filepath.FromSlash(`b/../../..`)}, expectErr: true}, - {input: []string{"a", filepath.FromSlash(`b/../.././..`)}, expectErr: true}, - {input: []string{ - filepath.FromSlash(`NewSuperHeroMovie-2019-English-720p.avi /../../../../../Roaming/Microsoft/Windows/Start Menu/Programs/Startup/test3.exe`)}, - expectErr: true, - }, - } { +// I think these are mainly tests for bad metainfos that try to escape the client base directory. +var safeFilePathTests = []struct { + input []string + expectErr bool +}{ + // We might want a test for invalid chars inside components, or file maker opt funcs returning + // absolute paths (and thus presumably clobbering earlier "makers"). + {input: []string{"a", filepath.FromSlash(`b/..`)}, expectErr: false}, + {input: []string{"a", filepath.FromSlash(`b/../../..`)}, expectErr: true}, + {input: []string{"a", filepath.FromSlash(`b/../.././..`)}, expectErr: true}, + {input: []string{ + filepath.FromSlash(`NewSuperHeroMovie-2019-English-720p.avi /../../../../../Roaming/Microsoft/Windows/Start Menu/Programs/Startup/test3.exe`)}, + expectErr: true, + }, +} + +// Tests the ToSafeFilePath func. +func TestToSafeFilePath(t *testing.T) { + for _, _case := range safeFilePathTests { actual, err := ToSafeFilePath(_case.input...) if _case.expectErr { if err != nil { @@ -32,3 +42,27 @@ func TestSafePath(t *testing.T) { } } } + +// Check that safe file path handling still exists for the newer file-opt-maker variants. +func TestFileOptsSafeFilePathHandling(t *testing.T) { + c := qt.New(t) + for i, _case := range safeFilePathTests { + c.Run(fmt.Sprintf("Case%v", i), func(c *qt.C) { + info := metainfo.Info{ + Files: []metainfo.FileInfo{ + {Path: _case.input}, + }, + } + client := NewFileOpts(NewFileClientOpts{ + ClientBaseDir: "somedir", + }) + defer func() { c.Check(client.Close(), qt.IsNil) }() + torImpl, err := client.OpenTorrent(&info, metainfo.Hash{}) + if _case.expectErr { + c.Check(err, qt.Not(qt.IsNil)) + } else { + c.Check(torImpl.Close(), qt.IsNil) + } + }) + } +}