fusefrontend: do not encrypt ACLs

Pass through system.posix_acl_access and system.posix_acl_default
unencrypted to fix "cp -a" problems.

"cp -a" uses "setxattr" even to set normal permissions, see
https://www.spinics.net/lists/linux-nfs/msg63986.html .

Fixes https://github.com/rfjakob/gocryptfs/issues/543

README.md

@@ -196,6 +196,9 @@ vNEXT, in progress
 * Make `gocryptfs.diriv` and `gocryptfs.xxx.name` files world-readable to make encrypted backups easier
   when mounting via [/etc/fstab](Documentation/MANPAGE.md#fstab) ([#539](https://github.com/rfjakob/gocryptfs/issues/539))
 * Make it work with MacFUSE v4.x ([#524](https://github.com/rfjakob/gocryptfs/issues/524))
+* **Disable ACL encryption**, it causes a lot of problems ([#543](https://github.com/rfjakob/gocryptfs/issues/543), [#536](https://github.com/rfjakob/gocryptfs/issues/536))
+  * Old encrypted ACLs are reported by `gocryptfs -fsck` but otherwise ignored
+  * This fixes inheritance, but does not yet enforce them correctly
 
 v2.0-beta2, 2020-11-14
 * Improve [performance](Documentation/performance.txt#L69)
@@ -222,7 +225,7 @@ v2.0-beta1, 2020-10-15
 
 v1.8.0, 2020-05-09
 * Enable ACL support ([#453](https://github.com/rfjakob/gocryptfs/issues/453))
-  * **Warning 2021-02-07: This feature is incomplete!**
+  * **Warning 2021-02-07**: This feature is incomplete! Do not use ACLs before gocryptfs v2.0 final!
     Reading and writing ACLs works, but they are not enforced or inherited ([#542](https://github.com/rfjakob/gocryptfs/issues/542))
 * Ignore `.nfsXXX` temporary files
   ([#367](https://github.com/rfjakob/gocryptfs/issues/431))

internal/fusefrontend/node_xattr.go

@@ -10,6 +10,9 @@ import (
 	"github.com/rfjakob/gocryptfs/internal/tlog"
 )
 
+// -1 as uint32
+const minus1 = ^uint32(0)
+
 // xattr names are encrypted like file names, but with a fixed IV.
 // Padded with "_xx" for length 16.
 var xattrNameIV = []byte("xattr_name_iv_xx")
@@ -22,6 +25,13 @@ var xattrStorePrefix = "user.gocryptfs."
 // see https://github.com/rfjakob/gocryptfs/issues/515 for details.
 var xattrCapability = "security.capability"
 
+// isAcl returns true if the attribute name is for storing ACLs
+//
+// ACLs are passed through without encryption
+func isAcl(attr string) bool {
+	return attr == "system.posix_acl_access" || attr == "system.posix_acl_default"
+}
+
 // GetXAttr - FUSE call. Reads the value of extended attribute "attr".
 //
 // This function is symlink-safe through Fgetxattr.
@@ -36,15 +46,34 @@ func (n *Node) Getxattr(ctx context.Context, attr string, dest []byte) (uint32,
 		// and it did not cause trouble. Seems cleaner than saying ENODATA.
 		return 0, syscall.EOPNOTSUPP
 	}
+	var data []byte
+	// ACLs are passed through without encryption
+	if isAcl(attr) {
+		var errno syscall.Errno
+		data, errno = n.getXAttr(attr)
+		if errno != 0 {
+			return minus1, errno
+		}
+	} else {
+		// encrypted user xattr
 		cAttr := rn.encryptXattrName(attr)
 		cData, errno := n.getXAttr(cAttr)
 		if errno != 0 {
 			return 0, errno
 		}
-	data, err := rn.decryptXattrValue(cData)
+		var err error
+		data, err = rn.decryptXattrValue(cData)
 		if err != nil {
 			tlog.Warn.Printf("GetXAttr: %v", err)
-		return ^uint32(0), syscall.EIO
+			return minus1, syscall.EIO
+		}
+	}
+	// Caller passes size zero to find out how large their buffer should be
+	if len(dest) == 0 {
+		return uint32(len(data)), 0
+	}
+	if len(dest) < len(data) {
+		return minus1, syscall.ERANGE
 	}
 	l := copy(dest, data)
 	return uint32(l), 0
@@ -56,6 +85,12 @@ func (n *Node) Getxattr(ctx context.Context, attr string, dest []byte) (uint32,
 func (n *Node) Setxattr(ctx context.Context, attr string, data []byte, flags uint32) syscall.Errno {
 	rn := n.rootNode()
 	flags = uint32(filterXattrSetFlags(int(flags)))
+
+	// ACLs are passed through without encryption
+	if isAcl(attr) {
+		return n.setXAttr(attr, data, flags)
+	}
+
 	cAttr := rn.encryptXattrName(attr)
 	cData := rn.encryptXattrValue(data)
 	return n.setXAttr(cAttr, cData, flags)
@@ -66,6 +101,12 @@ func (n *Node) Setxattr(ctx context.Context, attr string, data []byte, flags uin
 // This function is symlink-safe through Fremovexattr.
 func (n *Node) Removexattr(ctx context.Context, attr string) syscall.Errno {
 	rn := n.rootNode()
+
+	// ACLs are passed through without encryption
+	if isAcl(attr) {
+		return n.removeXAttr(attr)
+	}
+
 	cAttr := rn.encryptXattrName(attr)
 	return n.removeXAttr(cAttr)
 }
@@ -81,6 +122,11 @@ func (n *Node) Listxattr(ctx context.Context, dest []byte) (uint32, syscall.Errn
 	rn := n.rootNode()
 	var buf bytes.Buffer
 	for _, curName := range cNames {
+		// ACLs are passed through without encryption
+		if isAcl(curName) {
+			buf.WriteString(curName + "\000")
+			continue
+		}
 		if !strings.HasPrefix(curName, xattrStorePrefix) {
 			continue
 		}
@@ -90,10 +136,20 @@ func (n *Node) Listxattr(ctx context.Context, dest []byte) (uint32, syscall.Errn
 			rn.reportMitigatedCorruption(curName)
 			continue
 		}
+		// We *used to* encrypt ACLs, which caused a lot of problems.
+		if isAcl(name) {
+			tlog.Warn.Printf("ListXAttr: ignoring deprecated encrypted ACL %q = %q", curName, name)
+			rn.reportMitigatedCorruption(curName)
+			continue
+		}
 		buf.WriteString(name + "\000")
 	}
+	// Caller passes size zero to find out how large their buffer should be
+	if len(dest) == 0 {
+		return uint32(buf.Len()), 0
+	}
 	if buf.Len() > len(dest) {
-		return ^uint32(0), syscall.ERANGE
+		return minus1, syscall.ERANGE
 	}
 	return uint32(copy(dest, buf.Bytes())), 0
 }

tests/defaults/acl_test.go

@@ -0,0 +1,202 @@
+package defaults
+
+import (
+	"io/ioutil"
+	"math/rand"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"golang.org/x/sys/unix"
+
+	"github.com/pkg/xattr"
+
+	"github.com/rfjakob/gocryptfs/tests/test_helpers"
+)
+
+// https://github.com/rfjakob/gocryptfs/issues/543
+func TestCpA(t *testing.T) {
+	fn1 := filepath.Join(test_helpers.TmpDir, t.Name())
+	fn2 := filepath.Join(test_helpers.DefaultPlainDir, t.Name())
+
+	rand.Seed(int64(time.Now().Nanosecond()))
+
+	{
+		// Need unrestricted umask
+		old := syscall.Umask(000)
+		defer syscall.Umask(old)
+	}
+
+	for i := 0; i < 10; i++ {
+		// Random permissions (except owner read, which cp needs)
+		var modeWant os.FileMode = os.FileMode(rand.Int31n(0777+1) | 0400)
+
+		// Create file outside mount
+		err := ioutil.WriteFile(fn1, nil, modeWant)
+		if err != nil {
+			t.Fatal(err)
+		}
+		// Verify perms (umask problems?)
+		fi, err := os.Stat(fn1)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if fi.Mode() != modeWant {
+			t.Errorf("ioutil.WriteFile created wrong permissions: want %o have %o", modeWant, fi.Mode())
+		}
+
+		// "cp -a" from outside to inside mount
+		c := exec.Command("cp", "-a", fn1, fn2)
+		c.Stderr = os.Stderr
+		c.Stdout = os.Stdout
+		err = c.Run()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Check perms
+		fi, err = os.Stat(fn2)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if fi.Mode() != modeWant {
+			t.Errorf("cp -a did not preserve permissions: want %o have %o", modeWant, fi.Mode())
+		}
+
+		syscall.Unlink(fn1)
+		syscall.Unlink(fn2)
+	}
+}
+
+func getfacl(fn string) (string, error) {
+	c := exec.Command("getfacl", "-c", "--", fn)
+	out, err := c.Output()
+	return string(out), err
+}
+
+// https://github.com/rfjakob/gocryptfs/issues/543
+func TestAcl543(t *testing.T) {
+	fn1 := test_helpers.TmpDir + "/TestAcl543"
+	fn2 := test_helpers.DefaultPlainDir + "/TestAcl543"
+
+	var c *exec.Cmd
+
+	var modeWant os.FileMode = 0777
+
+	{
+		// Need unrestricted umask
+		old := syscall.Umask(000)
+		defer syscall.Umask(old)
+	}
+
+	// Set acl on file outside gocryptfs mount
+	err := ioutil.WriteFile(fn1, nil, modeWant)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c = exec.Command("setfacl", "-m", "u:daemon:rwx", fn1)
+	c.Stderr = os.Stderr
+	c.Stdout = os.Stdout
+	err = c.Run()
+	if err != nil {
+		t.Skip(err)
+	}
+	aclWant, err := getfacl(fn1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fi, err := os.Stat(fn1)
+	if fi.Mode() != modeWant {
+		t.Fatalf("mode changed from %o to %o", modeWant, fi.Mode())
+	}
+
+	// Set acl on file inside gocryptfs mount
+	err = ioutil.WriteFile(fn2, nil, modeWant)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c = exec.Command("setfacl", "-m", "u:daemon:rwx", fn2)
+	c.Stderr = os.Stderr
+	c.Stdout = os.Stdout
+	err = c.Run()
+	if err != nil {
+		t.Fatal(err)
+	}
+	aclHave1, err := getfacl(fn1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if aclHave1 != aclWant {
+		t.Error(aclHave1)
+	}
+	os.Remove(fn2)
+
+	// "cp -a" from outside to inside mount
+	c = exec.Command("cp", "-a", fn1, fn2)
+	c.Stderr = os.Stderr
+	c.Stdout = os.Stdout
+	err = c.Run()
+	if err != nil {
+		t.Fatal(err)
+	}
+	fi, err = os.Stat(fn2)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if fi.Mode() != modeWant {
+		t.Errorf("cp -a did not preserve permissions: want %o have %o", modeWant, fi.Mode())
+	}
+	aclHave2, err := getfacl(fn2)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if aclHave2 != aclWant {
+		t.Errorf("cp -a did not preserve acl: %q", aclHave1)
+	}
+}
+
+// Check that we handle zero-sized and undersized buffers correctly
+func TestXattrOverflow(t *testing.T) {
+	fn := filepath.Join(test_helpers.DefaultPlainDir, t.Name())
+	ioutil.WriteFile(fn, nil, 0600)
+
+	attr := "user.foo123"
+	val := []byte("12341234")
+	err := xattr.LSet(fn, attr, val)
+	if err != nil {
+		t.Skip(err)
+	}
+
+	// Getxattr
+	sz, err := unix.Lgetxattr(fn, attr, nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if sz != len(val) {
+		t.Errorf("wrong sz: want %d have %d", len(val), sz)
+	}
+	sz, err = unix.Lgetxattr(fn, attr, make([]byte, 1))
+	if err != syscall.ERANGE {
+		t.Error(err)
+	}
+
+	// Listxattr
+	szWant, err := unix.Llistxattr(fn, make([]byte, 64*1024))
+	if err != nil {
+		t.Fatal(err)
+	}
+	sz, err = unix.Llistxattr(fn, nil)
+	if err != nil {
+		t.Error(err)
+	}
+	if sz != szWant {
+		t.Errorf("wrong sz: want %d have %d", szWant, sz)
+	}
+	sz, err = unix.Llistxattr(fn, make([]byte, 1))
+	if err != syscall.ERANGE {
+		t.Error(err)
+	}
+}