From bc9a7278a4467445ac6aab90011f59961f7bbdba Mon Sep 17 00:00:00 2001 From: Roman Perekhod <2403905@gmail.com> Date: Wed, 11 Nov 2026 08:17:00 +0100 Subject: [PATCH 1/4] fix: [OCISDEV-1140] check the grant read result before the expiration The function ReadUserPermissions in pkg/storage/utils/decomposedfs/node/node.go checked the expiration of a grant before it checked the read error. A failed read returned a nil grant, and the check caused a nil pointer panic. - Check the expiration only when the grant read succeeds. - Return false from isGrantExpired for a nil grant. - Add the node path to the log message for a failed grant read. - Add a unit test for isGrantExpired. Co-Authored-By: Claude Sonnet 5.5 --- .../bugfix-decomposedfs-nil-grant-panic.md | 7 ++++ .../decomposedfs/node/grant_expired_test.go | 33 +++++++++++++++++++ pkg/storage/utils/decomposedfs/node/node.go | 12 +++---- 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md create mode 100644 pkg/storage/utils/decomposedfs/node/grant_expired_test.go diff --git a/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md new file mode 100644 index 00000000000..cf620a5f87e --- /dev/null +++ b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md @@ -0,0 +1,7 @@ +Bugfix: Fix a crash when decomposedfs reads permissions + +The storage provider could stop when it read the permissions of a user. +We changed the code. The code now checks the result of a read before it uses +the result. + +https://github.com/owncloud/reva/pull/804 diff --git a/pkg/storage/utils/decomposedfs/node/grant_expired_test.go b/pkg/storage/utils/decomposedfs/node/grant_expired_test.go new file mode 100644 index 00000000000..879a397cdef --- /dev/null +++ b/pkg/storage/utils/decomposedfs/node/grant_expired_test.go @@ -0,0 +1,33 @@ +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) + } + }) + } +} diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index fbc9e5ebdc4..6b4d734e826 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/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 } } From b9e8cf57cc4dde05cac189d439d52d75a737870c Mon Sep 17 00:00:00 2001 From: Roman Perekhod <2403905@gmail.com> Date: Wed, 11 Nov 2026 13:51:11 +0100 Subject: [PATCH 2/4] test: [OCISDEV-1140] test ReadUserPermissions with an unreadable grant - Add a test for the function ReadUserPermissions in pkg/storage/utils/decomposedfs/node/node_test.go. The test stores a grant that ReadGrant cannot read. The test checks that the function does not panic and gives no permissions. - Add the license header to pkg/storage/utils/decomposedfs/node/grant_expired_test.go. Co-Authored-By: Claude Sonnet 5.5 --- .../decomposedfs/node/grant_expired_test.go | 18 +++++++++++++++++ .../utils/decomposedfs/node/node_test.go | 20 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/pkg/storage/utils/decomposedfs/node/grant_expired_test.go b/pkg/storage/utils/decomposedfs/node/grant_expired_test.go index 879a397cdef..6745b78b44c 100644 --- a/pkg/storage/utils/decomposedfs/node/grant_expired_test.go +++ b/pkg/storage/utils/decomposedfs/node/grant_expired_test.go @@ -1,3 +1,21 @@ +// 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 ( diff --git a/pkg/storage/utils/decomposedfs/node/node_test.go b/pkg/storage/utils/decomposedfs/node/node_test.go index 4c4b54bab42..968afd610bd 100644 --- a/pkg/storage/utils/decomposedfs/node/node_test.go +++ b/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()) From adc8163b101c7c03bbee787a7f668926028cc715 Mon Sep 17 00:00:00 2001 From: Roman Perekhod <2403905@gmail.com> Date: Wed, 11 Nov 2026 14:01:51 +0100 Subject: [PATCH 3/4] fix: [OCISDEV-1140] return an error for an invalid ace value The function Unmarshal in pkg/storage/utils/ace/ace.go ignored the error from unmarshalKV and then read a field of a nil value. The function also read the first byte of an empty value. Both cases caused a panic. - Return the error when unmarshalKV fails. - Return an error for an empty value. - Add tests in pkg/storage/utils/ace/ace_test.go. - Update the changelog entry. Co-Authored-By: Claude Sonnet 5.5 --- .../bugfix-decomposedfs-nil-grant-panic.md | 2 +- pkg/storage/utils/ace/ace.go | 8 +++-- pkg/storage/utils/ace/ace_test.go | 30 +++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md index cf620a5f87e..37df9aeeff2 100644 --- a/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md +++ b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md @@ -2,6 +2,6 @@ Bugfix: Fix a crash when decomposedfs reads permissions The storage provider could stop when it read the permissions of a user. We changed the code. The code now checks the result of a read before it uses -the result. +the result. The code also rejects invalid permission data and does not stop. https://github.com/owncloud/reva/pull/804 diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index d6ff52f9589..05739f8512f 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -247,13 +247,17 @@ 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 } + e.principal = principal // check consistency of Flags and principal type if strings.Contains(e.flags, "g") { if principal[:1] != "g" { diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index 8bc4312bffa..9f23de602de 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -100,6 +100,36 @@ 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()) + }) + }) + Describe("converting permissions", func() { It("converts t", func() { userGrant.Permissions.Stat = true From e3b96ab89e8b085a76dd224f4fe0f2bbac4a1334 Mon Sep 17 00:00:00 2001 From: Roman Perekhod <2403905@gmail.com> Date: Thu, 12 Nov 2026 11:24:04 +0100 Subject: [PATCH 4/4] fix: [OCISDEV-1140] reject an invalid ace principal and key value pair The function Unmarshal in pkg/storage/utils/ace/ace.go read the first character of the principal without a length check. An empty principal caused a panic. A principal with one character passed the check, and the function Grant in pkg/storage/utils/ace/ace.go then caused a panic. The function unmarshalKV in pkg/storage/utils/ace/ace.go read the value of a field without an equals sign, and this also caused a panic. - Return an error for a principal that does not have the form ":". - Return an error for a field without an equals sign. - Keep the full value when the value contains an equals sign. - Ignore empty fields, as the old parser did. - Add tests in pkg/storage/utils/ace/ace_test.go. Co-Authored-By: Claude Opus 5.5 --- .../bugfix-decomposedfs-nil-grant-panic.md | 13 ++++- pkg/storage/utils/ace/ace.go | 35 ++++++++----- pkg/storage/utils/ace/ace_test.go | 41 +++++++++++++++ .../utils/decomposedfs/grant_expired_test.go | 51 +++++++++++++++++++ pkg/storage/utils/decomposedfs/spaces.go | 2 +- 5 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 pkg/storage/utils/decomposedfs/grant_expired_test.go diff --git a/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md index 37df9aeeff2..cd4181e809a 100644 --- a/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md +++ b/changelog/unreleased/bugfix-decomposedfs-nil-grant-panic.md @@ -1,7 +1,16 @@ Bugfix: Fix a crash when decomposedfs reads permissions The storage provider could stop when it read the permissions of a user. -We changed the code. The code now checks the result of a read before it uses -the result. The code also rejects invalid permission data and does not stop. +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/804 diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index 05739f8512f..b336d7d6aa5 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -257,14 +257,18 @@ func Unmarshal(principal string, v []byte) (e *ACE, err error) { if e, err = unmarshalKV(s); err != nil { return nil, err } + // the principal must have the form ":" + 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") } } @@ -407,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 ... } } diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index 9f23de602de..e8290fe0a72 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -128,6 +128,47 @@ var _ = Describe("ACE", func() { 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() { diff --git a/pkg/storage/utils/decomposedfs/grant_expired_test.go b/pkg/storage/utils/decomposedfs/grant_expired_test.go new file mode 100644 index 00000000000..5e1da9bcc98 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index 6d582719a64..4b340df2bce 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/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)))