Merge pull request #346 from google/fixes

Metadata validation and other security improvements

README.md

@@ -37,6 +37,7 @@ dependencies](#runtime-dependencies).
 - [Building and installing](#building-and-installing)
 - [Runtime dependencies](#runtime-dependencies)
 - [Configuration file](#configuration-file)
+- [Setting up `fscrypt` on a filesystem](#setting-up-fscrypt-on-a-filesystem)
 - [Setting up for login protectors](#setting-up-for-login-protectors)
   - [Securing your login passphrase](#securing-your-login-passphrase)
   - [Enabling the PAM module](#enabling-the-pam-module)
@@ -319,7 +320,8 @@ that looks like the following:
 		"filenames": "AES_256_CTS",
 		"policy_version": "2"
 	},
-	"use_fs_keyring_for_v1_policies": false
+	"use_fs_keyring_for_v1_policies": false,
+	"allow_cross_user_metadata": false
 }
 ```
 
@@ -377,6 +379,54 @@ The fields are:
   kernels, it's better to not use this setting and instead (re-)create your
   encrypted directories with `"policy_version": "2"`.
 
+* "allow\_cross\_user\_metadata" specifies whether `fscrypt` will allow
+  protectors and policies from other non-root users to be read, e.g. to be
+  offered as options by `fscrypt encrypt`.  The default value is `false`, since
+  other users might be untrusted and could create malicious files.  This can be
+  set to `true` to restore the old behavior on systems where `fscrypt` metadata
+  needs to be shared between multiple users.  Note that this option is
+  independent from the permissions on the metadata files themselves, which are
+  set to 0600 by default; users who wish to share their metadata files with
+  other users would also need to explicitly change their mode to 0644.
+
+## Setting up `fscrypt` on a filesystem
+
+`fscrypt` needs some directories to exist on the filesystem on which encryption
+will be used:
+
+* `MOUNTPOINT/.fscrypt/policies`
+* `MOUNTPOINT/.fscrypt/protectors`
+
+(If login protectors are used, these must also exist on the root filesystem.)
+
+To create these directories, run `fscrypt setup MOUNTPOINT`.  If MOUNTPOINT is
+owned by root, as is usually the case, then this command will require root.
+
+There will be one decision you'll need to make: whether non-root users will be
+allowed to create `fscrypt` metadata (policies and protectors).
+
+If you say `y`, then these directories will be made world-writable, with the
+sticky bit set so that users can't delete each other's files -- just like
+`/tmp`.  If you say `N`, then these directories will be writable only by root.
+
+Saying `y` maximizes the usability of `fscrypt`, and on most systems it's fine
+to say `y`.  However, on some systems this may be inappropriate, as it will
+allow malicious users to fill the entire filesystem unless filesystem quotas
+have been configured -- similar to problems that have historically existed with
+other world-writable directories, e.g. `/tmp`.  If you are concerned about this,
+say `N`.  If you say `N`, then you'll only be able to run `fscrypt` as root to
+set up encryption on users' behalf, unless you manually set custom permissions
+on the metadata directories to grant write access to specific users or groups.
+
+If you chose the wrong mode at `fscrypt setup` time, you can change the
+directory permissions at any time.  To enable single-user writable mode, run:
+
+    sudo chmod 0755 MOUNTPOINT/.fscrypt/*
+
+To enable world-writable mode, run:
+
+    sudo chmod 1777 MOUNTPOINT/.fscrypt/*
+
 ## Setting up for login protectors
 
 If you want any encrypted directories to be protected by your login passphrase,
@@ -646,11 +696,15 @@ MOUNTPOINT  DEVICE     FILESYSTEM  ENCRYPTION   FSCRYPT
 Defaulting to policy_version 2 because kernel supports it.
 Customizing passphrase hashing difficulty for this system...
 Created global config file at "/etc/fscrypt.conf".
-Metadata directories created at "/.fscrypt".
+Allow users other than root to create fscrypt metadata on the root filesystem?
+(See https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem) [y/N] y
+Metadata directories created at "/.fscrypt", writable by everyone.
 
 # Start using fscrypt with our filesystem
->>>>> fscrypt setup /mnt/disk
-Metadata directories created at "/mnt/disk/.fscrypt".
+>>>>> sudo fscrypt setup /mnt/disk
+Allow users other than root to create fscrypt metadata on this filesystem? (See
+https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem) [y/N] y
+Metadata directories created at "/mnt/disk/.fscrypt", writable by everyone.
 
 # Initialize encryption on a new empty directory
 >>>>> mkdir /mnt/disk/dir1
@@ -678,8 +732,8 @@ POLICY                            UNLOCKED  PROTECTORS
 
 #### Quiet version
 ```bash
->>>>> sudo fscrypt setup --quiet --force
->>>>> fscrypt setup /mnt/disk --quiet
+>>>>> sudo fscrypt setup --quiet --force --all-users
+>>>>> sudo fscrypt setup /mnt/disk --quiet --all-users
 >>>>> echo "hunter2" | fscrypt encrypt /mnt/disk/dir1 --quiet --source=custom_passphrase  --name="Super Secret"
 ```
 

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
 	}

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.

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

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)
 	}

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)
 }

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()