Browse Source

drive: fix index out of bounds when parsing request local paths (#15517)

Fix the index out of bound panic when a request is made to the local
fileserver mux with a valid secret-token, but missing share name.

Example error:

    http: panic serving 127.0.0.1:40974: runtime error: slice bounds out of range [2:1]

Additionally, we document the edge case behavior of utilities that
this fileserver mux depends on.

Signed-off-by: Craig Hesling <[email protected]>
Craig Hesling 11 months ago
parent
commit
b9277ade1f

+ 69 - 4
drive/driveimpl/drive_test.go

@@ -133,6 +133,71 @@ func TestPermissions(t *testing.T) {
 	}
 	}
 }
 }
 
 
+// TestMissingPaths verifies that the fileserver running at localhost
+// correctly handles paths with missing required components.
+//
+// Expected path format:
+// http://localhost:[PORT]/<secretToken>/<share>[/<subSharePath...>]
+func TestMissingPaths(t *testing.T) {
+	s := newSystem(t)
+
+	fileserverAddr := s.addRemote(remote1)
+	s.addShare(remote1, share11, drive.PermissionReadWrite)
+
+	client := &http.Client{
+		Transport: &http.Transport{DisableKeepAlives: true},
+	}
+	addr := strings.Split(fileserverAddr, "|")[1]
+	secretToken := strings.Split(fileserverAddr, "|")[0]
+
+	testCases := []struct {
+		name       string
+		path       string
+		wantStatus int
+	}{
+		{
+			name:       "empty path",
+			path:       "",
+			wantStatus: http.StatusForbidden,
+		},
+		{
+			name:       "single slash",
+			path:       "/",
+			wantStatus: http.StatusForbidden,
+		},
+		{
+			name:       "only token",
+			path:       "/" + secretToken,
+			wantStatus: http.StatusBadRequest,
+		},
+		{
+			name:       "token with trailing slash",
+			path:       "/" + secretToken + "/",
+			wantStatus: http.StatusBadRequest,
+		},
+		{
+			name:       "token and invalid share",
+			path:       "/" + secretToken + "/nonexistentshare",
+			wantStatus: http.StatusNotFound,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			u := fmt.Sprintf("http://%s%s", addr, tc.path)
+			resp, err := client.Get(u)
+			if err != nil {
+				t.Fatalf("unexpected error making request: %v", err)
+			}
+			defer resp.Body.Close()
+
+			if resp.StatusCode != tc.wantStatus {
+				t.Errorf("got status code %d, want %d", resp.StatusCode, tc.wantStatus)
+			}
+		})
+	}
+}
+
 // TestSecretTokenAuth verifies that the fileserver running at localhost cannot
 // TestSecretTokenAuth verifies that the fileserver running at localhost cannot
 // be accessed directly without the correct secret token. This matters because
 // be accessed directly without the correct secret token. This matters because
 // if a victim can be induced to visit the localhost URL and access a malicious
 // if a victim can be induced to visit the localhost URL and access a malicious
@@ -704,8 +769,8 @@ func (a *noopAuthenticator) Close() error {
 	return nil
 	return nil
 }
 }
 
 
-const lockBody = `<?xml version="1.0" encoding="utf-8" ?> 
-<D:lockinfo xmlns:D='DAV:'> 
-  <D:lockscope><D:exclusive/></D:lockscope> 
-  <D:locktype><D:write/></D:locktype> 
+const lockBody = `<?xml version="1.0" encoding="utf-8" ?>
+<D:lockinfo xmlns:D='DAV:'>
+  <D:lockscope><D:exclusive/></D:lockscope>
+  <D:locktype><D:write/></D:locktype>
 </D:lockinfo>`
 </D:lockinfo>`

+ 4 - 0
drive/driveimpl/fileserver.go

@@ -142,6 +142,10 @@ func (s *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 		return
 	}
 	}
 
 
+	if len(parts) < 2 {
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
 	r.URL.Path = shared.Join(parts[2:]...)
 	r.URL.Path = shared.Join(parts[2:]...)
 	share := parts[1]
 	share := parts[1]
 	s.sharesMu.RLock()
 	s.sharesMu.RLock()

+ 5 - 0
drive/driveimpl/shared/pathutil.go

@@ -22,6 +22,9 @@ const (
 // CleanAndSplit cleans the provided path p and splits it into its constituent
 // CleanAndSplit cleans the provided path p and splits it into its constituent
 // parts. This is different from path.Split which just splits a path into prefix
 // parts. This is different from path.Split which just splits a path into prefix
 // and suffix.
 // and suffix.
+//
+// If p is empty or contains only path separators, CleanAndSplit returns a slice
+// of length 1 whose only element is "".
 func CleanAndSplit(p string) []string {
 func CleanAndSplit(p string) []string {
 	return strings.Split(strings.Trim(path.Clean(p), sepStringAndDot), sepString)
 	return strings.Split(strings.Trim(path.Clean(p), sepStringAndDot), sepString)
 }
 }
@@ -38,6 +41,8 @@ func Parent(p string) string {
 }
 }
 
 
 // Join behaves like path.Join() but also includes a leading slash.
 // Join behaves like path.Join() but also includes a leading slash.
+//
+// When parts are missing, the result is "/".
 func Join(parts ...string) string {
 func Join(parts ...string) string {
 	fullParts := make([]string, 0, len(parts))
 	fullParts := make([]string, 0, len(parts))
 	fullParts = append(fullParts, sepString)
 	fullParts = append(fullParts, sepString)

+ 1 - 0
drive/driveimpl/shared/pathutil_test.go

@@ -40,6 +40,7 @@ func TestJoin(t *testing.T) {
 		parts []string
 		parts []string
 		want  string
 		want  string
 	}{
 	}{
+		{[]string{}, "/"},
 		{[]string{""}, "/"},
 		{[]string{""}, "/"},
 		{[]string{"a"}, "/a"},
 		{[]string{"a"}, "/a"},
 		{[]string{"/a"}, "/a"},
 		{[]string{"/a"}, "/a"},