fix: [OCISDEV-1140] check the grant read result before the expiration

changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md

@@ -0,0 +1,16 @@
+Bugfix: Fix a crash when decomposedfs reads permissions
+
+The storage provider could stop when it read the permissions of a user.
+A grant with invalid data on disk caused a panic. The panic stopped the full
+storage provider process.
+
+We changed the code:
+
+- The code checks the result of a grant read before it uses the result.
+- The code checks for a missing grant before it checks the expiration.
+- The code returns an error for an invalid grant value or grantee name.
+- The code keeps the full value of a grant field that contains an equals sign.
+
+The storage provider now logs the invalid grant and does not stop.
+
+https://github.com/owncloud/reva/pull/811

pkg/storage/utils/ace/ace.go

@@ -247,20 +247,28 @@ func (e *ACE) Marshal() (string, []byte) {
 
 // Unmarshal parses a principal string and byte[] into an ACE
 func Unmarshal(principal string, v []byte) (e *ACE, err error) {
+	if len(v) == 0 {
+		return nil, fmt.Errorf("empty ace")
+	}
 	// first byte indicates type of value
 	switch v[0] {
 	case 0: // = ':' separated key=value pairs
 		s := string(v[1:])
-		if e, err = unmarshalKV(s); err == nil {
-			e.principal = principal
+		if e, err = unmarshalKV(s); err != nil {
+			return nil, err
+		}
+		// the principal must have the form "<type>:<id>"
+		if len(principal) < 2 || principal[1] != ':' {
+			return nil, fmt.Errorf("invalid ace principal %q", principal)
 		}
+		e.principal = principal
 		// check consistency of Flags and principal type
 		if strings.Contains(e.flags, "g") {
-			if principal[:1] != "g" {
+			if principal[0] != 'g' {
 				return nil, fmt.Errorf("inconsistent ace: expected group")
 			}
 		} else {
-			if principal[:1] != "u" {
+			if principal[0] != 'u' {
 				return nil, fmt.Errorf("inconsistent ace: expected user")
 			}
 		}
@@ -403,32 +411,39 @@ func unmarshalKV(s string) (*ACE, error) {
 		return nil, fmt.Errorf("more than one row of ace kvs")
 	}
 	for i := range records[0] {
-		kv := strings.Split(records[0][i], "=")
-		switch kv[0] {
+		if records[0][i] == "" {
+			// the old parser also ignored empty fields
+			continue
+		}
+		key, value, ok := strings.Cut(records[0][i], "=")
+		if !ok {
+			return nil, fmt.Errorf("invalid ace key value pair %q", records[0][i])
+		}
+		switch key {
 		case "t":
-			e._type = kv[1]
+			e._type = value
 		case "f":
-			e.flags = kv[1]
+			e.flags = value
 		case "p":
-			e.permissions = kv[1]
+			e.permissions = value
 		case "s":
-			v, err := strconv.Atoi(kv[1])
+			v, err := strconv.Atoi(value)
 			if err != nil {
 				return nil, err
 			}
 			e.shareTime = v
 		case "c":
-			e.creator = kv[1]
+			e.creator = value
 		case "e":
-			v, err := strconv.ParseInt(kv[1], 10, 64)
+			v, err := strconv.ParseInt(value, 10, 64)
 			if err != nil {
 				return nil, err
 			}
 			e.expires = v
 		case "w":
-			e.password = kv[1]
+			e.password = value
 		case "l":
-			e.label = kv[1]
+			e.label = value
 			// TODO default ... log unknown keys? or add as opaque? hm we need that for tagged shares ...
 		}
 	}

pkg/storage/utils/ace/ace_test.go

@@ -100,6 +100,77 @@ var _ = Describe("ACE", func() {
 		})
 	})
 
+	Describe("unmarshalling invalid values", func() {
+		It("returns an error for an empty value", func() {
+			e, err := ace.Unmarshal("u:einstein", []byte{})
+			Expect(err).To(HaveOccurred())
+			Expect(e).To(BeNil())
+		})
+		It("returns an error for an unknown encoding", func() {
+			e, err := ace.Unmarshal("u:einstein", []byte{1})
+			Expect(err).To(HaveOccurred())
+			Expect(e).To(BeNil())
+		})
+		It("returns an error and does not panic for a malformed key value body", func() {
+			var (
+				e   *ace.ACE
+				err error
+			)
+			Expect(func() {
+				e, err = ace.Unmarshal("u:einstein", append([]byte{0}, []byte(`t=A:s=notanumber`)...))
+			}).ToNot(Panic())
+			Expect(err).To(HaveOccurred())
+			Expect(e).To(BeNil())
+
+			Expect(func() {
+				e, err = ace.Unmarshal("u:einstein", append([]byte{0}, []byte(`t="A`)...))
+			}).ToNot(Panic())
+			Expect(err).To(HaveOccurred())
+			Expect(e).To(BeNil())
+		})
+		DescribeTable("returns an error and does not panic for an invalid principal",
+			func(principal string) {
+				var (
+					e   *ace.ACE
+					err error
+				)
+				Expect(func() {
+					e, err = ace.Unmarshal(principal, append([]byte{0}, []byte("t=A:f=:p=rw:c=:e=0\n")...))
+				}).ToNot(Panic())
+				Expect(err).To(HaveOccurred())
+				Expect(e).To(BeNil())
+			},
+			Entry("empty", ""),
+			Entry("type only", "u"),
+			Entry("no separator", "ux"),
+		)
+		DescribeTable("returns an error and does not panic for a field without a value",
+			func(body string) {
+				var (
+					e   *ace.ACE
+					err error
+				)
+				Expect(func() {
+					e, err = ace.Unmarshal("u:einstein", append([]byte{0}, []byte(body)...))
+				}).ToNot(Panic())
+				Expect(err).To(HaveOccurred())
+				Expect(e).To(BeNil())
+			},
+			Entry("known key", "t:f=:p=rw\n"),
+			Entry("unknown key", "t=A:x:p=rw\n"),
+		)
+		It("keeps the full value when the value contains an equals sign", func() {
+			e, err := ace.Unmarshal("u:einstein", append([]byte{0}, []byte("t=A:f=:p=rw:c=a=b:e=0\n")...))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(e.Grant().GetCreator().GetOpaqueId()).To(Equal("a=b"))
+		})
+		It("ignores empty fields", func() {
+			e, err := ace.Unmarshal("u:einstein", append([]byte{0}, []byte("t=A::p=rw\n")...))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(e.Grant().GetPermissions().GetInitiateFileUpload()).To(BeTrue())
+		})
+	})
+
 	Describe("converting permissions", func() {
 		It("converts t", func() {
 			userGrant.Permissions.Stat = true

pkg/storage/utils/decomposedfs/grant_expired_test.go

@@ -0,0 +1,51 @@
+// Copyright 2018-2021 CERN
+//
+// 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.
+//
+// In applying this license, CERN does not waive the privileges and immunities
+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+package decomposedfs
+
+import (
+	"testing"
+	"time"
+
+	typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
+
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+)
+
+func TestIsGrantExpired(t *testing.T) {
+	past := &typespb.Timestamp{Seconds: uint64(time.Now().Add(-time.Hour).Unix())}
+	future := &typespb.Timestamp{Seconds: uint64(time.Now().Add(time.Hour).Unix())}
+
+	tests := []struct {
+		name string
+		g    *provider.Grant
+		want bool
+	}{
+		{"nil grant must not panic", nil, false},
+		{"no expiration", &provider.Grant{}, false},
+		{"expired", &provider.Grant{Expiration: past}, true},
+		{"not yet expired", &provider.Grant{Expiration: future}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isGrantExpired(tt.g); got != tt.want {
+				t.Fatalf("isGrantExpired() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/storage/utils/decomposedfs/node/grant_expired_test.go

@@ -0,0 +1,51 @@
+// Copyright 2018-2021 CERN
+//
+// 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.
+//
+// In applying this license, CERN does not waive the privileges and immunities
+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+package node
+
+import (
+	"testing"
+	"time"
+
+	typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
+
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+)
+
+func TestIsGrantExpired(t *testing.T) {
+	past := &typespb.Timestamp{Seconds: uint64(time.Now().Add(-time.Hour).Unix())}
+	future := &typespb.Timestamp{Seconds: uint64(time.Now().Add(time.Hour).Unix())}
+
+	tests := []struct {
+		name string
+		g    *provider.Grant
+		want bool
+	}{
+		{"nil grant must not panic", nil, false},
+		{"no expiration", &provider.Grant{}, false},
+		{"expired", &provider.Grant{Expiration: past}, true},
+		{"not yet expired", &provider.Grant{Expiration: future}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isGrantExpired(tt.g); got != tt.want {
+				t.Fatalf("isGrantExpired() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/storage/utils/decomposedfs/node/node.go

@@ -985,7 +985,7 @@ func (n *Node) UnsetTempEtag(ctx context.Context) (err error) {
 }
 
 func isGrantExpired(g *provider.Grant) bool {
-	if g.Expiration == nil {
+	if g == nil || g.Expiration == nil {
 		return false
 	}
 	return time.Now().After(time.Unix(int64(g.Expiration.Seconds), int64(g.Expiration.Nanos)))
@@ -1043,12 +1043,12 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap *pro
 			continue
 		}
 
-		if isGrantExpired(g) {
-			continue
-		}
-
 		switch {
 		case err == nil:
+			// only evaluate the expiration if the grant was read successfully
+			if isGrantExpired(g) {
+				continue
+			}
 			// If all permissions are set to false we have a deny grant
 			if grants.PermissionsEqual(g.Permissions, &provider.ResourcePermissions{}) {
 				return NoPermissions(), true, nil
@@ -1058,7 +1058,7 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap *pro
 			appctx.GetLogger(ctx).Error().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing")
 			// continue with next segment
 		default:
-			appctx.GetLogger(ctx).Error().Err(err).Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Str("grant", grantees[i]).Msg("error reading permissions")
+			appctx.GetLogger(ctx).Error().Err(err).Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Str("path", n.InternalPath()).Str("grant", grantees[i]).Msg("error reading permissions")
 			// continue with next segment
 		}
 	}

pkg/storage/utils/decomposedfs/node/node_test.go

@@ -254,6 +254,26 @@ var _ = Describe("Node", func() {
 		})
 	})
 	Describe("Permissions", func() {
+		It("does not panic and grants no access when a grant of the user cannot be read", func() {
+			storageSpace, err := env.CreateTestStorageSpace("project", &provider.Quota{QuotaMaxBytes: 2000})
+			Expect(err).ToNot(HaveOccurred())
+			spaceRoot, err := env.Lookup.NodeFromSpaceID(env.Ctx, storageSpace.SpaceId)
+			Expect(err).ToNot(HaveOccurred())
+			u := ctxpkg.ContextMustGetUser(env.Ctx)
+
+			// a value with an unknown encoding makes ReadGrant return a nil grant and an error
+			err = spaceRoot.SetXattr(env.Ctx, prefixes.GrantUserAcePrefix+u.Id.OpaqueId, []byte{1})
+			Expect(err).ToNot(HaveOccurred())
+
+			var perms *provider.ResourcePermissions
+			var denied bool
+			Expect(func() {
+				perms, denied, err = spaceRoot.ReadUserPermissions(env.Ctx, u)
+			}).ToNot(Panic())
+			Expect(err).ToNot(HaveOccurred())
+			Expect(denied).To(BeFalse())
+			Expect(grants.PermissionsEqual(perms, &provider.ResourcePermissions{})).To(BeTrue())
+		})
 		It("Checks the owner permissions on a personal space", func() {
 			node1, err := env.Lookup.NodeFromSpaceID(env.Ctx, env.SpaceRootRes.SpaceId)
 			Expect(err).ToNot(HaveOccurred())

pkg/storage/utils/decomposedfs/spaces.go

@@ -1244,7 +1244,7 @@ func mapHasKey(checkMap map[string][]byte, keys ...string) bool {
 }
 
 func isGrantExpired(g *provider.Grant) bool {
-	if g.Expiration == nil {
+	if g == nil || g.Expiration == nil {
 		return false
 	}
 	return time.Now().After(time.Unix(int64(g.Expiration.Seconds), int64(g.Expiration.Nanos)))