Skip to content

Commit

Permalink
Enhance security on FileStore (#33)
Browse files Browse the repository at this point in the history
Resolve the following security flaws by default options
- File overwritting
- Directory traversal attack
  • Loading branch information
shizhMSFT committed Jan 10, 2019
1 parent a12f2e7 commit 4cf1be7
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 6 deletions.
8 changes: 7 additions & 1 deletion cmd/oras/pull.go
Expand Up @@ -15,6 +15,8 @@ type pullOptions struct {
targetRef string
allowedMediaTypes []string
allowAllMediaTypes bool
overwrite bool
pathTraversal bool
output string
verbose bool

Expand All @@ -39,7 +41,7 @@ Example - Pull only files with the custom "application/vnd.me.hi" media type:
Example - Pull all files, any media type:
oras pull localhost:5000/hello:latest -a
`,
Args: cobra.ExactArgs(1),
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
opts.targetRef = args[0]
return runPull(opts)
Expand All @@ -48,6 +50,8 @@ Example - Pull all files, any media type:

cmd.Flags().StringArrayVarP(&opts.allowedMediaTypes, "media-type", "t", nil, "allowed media types to be pulled")
cmd.Flags().BoolVarP(&opts.allowAllMediaTypes, "allow-all", "a", false, "allow all media types to be pulled")
cmd.Flags().BoolVarP(&opts.overwrite, "force", "f", false, "allow overwrite on existing files")
cmd.Flags().BoolVarP(&opts.pathTraversal, "allow-path-traversal", "T", false, "allow storing files out of the output directory")
cmd.Flags().StringVarP(&opts.output, "output", "o", "", "output directory")
cmd.Flags().BoolVarP(&opts.verbose, "verbose", "v", false, "verbose output")

Expand All @@ -69,6 +73,8 @@ func runPull(opts pullOptions) error {

resolver := newResolver(opts.username, opts.password)
store := content.NewFileStore(opts.output)
store.AllowOverwrite = opts.overwrite
store.AllowPathTraversalOnWrite = opts.pathTraversal
files, err := oras.Pull(context.Background(), resolver, opts.targetRef, store, opts.allowedMediaTypes...)
if err != nil {
return err
Expand Down
9 changes: 5 additions & 4 deletions pkg/content/content_test.go
Expand Up @@ -9,7 +9,7 @@ import (
"testing"

"github.com/containerd/containerd/content"
"github.com/opencontainers/go-digest"
digest "github.com/opencontainers/go-digest"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/stretchr/testify/suite"
)
Expand All @@ -33,7 +33,7 @@ var (
ocispec.AnnotationTitle: testRef,
},
}
testBadContent = []byte("doesnotexist")
testBadContent = []byte("doesnotexist")
testBadDescriptor = ocispec.Descriptor{
MediaType: ocispec.MediaTypeImageConfig,
Digest: digest.FromBytes(testBadContent),
Expand All @@ -50,6 +50,7 @@ func (suite *ContentTestSuite) SetupSuite() {
err := ioutil.WriteFile(testFileName, testContent, 0644)
suite.Nil(err, "no error creating test file on disk")
testFileStore := NewFileStore(testDirRoot)
testFileStore.AllowOverwrite = true
_, err = testFileStore.Add(testRef, "", testFileName)
suite.Nil(err, "no error adding item to file store")
suite.TestFileStore = testFileStore
Expand Down Expand Up @@ -79,7 +80,7 @@ func (suite *ContentTestSuite) Test_0_Ingesters() {
suite.Nil(err, fmt.Sprintf("no error getting writer w good ref for %s store", key))
_, err = writer.Write(testContent)
suite.Nil(err, fmt.Sprintf("no error using writer.Write w good ref for %s store", key))
err = writer.Commit(ctx, testDescriptor.Size, testDescriptor.Digest)
err = writer.Commit(ctx, testDescriptor.Size, testDescriptor.Digest)
suite.Nil(err, fmt.Sprintf("no error using writer.Commit w good ref for %s store", key))

digest := writer.Digest()
Expand All @@ -106,7 +107,7 @@ func (suite *ContentTestSuite) Test_0_Ingesters() {
err = writer.Truncate(0)
suite.Nil(err, fmt.Sprintf("no error using writer.Truncate w valid size, good ref for %s store", key))

writer.Commit(ctx, testDescriptor.Size, testDescriptor.Digest)
writer.Commit(ctx, testDescriptor.Size, testDescriptor.Digest)

// bad size
err = writer.Commit(ctx, 1, testDescriptor.Digest)
Expand Down
6 changes: 6 additions & 0 deletions pkg/content/errors.go
Expand Up @@ -8,3 +8,9 @@ var (
ErrNoName = errors.New("no_name")
ErrUnsupportedSize = errors.New("unsupported_size")
)

// FileStore errors
var (
ErrPathTraversalDisallowed = errors.New("path_traversal_disallowed")
ErrOverwriteDisallowed = errors.New("overwrite_disallowed")
)
27 changes: 26 additions & 1 deletion pkg/content/file.go
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"os"
"path/filepath"
"strings"
"sync"
"time"

Expand All @@ -23,6 +24,9 @@ var (

// FileStore provides content from the file system
type FileStore struct {
AllowOverwrite bool
AllowPathTraversalOnWrite bool

root string
descriptor *sync.Map // map[digest.Digest]ocispec.Descriptor
pathMap *sync.Map
Expand Down Expand Up @@ -112,10 +116,31 @@ func (s *FileStore) Writer(ctx context.Context, opts ...content.WriterOpt) (cont
return nil, ErrNoName
}
path := s.ResolvePath(name)
if !s.AllowPathTraversalOnWrite {
base, err := filepath.Abs(s.root)
if err != nil {
return nil, err
}
target, err := filepath.Abs(path)
if err != nil {
return nil, err
}
rel, err := filepath.Rel(base, target)
if err != nil || strings.HasPrefix(filepath.ToSlash(rel), "../") {
return nil, ErrPathTraversalDisallowed
}
}
if !s.AllowOverwrite {
if _, err := os.Stat(path); err == nil {
return nil, ErrOverwriteDisallowed
} else if !os.IsNotExist(err) {
return nil, err
}
}

if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
return nil, err
}

file, err := os.Create(path)
if err != nil {
return nil, err
Expand Down

0 comments on commit 4cf1be7

Please sign in to comment.