fix: perform extra validation on paths in tar archives (#5165)

* fix: perform extra validation on paths in tar archives

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

* fix: Cover a few Windows cases and also remove a duplicate tar reader

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

* fix: removed debug output

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

* fix: Expand again preserves the files verbatim

Also added tests for Expand

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

* fix: add license block and remove println

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
pull/5175/head
Matt Butcher 6 years ago committed by GitHub
parent 893c3b61f6
commit 5603fe8d3e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -17,58 +17,60 @@ limitations under the License.
package chartutil
import (
"archive/tar"
"compress/gzip"
"errors"
"io"
"io/ioutil"
"os"
"path/filepath"
securejoin "github.com/cyphar/filepath-securejoin"
)
// Expand uncompresses and extracts a chart into the specified directory.
func Expand(dir string, r io.Reader) error {
gr, err := gzip.NewReader(r)
files, err := loadArchiveFiles(r)
if err != nil {
return err
}
defer gr.Close()
tr := tar.NewReader(gr)
for {
header, err := tr.Next()
if err == io.EOF {
break
} else if err != nil {
return err
}
//split header name and create missing directories
d, _ := filepath.Split(header.Name)
fullDir := filepath.Join(dir, d)
_, err = os.Stat(fullDir)
if err != nil && d != "" {
if err := os.MkdirAll(fullDir, 0700); err != nil {
// Get the name of the chart
var chartName string
for _, file := range files {
if file.Name == "Chart.yaml" {
ch, err := UnmarshalChartfile(file.Data)
if err != nil {
return err
}
chartName = ch.GetName()
}
}
if chartName == "" {
return errors.New("chart name not specified")
}
path := filepath.Clean(filepath.Join(dir, header.Name))
info := header.FileInfo()
if info.IsDir() {
if err = os.MkdirAll(path, info.Mode()); err != nil {
return err
}
continue
}
// Find the base directory
chartdir, err := securejoin.SecureJoin(dir, chartName)
if err != nil {
return err
}
file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, info.Mode())
// Copy all files verbatim. We don't parse these files because parsing can remove
// comments.
for _, file := range files {
outpath, err := securejoin.SecureJoin(chartdir, file.Name)
if err != nil {
return err
}
_, err = io.Copy(file, tr)
if err != nil {
file.Close()
// Make sure the necessary subdirs get created.
basedir := filepath.Dir(outpath)
if err := os.MkdirAll(basedir, 0755); err != nil {
return err
}
if err := ioutil.WriteFile(outpath, file.Data, 0644); err != nil {
return err
}
file.Close()
}
return nil
}

@ -0,0 +1,121 @@
/*
Copyright The Helm Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package chartutil
import (
"io/ioutil"
"os"
"path/filepath"
"testing"
)
func TestExpand(t *testing.T) {
dest, err := ioutil.TempDir("", "helm-testing-")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dest)
reader, err := os.Open("testdata/frobnitz-1.2.3.tgz")
if err != nil {
t.Fatal(err)
}
if err := Expand(dest, reader); err != nil {
t.Fatal(err)
}
expectedChartPath := filepath.Join(dest, "frobnitz")
fi, err := os.Stat(expectedChartPath)
if err != nil {
t.Fatal(err)
}
if !fi.IsDir() {
t.Fatalf("expected a chart directory at %s", expectedChartPath)
}
dir, err := os.Open(expectedChartPath)
if err != nil {
t.Fatal(err)
}
fis, err := dir.Readdir(0)
if err != nil {
t.Fatal(err)
}
expectLen := 12
if len(fis) != expectLen {
t.Errorf("Expected %d files, but got %d", expectLen, len(fis))
}
for _, fi := range fis {
expect, err := os.Stat(filepath.Join("testdata", "frobnitz", fi.Name()))
if err != nil {
t.Fatal(err)
}
if fi.Size() != expect.Size() {
t.Errorf("Expected %s to have size %d, got %d", fi.Name(), expect.Size(), fi.Size())
}
}
}
func TestExpandFile(t *testing.T) {
dest, err := ioutil.TempDir("", "helm-testing-")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dest)
if err := ExpandFile(dest, "testdata/frobnitz-1.2.3.tgz"); err != nil {
t.Fatal(err)
}
expectedChartPath := filepath.Join(dest, "frobnitz")
fi, err := os.Stat(expectedChartPath)
if err != nil {
t.Fatal(err)
}
if !fi.IsDir() {
t.Fatalf("expected a chart directory at %s", expectedChartPath)
}
dir, err := os.Open(expectedChartPath)
if err != nil {
t.Fatal(err)
}
fis, err := dir.Readdir(0)
if err != nil {
t.Fatal(err)
}
expectLen := 12
if len(fis) != expectLen {
t.Errorf("Expected %d files, but got %d", expectLen, len(fis))
}
for _, fi := range fis {
expect, err := os.Stat(filepath.Join("testdata", "frobnitz", fi.Name()))
if err != nil {
t.Fatal(err)
}
if fi.Size() != expect.Size() {
t.Errorf("Expected %s to have size %d, got %d", fi.Name(), expect.Size(), fi.Size())
}
}
}

@ -25,7 +25,9 @@ import (
"io"
"io/ioutil"
"os"
"path"
"path/filepath"
"regexp"
"strings"
"github.com/golang/protobuf/ptypes/any"
@ -63,11 +65,13 @@ type BufferedFile struct {
Data []byte
}
// LoadArchive loads from a reader containing a compressed tar archive.
func LoadArchive(in io.Reader) (*chart.Chart, error) {
var drivePathPattern = regexp.MustCompile(`^[a-zA-Z]:/`)
// loadArchiveFiles loads files out of an archive
func loadArchiveFiles(in io.Reader) ([]*BufferedFile, error) {
unzipped, err := gzip.NewReader(in)
if err != nil {
return &chart.Chart{}, err
return nil, err
}
defer unzipped.Close()
@ -80,7 +84,7 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) {
break
}
if err != nil {
return &chart.Chart{}, err
return nil, err
}
if hd.FileInfo().IsDir() {
@ -101,12 +105,33 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) {
// Normalize the path to the / delimiter
n = strings.Replace(n, delimiter, "/", -1)
if path.IsAbs(n) {
return nil, errors.New("chart illegally contains absolute paths")
}
n = path.Clean(n)
if n == "." {
// In this case, the original path was relative when it should have been absolute.
return nil, errors.New("chart illegally contains empty path")
}
if strings.HasPrefix(n, "..") {
return nil, errors.New("chart illegally references parent directory")
}
// In some particularly arcane acts of path creativity, it is possible to intermix
// UNIX and Windows style paths in such a way that you produce a result of the form
// c:/foo even after all the built-in absolute path checks. So we explicitly check
// for this condition.
if drivePathPattern.MatchString(n) {
return nil, errors.New("chart contains illegally named files")
}
if parts[0] == "Chart.yaml" {
return nil, errors.New("chart yaml not in base directory")
}
if _, err := io.Copy(b, tr); err != nil {
return &chart.Chart{}, err
return files, err
}
files = append(files, &BufferedFile{Name: n, Data: b.Bytes()})
@ -116,7 +141,15 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) {
if len(files) == 0 {
return nil, errors.New("no files in chart archive")
}
return files, nil
}
// LoadArchive loads from a reader containing a compressed tar archive.
func LoadArchive(in io.Reader) (*chart.Chart, error) {
files, err := loadArchiveFiles(in)
if err != nil {
return nil, err
}
return LoadFiles(files)
}

@ -17,8 +17,14 @@ limitations under the License.
package chartutil
import (
"archive/tar"
"compress/gzip"
"io/ioutil"
"os"
"path"
"path/filepath"
"testing"
"time"
"k8s.io/helm/pkg/proto/hapi/chart"
)
@ -43,6 +49,97 @@ func TestLoadFile(t *testing.T) {
verifyRequirements(t, c)
}
func TestLoadArchive_InvalidArchive(t *testing.T) {
tmpdir, err := ioutil.TempDir("", "helm-test-")
if err != nil {
t.Fatal(err)
}
defer os.Remove(tmpdir)
writeTar := func(filename, internalPath string, body []byte) {
dest, err := os.Create(filename)
if err != nil {
t.Fatal(err)
}
zipper := gzip.NewWriter(dest)
tw := tar.NewWriter(zipper)
h := &tar.Header{
Name: internalPath,
Mode: 0755,
Size: int64(len(body)),
ModTime: time.Now(),
}
if err := tw.WriteHeader(h); err != nil {
t.Fatal(err)
}
if _, err := tw.Write(body); err != nil {
t.Fatal(err)
}
tw.Close()
zipper.Close()
dest.Close()
}
for _, tt := range []struct {
chartname string
internal string
expectError string
}{
{"illegal-dots.tgz", "../../malformed-helm-test", "chart illegally references parent directory"},
{"illegal-dots2.tgz", "/foo/../../malformed-helm-test", "chart illegally references parent directory"},
{"illegal-dots3.tgz", "/../../malformed-helm-test", "chart illegally references parent directory"},
{"illegal-dots4.tgz", "./../../malformed-helm-test", "chart illegally references parent directory"},
{"illegal-name.tgz", "./.", "chart illegally contains empty path"},
{"illegal-name2.tgz", "/./.", "chart illegally contains empty path"},
{"illegal-name3.tgz", "missing-leading-slash", "chart illegally contains empty path"},
{"illegal-name4.tgz", "/missing-leading-slash", "chart metadata (Chart.yaml) missing"},
{"illegal-abspath.tgz", "//foo", "chart illegally contains absolute paths"},
{"illegal-abspath2.tgz", "///foo", "chart illegally contains absolute paths"},
{"illegal-abspath3.tgz", "\\\\foo", "chart illegally contains absolute paths"},
{"illegal-abspath3.tgz", "\\..\\..\\foo", "chart illegally references parent directory"},
// Under special circumstances, this can get normalized to things that look like absolute Windows paths
{"illegal-abspath4.tgz", "\\.\\c:\\\\foo", "chart contains illegally named files"},
{"illegal-abspath5.tgz", "/./c://foo", "chart contains illegally named files"},
{"illegal-abspath6.tgz", "\\\\?\\Some\\windows\\magic", "chart illegally contains absolute paths"},
} {
illegalChart := filepath.Join(tmpdir, tt.chartname)
writeTar(illegalChart, tt.internal, []byte("hello: world"))
_, err = Load(illegalChart)
if err == nil {
t.Fatal("expected error when unpacking illegal files")
}
if err.Error() != tt.expectError {
t.Errorf("Expected %q, got %q for %s", tt.expectError, err.Error(), tt.chartname)
}
}
// Make sure that absolute path gets interpreted as relative
illegalChart := filepath.Join(tmpdir, "abs-path.tgz")
writeTar(illegalChart, "/Chart.yaml", []byte("hello: world"))
_, err = Load(illegalChart)
if err.Error() != "invalid chart (Chart.yaml): name must not be empty" {
t.Error(err)
}
// And just to validate that the above was not spurious
illegalChart = filepath.Join(tmpdir, "abs-path2.tgz")
writeTar(illegalChart, "files/whatever.yaml", []byte("hello: world"))
_, err = Load(illegalChart)
if err.Error() != "chart metadata (Chart.yaml) missing" {
t.Error(err)
}
// Finally, test that drive letter gets stripped off on Windows
illegalChart = filepath.Join(tmpdir, "abs-winpath.tgz")
writeTar(illegalChart, "c:\\Chart.yaml", []byte("hello: world"))
_, err = Load(illegalChart)
if err.Error() != "invalid chart (Chart.yaml): name must not be empty" {
t.Error(err)
}
}
func TestLoadFiles(t *testing.T) {
goodFiles := []*BufferedFile{
{

Loading…
Cancel
Save