Skip to content
Permalink
Browse files Browse the repository at this point in the history
repository/content: fix potential (authenticated) path traversal (#195)
  • Loading branch information
hoffie committed Jan 20, 2015
1 parent 0ee09bc commit 776bad4
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 8 deletions.
49 changes: 41 additions & 8 deletions repository/content/file_storage.go
@@ -1,9 +1,12 @@
package content

import (
"errors"
"io"
"os"
"path"
"path/filepath"
"strings"

"github.com/hoffie/larasync/helpers/atomic"
)
Expand All @@ -14,6 +17,10 @@ const (
defaultDirPerms = 0700
)

// ErrInvalidPath is returned if storage at a path not rooted at the FileStorage's
// root path is attempted.
var ErrInvalidPath = errors.New("invalid path")

// FileStorage is the basic implementation of the Storage
// implementation which stores the data into the file system.
type FileStorage struct {
Expand All @@ -39,23 +46,40 @@ func (f *FileStorage) CreateDir() error {
}

// storagePathFor returns the storage path for the data entry.
func (f *FileStorage) storagePathFor(contentID string) string {
return path.Join(f.path, contentID)
func (f *FileStorage) storagePathFor(contentID string) (string, error) {
p := path.Join(f.path, contentID)
p = filepath.Clean(p)
root := f.path
/*if len(root) > 1 && root[len(root)-1] != filepath.Separator {
root += filepath.Separator
}*/
if !strings.HasPrefix(p, root) {
return "", ErrInvalidPath
}
return p, nil
}

// Get returns the file handle for the given contentID.
// If there is no data stored for the Id it should return a
// os.ErrNotExists error.
func (f *FileStorage) Get(contentID string) (io.ReadCloser, error) {
if f.Exists(contentID) {
return os.Open(f.storagePathFor(contentID))
if !f.Exists(contentID) {
return nil, os.ErrNotExist
}
// FIXME TOCTU race
p, err := f.storagePathFor(contentID)
if err != nil {
return nil, err
}
return nil, os.ErrNotExist
return os.Open(p)
}

// Set sets the data of the given contentID in the blob storage.
func (f *FileStorage) Set(contentID string, reader io.Reader) error {
blobStoragePath := f.storagePathFor(contentID)
blobStoragePath, err := f.storagePathFor(contentID)
if err != nil {
return err
}

writer, err := atomic.NewStandardWriter(blobStoragePath, defaultFilePerms)
if err != nil {
Expand All @@ -79,7 +103,12 @@ func (f *FileStorage) Set(contentID string, reader io.Reader) error {

// Exists checks if the given entry is stored in the database.
func (f *FileStorage) Exists(contentID string) bool {
_, err := os.Stat(f.storagePathFor(contentID))
p, err := f.storagePathFor(contentID)
if err != nil {
// FIXME maybe return error instead?
return false
}
_, err = os.Stat(p)
if err != nil {
return !os.IsNotExist(err)
}
Expand All @@ -88,5 +117,9 @@ func (f *FileStorage) Exists(contentID string) bool {

// Delete removes the data with the given contentID from the store.
func (f *FileStorage) Delete(contentID string) error {
return os.Remove(f.storagePathFor(contentID))
p, err := f.storagePathFor(contentID)
if err != nil {
return err
}
return os.Remove(p)
}
17 changes: 17 additions & 0 deletions repository/content/file_storage_test.go
Expand Up @@ -20,6 +20,8 @@ type FileStorageTests struct {

var _ = Suite(&FileStorageTests{})

var dangerousNames = []string{"..", "../xyz"}

func (t *FileStorageTests) SetUpTest(c *C) {
t.dir = c.MkDir()
t.storage = NewFileStorage(t.dir)
Expand All @@ -43,6 +45,14 @@ func (t *FileStorageTests) setData() error {
return t.storage.Set(t.blobID(), t.testReader())
}

func (t *FileStorageTests) TestSetDangerousName(c *C) {
r := t.testReader()
for _, id := range dangerousNames {
err := t.storage.Set(id, r)
c.Assert(err, NotNil)
}
}

func (t *FileStorageTests) TestSet(c *C) {
err := t.setData()
c.Assert(err, IsNil)
Expand Down Expand Up @@ -72,6 +82,13 @@ func (t *FileStorageTests) TestGet(c *C) {
c.Assert(err, IsNil)
}

func (t *FileStorageTests) TestGetDangerousName(c *C) {
for _, id := range dangerousNames {
_, err := t.storage.Get(id)
c.Assert(err, NotNil)
}
}

func (t *FileStorageTests) TestGetData(c *C) {
t.setData()
file, _ := t.storage.Get(t.blobID())
Expand Down

0 comments on commit 776bad4

Please sign in to comment.