From 19a0e83b50fbcfd89927baedbb1f1fd14dc448ca Mon Sep 17 00:00:00 2001
From: Fabricio Voznika <fvoznika@google.com>
Date: Mon, 4 Jun 2018 18:04:05 -0700
Subject: [PATCH] Make fsgofer attach more strict

Refuse to mount paths with "." and ".." in the path to prevent
a compromised Sentry to mount "../../secrets". Only allow
Attach to be called once per mount point.

PiperOrigin-RevId: 199225929
Change-Id: I2a3eb7ea0b23f22eb8dde2e383e32563ec003bd5
---
 runsc/fsgofer/fsgofer.go      | 25 ++++++++++++++++----
 runsc/fsgofer/fsgofer_test.go | 43 +++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go
index cd6224de3..f685738c3 100644
--- a/runsc/fsgofer/fsgofer.go
+++ b/runsc/fsgofer/fsgofer.go
@@ -26,7 +26,6 @@ import (
 	"math"
 	"os"
 	"path"
-	"path/filepath"
 	"strings"
 	"sync"
 	"syscall"
@@ -83,6 +82,9 @@ type Config struct {
 type attachPoint struct {
 	prefix string
 	conf   Config
+
+	mu       sync.Mutex
+	attached bool
 }
 
 // NewAttachPoint creates a new attacher that gives local file
@@ -93,19 +95,22 @@ func NewAttachPoint(prefix string, c Config) p9.Attacher {
 
 // Attach implements p9.Attacher.
 func (a *attachPoint) Attach(appPath string) (p9.File, error) {
+	// Only proceed if 'appPath' is valid.
 	if !path.IsAbs(appPath) {
 		return nil, fmt.Errorf("invalid path %q", appPath)
 	}
+	if path.Clean(appPath) != appPath {
+		return nil, fmt.Errorf("invalid path %q", appPath)
+	}
 
-	root := filepath.Join(a.prefix, appPath)
+	root := path.Join(a.prefix, appPath)
 	fi, err := os.Stat(root)
 	if err != nil {
 		return nil, err
 	}
-
-	mode := syscall.O_RDWR
+	mode := os.O_RDWR
 	if a.conf.ROMount || fi.IsDir() {
-		mode = syscall.O_RDONLY
+		mode = os.O_RDONLY
 	}
 
 	f, err := os.OpenFile(root, mode|openFlags, 0)
@@ -114,8 +119,18 @@ func (a *attachPoint) Attach(appPath string) (p9.File, error) {
 	}
 	stat, err := stat(int(f.Fd()))
 	if err != nil {
+		f.Close()
 		return nil, fmt.Errorf("failed to stat file %q, err: %v", root, err)
 	}
+
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	if a.attached {
+		f.Close()
+		return nil, fmt.Errorf("attach point already attached, prefix: %s", a.prefix)
+	}
+	a.attached = true
+
 	return newLocalFile(a.conf, f, root, stat)
 }
 
diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go
index 249f67bf9..8d038eaf6 100644
--- a/runsc/fsgofer/fsgofer_test.go
+++ b/runsc/fsgofer/fsgofer_test.go
@@ -19,6 +19,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"strings"
 	"syscall"
 	"testing"
 
@@ -622,3 +623,45 @@ func TestAttachFile(t *testing.T) {
 		t.Fatalf("ReadAt() wrong data, got: %s, expected: %s", string(rBuf), "foobar")
 	}
 }
+
+func TestAttachError(t *testing.T) {
+	conf := Config{ROMount: false}
+	root, err := ioutil.TempDir("", "root-")
+	if err != nil {
+		t.Fatalf("ioutil.TempDir() failed, err: %v", err)
+	}
+	defer os.RemoveAll(root)
+	a := NewAttachPoint(root, conf)
+
+	c := path.Join(root, "test")
+	if err := os.Mkdir(c, 0700); err != nil {
+		t.Fatalf("os.Create(%q) failed, err: %v", c, err)
+	}
+
+	for _, p := range []string{"test", "/test/../", "/test/./", "/test//"} {
+		_, err := a.Attach(p)
+		if err == nil {
+			t.Fatalf("Attach(%q) should have failed", p)
+		}
+		if want := "invalid path"; !strings.Contains(err.Error(), want) {
+			t.Fatalf("Attach(%q) wrong error, got: %v, wanted: %v", p, err, want)
+		}
+	}
+}
+
+func TestDoubleAttachError(t *testing.T) {
+	conf := Config{ROMount: false}
+	root, err := ioutil.TempDir("", "root-")
+	if err != nil {
+		t.Fatalf("ioutil.TempDir() failed, err: %v", err)
+	}
+	defer os.RemoveAll(root)
+	a := NewAttachPoint(root, conf)
+
+	if _, err := a.Attach("/"); err != nil {
+		t.Fatalf("Attach(%q) failed: %v", "/", err)
+	}
+	if _, err := a.Attach("/"); err == nil {
+		t.Fatalf("Attach(%q) should have failed", "test")
+	}
+}