restrict untar locations

npm/tar.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 
 	"github.com/cdnjs/tools/util"
 )
@@ -25,7 +26,7 @@ func removePackageDir(path string) string {
 }
 
 // Untar uncompresses a tar at a destination.
-func Untar(dst string, r io.Reader) error {
+func Untar(ctx context.Context, dst string, r io.Reader) error {
 	gzr, err := gzip.NewReader(r)
 	if err != nil {
 		return err
@@ -55,9 +56,15 @@ func Untar(dst string, r io.Reader) error {
 		// the target location where the dir/file should be created
 		target := filepath.Join(dst, removePackageDir(header.Name))
 
-		// the following switch could also be done using fi.Mode(), not sure if there
-		// a benefit of using one vs. the other.
-		// fi := header.FileInfo()
+		// While this check prevents some path walking, a path that contains
+		// UTF-8 might bypass that check. For instance \u002e\u002e\u2215etc/passwd
+		// will be joined to the dst directory and appear safe. However, the open
+		// file syscall will interpret the UTF-8 and effectively allow path walking
+		// again. In production, the bot must run in a sandboxed environment.
+		if !strings.HasPrefix(target, dst) {
+			util.Warnf(ctx, "Unsafe file located outside `%s` with name: `%s`", dst, header.Name)
+			continue
+		}
 
 		// check the file type
 		switch header.Typeflag {
@@ -112,6 +119,6 @@ func DownloadTar(ctx context.Context, url string) string {
 
 	defer resp.Body.Close()
 
-	util.Check(Untar(dest, resp.Body))
+	util.Check(Untar(ctx, dest, resp.Body))
 	return dest
 }

test/checker/show_files_npm_test.go

@@ -23,6 +23,7 @@ const (
 	unpublishedFieldPkg = "unpublishedPkg"
 	sortByTimeStampPkg  = "sortByTimePkg"
 	symlinkPkg          = "symlinkPkg"
+	walkerPkg           = "walkerPkg"
 	timeStamp1          = "1.0.0"
 	timeStamp2          = "2.0.0"
 	timeStamp3          = "3.0.0"
@@ -222,6 +223,20 @@ func fakeNpmHandlerShowFiles(w http.ResponseWriter, r *http.Request) {
 				"latest": "0.0.2"
 			}
 		}`)
+	case "/" + walkerPkg:
+		fmt.Fprint(w, `{
+			"versions": {
+				"0.0.2": {
+					"dist": {
+						"tarball": "http://registry.npmjs.org/`+walkerPkg+`.tgz"
+					}
+				}
+			},
+			"time": { "0.0.2": "2012-06-19T04:01:32.220Z" },
+			"dist-tags": {
+				"latest": "0.0.2"
+			}
+		}`)
 	case "/" + jsFilesPkg + ".tgz":
 		servePackage(w, r, map[string]VirtualFile{
 			"a.js": VirtualFile{Content: "a"},
@@ -264,6 +279,12 @@ func fakeNpmHandlerShowFiles(w http.ResponseWriter, r *http.Request) {
 			"b.js": VirtualFile{LinkTo: "/dev/urandom"},
 			"c.js": VirtualFile{Content: "/dev/urandom"},
 		})
+	case "/" + walkerPkg + ".tgz":
+		servePackage(w, r, map[string]VirtualFile{
+			"a.js":          VirtualFile{Content: "a"},
+			"../../b.js":    VirtualFile{Content: "b"},
+			"../../../c.js": VirtualFile{Content: "c"},
+		})
 	default:
 		panic("unreachable: " + r.URL.Path)
 	}
@@ -595,3 +616,68 @@ c.js
 	assert.Contains(t, out, expected)
 	assert.Nil(t, testproxy.Shutdown(context.Background()))
 }
+
+func TestCheckerShowFilesTarWalker(t *testing.T) {
+	fakeBotPath := createFakeBotPath()
+	defer os.RemoveAll(fakeBotPath)
+
+	httpTestProxy := "localhost:8666"
+	pkgFile := path.Join(fakeBotPath, "packages", "packages", "i", "input-show-files.json")
+	input := `{
+		"name": "a-happy-tyler",
+		"description": "Tyler is happy. Be like Tyler.",
+		"keywords": [
+			"tyler",
+			"happy"
+		],
+		"authors": [
+			{
+				"name": "Tyler Caslin",
+				"email": "tylercaslin47@gmail.com",
+				"url": "https://github.com/tc80"
+			}
+		],
+		"license": "MIT",
+		"repository": {
+			"type": "git",
+			"url": "git://github.com/tc80/a-happy-tyler.git"
+		},
+		"filename": "a.js",
+		"homepage": "https://github.com/tc80",
+		"autoupdate": {
+			"source": "npm",
+			"target": "` + walkerPkg + `",
+			"fileMap": [
+				{ "basePath":"", "files":["*.js"] }
+			]
+		}
+	}`
+	expected := []string{`
+` + "```" + `
+a.js
+` + "```" + ``,
+		"Unsafe file located outside", "with name: `package/../../b.js`",
+		"Unsafe file located outside", "with name: `package/../../../c.js`",
+	}
+
+	err := ioutil.WriteFile(pkgFile, []byte(input), 0644)
+	assert.Nil(t, err)
+	defer os.Remove(pkgFile)
+
+	testproxy := &http.Server{
+		Addr:    httpTestProxy,
+		Handler: http.Handler(http.HandlerFunc(fakeNpmHandlerShowFiles)),
+	}
+
+	go func() {
+		if err := testproxy.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+			panic(err)
+		}
+	}()
+
+	out := runChecker(fakeBotPath, httpTestProxy, false, "show-files", pkgFile)
+	for _, text := range expected {
+		assert.Contains(t, out, text)
+	}
+	assert.Nil(t, testproxy.Shutdown(context.Background()))
+}