Merge pull request #346 from google/fixes

Metadata validation and other security improvements

7 files changed, 82 insertions, 25 deletions

diff --git a/actions/context.go b/actions/context.go
index 26295ec..ac3f6d3 100644
--- a/actions/context.go
+++ b/actions/context.go
@@ -58,6 +58,12 @@ type Context struct {
 	// the user for whom the keys are claimed in the filesystem keyring when
 	// v2 policies are provisioned.
 	TargetUser *user.User
+	// TrustedUser is the user for whom policies and protectors are allowed
+	// to be read.  Specifically, if TrustedUser is set, then only
+	// policies and protectors owned by TrustedUser or by root will be
+	// allowed to be read.  If it's nil, then all policies and protectors
+	// the process has filesystem-level read access to will be allowed.
+	TrustedUser *user.User
 }
 
 // NewContextFromPath makes a context for the filesystem containing the
@@ -112,6 +118,16 @@ func newContextFromUser(targetUser *user.User) (*Context, error) {
 		return nil, err
 	}
 
+	// By default, when running as a non-root user we only read policies and
+	// protectors owned by the user or root.  When running as root, we allow
+	// reading all policies and protectors.
+	if !ctx.Config.GetAllowCrossUserMetadata() && !util.IsUserRoot() {
+		ctx.TrustedUser, err = util.EffectiveUser()
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	log.Printf("creating context for user %q", targetUser.Username)
 	return ctx, nil
 }
@@ -122,7 +138,7 @@ func (ctx *Context) checkContext() error {
 	if err := ctx.Config.CheckValidity(); err != nil {
 		return &ErrBadConfig{ctx.Config, err}
 	}
-	return ctx.Mount.CheckSetup()
+	return ctx.Mount.CheckSetup(ctx.TrustedUser)
 }
 
 func (ctx *Context) getKeyringOptions() *keyring.Options {
@@ -136,7 +152,7 @@ func (ctx *Context) getKeyringOptions() *keyring.Options {
 // getProtectorOption returns the ProtectorOption for the protector on the
 // context's mountpoint with the specified descriptor.
 func (ctx *Context) getProtectorOption(protectorDescriptor string) *ProtectorOption {
-	mnt, data, err := ctx.Mount.GetProtector(protectorDescriptor)
+	mnt, data, err := ctx.Mount.GetProtector(protectorDescriptor, ctx.TrustedUser)
 	if err != nil {
 		return &ProtectorOption{ProtectorInfo{}, nil, err}
 	}
@@ -155,7 +171,7 @@ func (ctx *Context) ProtectorOptions() ([]*ProtectorOption, error) {
 	if err := ctx.checkContext(); err != nil {
 		return nil, err
 	}
-	descriptors, err := ctx.Mount.ListProtectors()
+	descriptors, err := ctx.Mount.ListProtectors(ctx.TrustedUser)
 	if err != nil {
 		return nil, err
 	}
diff --git a/actions/context_test.go b/actions/context_test.go
index 7b56d92..6e28857 100644
--- a/actions/context_test.go
+++ b/actions/context_test.go
@@ -27,6 +27,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/fscrypt/filesystem"
 	"github.com/google/fscrypt/util"
 	"github.com/pkg/errors"
 )
@@ -67,7 +68,7 @@ func setupContext() (ctx *Context, err error) {
 		return nil, err
 	}
 
-	return ctx, ctx.Mount.Setup()
+	return ctx, ctx.Mount.Setup(filesystem.WorldWritable)
 }
 
 // Cleans up the testing config file and testing filesystem data.
diff --git a/actions/policy.go b/actions/policy.go
index 7204380..3b20176 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"os/user"
 
 	"github.com/golang/protobuf/proto"
 	"github.com/pkg/errors"
@@ -145,7 +146,7 @@ func PurgeAllPolicies(ctx *Context) error {
 	if err := ctx.checkContext(); err != nil {
 		return err
 	}
-	policies, err := ctx.Mount.ListPolicies()
+	policies, err := ctx.Mount.ListPolicies(nil)
 	if err != nil {
 		return err
 	}
@@ -178,6 +179,7 @@ type Policy struct {
 	data                *metadata.PolicyData
 	key                 *crypto.Key
 	created             bool
+	ownerIfCreating     *user.User
 	newLinkedProtectors []string
 }
 
@@ -210,6 +212,12 @@ func CreatePolicy(ctx *Context, protector *Protector) (*Policy, error) {
 		created: true,
 	}
 
+	policy.ownerIfCreating, err = getOwnerOfMetadataForProtector(protector)
+	if err != nil {
+		policy.Lock()
+		return nil, err
+	}
+
 	if err = policy.AddProtector(protector); err != nil {
 		policy.Lock()
 		return nil, err
@@ -225,7 +233,7 @@ func GetPolicy(ctx *Context, descriptor string) (*Policy, error) {
 	if err := ctx.checkContext(); err != nil {
 		return nil, err
 	}
-	data, err := ctx.Mount.GetPolicy(descriptor)
+	data, err := ctx.Mount.GetPolicy(descriptor, ctx.TrustedUser)
 	if err != nil {
 		return nil, err
 	}
@@ -262,7 +270,7 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) {
 	descriptor := pathData.KeyDescriptor
 	log.Printf("found policy %s for %q", descriptor, path)
 
-	mountData, err := ctx.Mount.GetPolicy(descriptor)
+	mountData, err := ctx.Mount.GetPolicy(descriptor, ctx.TrustedUser)
 	if err != nil {
 		log.Printf("getting policy metadata: %v", err)
 		if _, ok := err.(*filesystem.ErrPolicyNotFound); ok {
@@ -410,6 +418,25 @@ func (policy *Policy) UsesProtector(protector *Protector) bool {
 	return ok
 }
 
+// getOwnerOfMetadataForProtector returns the User to whom the owner of any new
+// policies or protector links for the given protector should be set.
+//
+// This will return a non-nil value only when the protector is a login protector
+// and the process is running as root.  In this scenario, root is setting up
+// encryption on the user's behalf, so we need to make new policies and
+// protector links owned by the user (rather than root) to allow them to be read
+// by the user, just like the login protector itself which is handled elsewhere.
+func getOwnerOfMetadataForProtector(protector *Protector) (*user.User, error) {
+	if protector.data.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() {
+		owner, err := util.UserFromUID(protector.data.Uid)
+		if err != nil {
+			return nil, err
+		}
+		return owner, nil
+	}
+	return nil, nil
+}
+
 // AddProtector updates the data that is wrapping the Policy Key so that the
 // provided Protector is now protecting the specified Policy. If an error is
 // returned, no data has been changed. If the policy and protector are on
@@ -427,8 +454,13 @@ func (policy *Policy) AddProtector(protector *Protector) error {
 	// to it on the policy's filesystem.
 	if policy.Context.Mount != protector.Context.Mount {
 		log.Printf("policy on %s\n protector on %s\n", policy.Context.Mount, protector.Context.Mount)
+		ownerIfCreating, err := getOwnerOfMetadataForProtector(protector)
+		if err != nil {
+			return err
+		}
 		isNewLink, err := policy.Context.Mount.AddLinkedProtector(
-			protector.Descriptor(), protector.Context.Mount)
+			protector.Descriptor(), protector.Context.Mount,
+			protector.Context.TrustedUser, ownerIfCreating)
 		if err != nil {
 			return err
 		}
@@ -554,7 +586,7 @@ func (policy *Policy) CanBeAppliedWithoutProvisioning() bool {
 
 // commitData writes the Policy's current data to the filesystem.
 func (policy *Policy) commitData() error {
-	return policy.Context.Mount.AddPolicy(policy.data)
+	return policy.Context.Mount.AddPolicy(policy.data, policy.ownerIfCreating)
 }
 
 // findWrappedPolicyKey returns the index of the wrapped policy key
diff --git a/actions/policy_test.go b/actions/policy_test.go
index 07943b8..8248862 100644
--- a/actions/policy_test.go
+++ b/actions/policy_test.go
@@ -27,7 +27,7 @@ import (
 
 // Makes a protector and policy
 func makeBoth() (*Protector, *Policy, error) {
-	protector, err := CreateProtector(testContext, testProtectorName, goodCallback)
+	protector, err := CreateProtector(testContext, testProtectorName, goodCallback, nil)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -68,7 +68,7 @@ func TestPolicyGoodAddProtector(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -103,7 +103,7 @@ func TestPolicyGoodRemoveProtector(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -129,7 +129,7 @@ func TestPolicyBadRemoveProtector(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+	pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/actions/protector.go b/actions/protector.go
index 3278e63..b986eb0 100644
--- a/actions/protector.go
+++ b/actions/protector.go
@@ -109,17 +109,18 @@ func checkIfUserHasLoginProtector(ctx *Context, uid int64) error {
 // to unlock policies and create new polices. As with the key struct, a
 // Protector should be wiped after use.
 type Protector struct {
-	Context *Context
-	data    *metadata.ProtectorData
-	key     *crypto.Key
-	created bool
+	Context         *Context
+	data            *metadata.ProtectorData
+	key             *crypto.Key
+	created         bool
+	ownerIfCreating *user.User
 }
 
 // CreateProtector creates an unlocked protector with a given name (name only
 // needed for custom and raw protector types). The keyFn provided to create the
 // Protector key will only be called once. If an error is returned, no data has
 // been changed on the filesystem.
-func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, error) {
+func CreateProtector(ctx *Context, name string, keyFn KeyFunc, owner *user.User) (*Protector, error) {
 	if err := ctx.checkContext(); err != nil {
 		return nil, err
 	}
@@ -147,7 +148,8 @@ func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, erro
 			Name:   name,
 			Source: ctx.Config.Source,
 		},
-		created: true,
+		created:         true,
+		ownerIfCreating: owner,
 	}
 
 	// Extra data is needed for some SourceTypes
@@ -199,7 +201,7 @@ func GetProtector(ctx *Context, descriptor string) (*Protector, error) {
 	}
 
 	protector := &Protector{Context: ctx}
-	protector.data, err = ctx.Mount.GetRegularProtector(descriptor)
+	protector.data, err = ctx.Mount.GetRegularProtector(descriptor, ctx.TrustedUser)
 	return protector, err
 }
 
@@ -218,7 +220,7 @@ func GetProtectorFromOption(ctx *Context, option *ProtectorOption) (*Protector,
 
 	// Replace the context if this is a linked protector
 	if option.LinkedMount != nil {
-		ctx = &Context{ctx.Config, option.LinkedMount, ctx.TargetUser}
+		ctx = &Context{ctx.Config, option.LinkedMount, ctx.TargetUser, ctx.TrustedUser}
 	}
 	return &Protector{Context: ctx, data: option.data}, nil
 }
@@ -294,5 +296,5 @@ func (protector *Protector) Rewrap(keyFn KeyFunc) error {
 		return err
 	}
 
-	return protector.Context.Mount.AddProtector(protector.data)
+	return protector.Context.Mount.AddProtector(protector.data, protector.ownerIfCreating)
 }
diff --git a/actions/protector_test.go b/actions/protector_test.go
index 6c81d49..f20dbcf 100644
--- a/actions/protector_test.go
+++ b/actions/protector_test.go
@@ -43,7 +43,7 @@ func badCallback(info ProtectorInfo, retry bool) (*crypto.Key, error) {
 
 // Tests that we can create a valid protector.
 func TestCreateProtector(t *testing.T) {
-	p, err := CreateProtector(testContext, testProtectorName, goodCallback)
+	p, err := CreateProtector(testContext, testProtectorName, goodCallback, nil)
 	if err != nil {
 		t.Error(err)
 	} else {
@@ -54,7 +54,7 @@ func TestCreateProtector(t *testing.T) {
 
 // Tests that a failure in the callback is relayed back to the caller.
 func TestBadCallback(t *testing.T) {
-	p, err := CreateProtector(testContext, testProtectorName, badCallback)
+	p, err := CreateProtector(testContext, testProtectorName, badCallback, nil)
 	if err == nil {
 		p.Lock()
 		p.Destroy()
diff --git a/actions/recovery.go b/actions/recovery.go
index f533906..8a769cc 100644
--- a/actions/recovery.go
+++ b/actions/recovery.go
@@ -25,6 +25,7 @@ import (
 
 	"github.com/google/fscrypt/crypto"
 	"github.com/google/fscrypt/metadata"
+	"github.com/google/fscrypt/util"
 )
 
 // modifiedContextWithSource returns a copy of ctx with the protector source
@@ -66,7 +67,7 @@ func AddRecoveryPassphrase(policy *Policy, dirname string) (*crypto.Key, *Protec
 		if seq != 1 {
 			name += " (" + strconv.Itoa(seq) + ")"
 		}
-		recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn)
+		recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn, policy.ownerIfCreating)
 		if err == nil {
 			break
 		}
@@ -121,5 +122,10 @@ It is safe to keep it around though, as the recovery passphrase is high-entropy.
 	if _, err = file.WriteString(str); err != nil {
 		return err
 	}
+	if recoveryProtector.ownerIfCreating != nil {
+		if err = util.Chown(file, recoveryProtector.ownerIfCreating); err != nil {
+			return err
+		}
+	}
 	return file.Sync()
 }