Merge pull request #346 from google/fixes

Metadata validation and other security improvements

7 files changed, 148 insertions, 31 deletions

diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index 023c0fa..30aa3a7 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -63,7 +63,7 @@ var Setup = cli.Command{
 		the README). This may require root privileges.`,
 		mountpointArg, actions.ConfigFileLocation,
 		shortDisplay(timeTargetFlag)),
-	Flags:  []cli.Flag{timeTargetFlag, forceFlag},
+	Flags:  []cli.Flag{timeTargetFlag, forceFlag, allUsersSetupFlag},
 	Action: setupAction,
 }
 
@@ -468,7 +468,7 @@ var Lock = cli.Command{
 		recoverable by an attacker who compromises system memory. To be
 		fully safe, you must reboot with a power cycle.`,
 		directoryArg, shortDisplay(dropCachesFlag)),
-	Flags:  []cli.Flag{dropCachesFlag, userFlag, allUsersFlag},
+	Flags:  []cli.Flag{dropCachesFlag, userFlag, allUsersLockFlag},
 	Action: lockAction,
 }
 
@@ -502,7 +502,7 @@ func lockAction(c *cli.Context) error {
 		return newExitError(c, ErrDropCachesPerm)
 	}
 
-	if err = policy.Deprovision(allUsersFlag.Value); err != nil {
+	if err = policy.Deprovision(allUsersLockFlag.Value); err != nil {
 		switch err {
 		case keyring.ErrKeyNotPresent:
 			break
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index bcf5b59..2d76792 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -232,6 +232,10 @@ func getErrorSuggestions(err error) string {
 			}
 		}
 		return ""
+	case *filesystem.ErrNoCreatePermission:
+		return `For how to allow users to create fscrypt metadata on a
+			filesystem, refer to
+			https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem`
 	case *filesystem.ErrNotSetup:
 		return fmt.Sprintf(`Run "sudo fscrypt setup %s" to use fscrypt
 		        on this filesystem.`, e.Mount.Path)
diff --git a/cmd/fscrypt/flags.go b/cmd/fscrypt/flags.go
index 044b71e..1b41839 100644
--- a/cmd/fscrypt/flags.go
+++ b/cmd/fscrypt/flags.go
@@ -116,7 +116,8 @@ var (
 	allFlags = []prettyFlag{helpFlag, versionFlag, verboseFlag, quietFlag,
 		forceFlag, skipUnlockFlag, timeTargetFlag,
 		sourceFlag, nameFlag, keyFileFlag, protectorFlag,
-		unlockWithFlag, policyFlag, allUsersFlag, noRecoveryFlag}
+		unlockWithFlag, policyFlag, allUsersLockFlag, allUsersSetupFlag,
+		noRecoveryFlag}
 	// universalFlags contains flags that should be on every command
 	universalFlags = []cli.Flag{verboseFlag, quietFlag, helpFlag}
 )
@@ -164,7 +165,7 @@ var (
 			privileges.`,
 		Default: true,
 	}
-	allUsersFlag = &boolFlag{
+	allUsersLockFlag = &boolFlag{
 		Name: "all-users",
 		Usage: `Lock the directory no matter which user(s) have unlocked
 			it. Requires root privileges. This flag is only
@@ -172,6 +173,15 @@ var (
 			different from the one you're locking it as. This flag
 			is only implemented for v2 encryption policies.`,
 	}
+	allUsersSetupFlag = &boolFlag{
+		Name: "all-users",
+		Usage: `When setting up a filesystem for fscrypt, allow users
+			other than the calling user (typically root) to create
+			fscrypt policies and protectors on the filesystem. Note
+			that this will create a world-writable directory, which
+			users could use to fill up the entire filesystem. Hence,
+			this option may not be appropriate for some systems.`,
+	}
 	noRecoveryFlag = &boolFlag{
 		Name:  "no-recovery",
 		Usage: `Don't generate a recovery passphrase.`,
diff --git a/cmd/fscrypt/fscrypt_bash_completion b/cmd/fscrypt/fscrypt_bash_completion
index 00ee490..110d2d4 100644
--- a/cmd/fscrypt/fscrypt_bash_completion
+++ b/cmd/fscrypt/fscrypt_bash_completion
@@ -15,25 +15,71 @@
 # License for the specific language governing permissions and limitations under
 # the License.
 
-
-# Prefer completion script style COMPREPLY=($(...)) assignment.
+#
+# bash completion scripts require exercising some unusual shell script
+# features/quirks, so we have to disable some shellcheck warnings:
+#
+# Disable SC2016 ("Expressions don't expand in single quotes, use double quotes
+# for that") because the 'compgen' built-in expands the argument passed to -W,
+# so that argument *must* be single-quoted to avoid command injection.
+# shellcheck disable=SC2016
+#
+# Disable SC2034 ("{Variable} appears unused. Verify use (or export if used
+# externally)") because of the single quoting mentioned above as well as the
+# fact that we have to declare "local" variables used only by a called function
+# (_init_completion()) and not by the function itself.
+# shellcheck disable=SC2034
+#
+# Disable SC2207 ("Prefer mapfile or read -a to split command output (or quote
+# to avoid splitting)") because bash completion scripts conventionally use
+# COMPREPLY=($(...)) assignments.
 # shellcheck disable=SC2207
-true  # To apply shellcheck directive to all file
+#
+true  # To apply the above shellcheck directives to the entire file
 
 
-# Output list of possible mount points
-_fscrypt_mountpoints()
+# Generate the completion list for possible mountpoints.
+#
+# We need to be super careful here because mountpoints can contain whitespace
+# and shell meta-characters.  To avoid most problems, we do the following:
+#
+#   1.) To avoid parsing ambiguities, 'fscrypt status' replaces the space, tab,
+#       newline, and backslash characters with octal escape sequences -- like
+#       what /proc/self/mountinfo does.  To properly process its output, we need
+#       to split lines on space only (and not on other whitespace which might
+#       not be escaped), and unescape these characters.  Exception: we don't
+#       unescape newlines, as we need to reserve newline as the separator for
+#       the words passed to compgen.  (This causes mountpoints containing
+#       newlines to not be completed correctly, which we have to tolerate.)
+#
+#   2.) We backslash-escape all shell meta-characters, and single-quote the
+#       argument passed to compgen -W.  Without either step, command injection
+#       would be possible.  Without both steps, completions would be incorrect.
+#       The list of shell meta-characters used comes from that used by the
+#       completion script for umount, which has to solve this same problem.
+#
+_fscrypt_compgen_mountpoints()
 {
-    # shellcheck disable=SC2016
-    fscrypt status 2>/dev/null | \
-        command awk 'substr($0, 1, 1) == "/" && $5 == "Yes" { print $1 }'
+    local IFS=$'\n'
+    compgen -W '$(_fscrypt_mountpoints_internal)' -- "${cur}"
 }
 
+_fscrypt_mountpoints_internal()
+{
+    fscrypt status 2>/dev/null | command awk -F " " \
+    'substr($0, 1, 1) == "/" && $5 == "Yes" {
+        gsub(/\\040/, " ", $1)
+        gsub(/\\011/, "\t", $1)
+        gsub(/\\134/, "\\", $1)
+        gsub(/[\]\[(){}<>",:;^&!$=?`|'\''\\ \t\f\n\r\v]/, "\\\\&", $1)
+        print $1
+    }'
+}
 
 # Complete with all possible mountpoints
 _fscrypt_complete_mountpoint()
 {
-    COMPREPLY=($(compgen -W "$(_fscrypt_mountpoints)" -- "${cur}"))
+    COMPREPLY=($(_fscrypt_compgen_mountpoints))
 }
 
 
@@ -43,7 +89,6 @@ _fscrypt_complete_mountpoint()
 _fscrypt_status_section()
 {
     local section=${2^^}
-    # shellcheck disable=SC2016
     fscrypt status "$1" 2>/dev/null | \
         command awk '/^[[:xdigit:]]{16}/ && section == "'"$section"'" { print $1; next; }
                      { section = $1 }'
@@ -57,13 +102,13 @@ _fscrypt_complete_policy_or_protector()
     if [[ $cur = *:* ]]; then
         # Complete with IDs of the given mountpoint
         local mountpoint="${cur%:*}" id="${cur#*:}"
+        # Note: compgen expands the argument to -W, so it *must* be single-quoted.
         COMPREPLY=($(compgen \
-            -W "$(_fscrypt_status_section "${mountpoint}" "${status_section}")" \
+            -W '$(_fscrypt_status_section "${mountpoint}" "${status_section}")' \
             -- "${id}"))
     else
         # Complete with mountpoints, with colon and without ending space
-        COMPREPLY=($(compgen -W "$(_fscrypt_mountpoints)" \
-                     -- "${cur}" | sed s/\$/:/))
+        COMPREPLY=($(_fscrypt_compgen_mountpoints | sed s/\$/:/))
         compopt -o nospace
     fi
 }
@@ -72,7 +117,8 @@ _fscrypt_complete_policy_or_protector()
 # Complete with all arguments of that function
 _fscrypt_complete_word()
 {
-    COMPREPLY=($(compgen -W "$*" -- "${cur}"))
+    # Note: compgen expands the argument to -W, so it *must* be single-quoted.
+    COMPREPLY=($(compgen -W '$*' -- "${cur}"))
 }
 
 
@@ -82,7 +128,8 @@ _fscrypt_complete_option()
     local additional_opts=( "$@" )
     # Add global options, always correct
     additional_opts+=( --verbose --quiet --help )
-    COMPREPLY=($(compgen -W "${additional_opts[*]}" -- "${cur}"))
+    # Note: compgen expands the argument to -W, so it *must* be single-quoted.
+    COMPREPLY=($(compgen -W '${additional_opts[*]}' -- "${cur}"))
 }
 
 
@@ -95,7 +142,6 @@ _fscrypt()
     #
     # `split` is set by `_init_completion -s`, we must declare it local
     # even if we don't use it, not to modify the environment.
-    # shellcheck disable=SC2034
     local cur prev words cword split
     _init_completion -s -n : || return
 
diff --git a/cmd/fscrypt/protector.go b/cmd/fscrypt/protector.go
index ac864dd..186ca7a 100644
--- a/cmd/fscrypt/protector.go
+++ b/cmd/fscrypt/protector.go
@@ -23,6 +23,7 @@ package main
 import (
 	"fmt"
 	"log"
+	"os/user"
 
 	"github.com/google/fscrypt/actions"
 	"github.com/google/fscrypt/filesystem"
@@ -38,7 +39,6 @@ func createProtectorFromContext(ctx *actions.Context) (*actions.Protector, error
 		return nil, err
 	}
 	log.Printf("using source: %s", ctx.Config.Source.String())
-
 	if ctx.Config.Source == metadata.SourceType_pam_passphrase {
 		if userFlag.Value == "" && util.IsUserRoot() {
 			return nil, ErrSpecifyUser
@@ -70,7 +70,11 @@ IMPORTANT: Before continuing, ensure you have properly set up your system for
 		}
 	}
 
-	return actions.CreateProtector(ctx, name, createKeyFn)
+	var owner *user.User
+	if ctx.Config.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() {
+		owner = ctx.TargetUser
+	}
+	return actions.CreateProtector(ctx, name, createKeyFn, owner)
 }
 
 // selectExistingProtector returns a locked Protector which corresponds to an
diff --git a/cmd/fscrypt/setup.go b/cmd/fscrypt/setup.go
index 7b9bebb..b9a16e8 100644
--- a/cmd/fscrypt/setup.go
+++ b/cmd/fscrypt/setup.go
@@ -26,6 +26,7 @@ import (
 	"os"
 
 	"github.com/google/fscrypt/actions"
+	"github.com/google/fscrypt/filesystem"
 	"github.com/google/fscrypt/util"
 )
 
@@ -80,11 +81,47 @@ func setupFilesystem(w io.Writer, path string) error {
 	if err != nil {
 		return err
 	}
+	username := ctx.TargetUser.Username
 
-	if err = ctx.Mount.Setup(); err != nil {
+	err = ctx.Mount.CheckSetup(ctx.TrustedUser)
+	if err == nil {
+		return &filesystem.ErrAlreadySetup{Mount: ctx.Mount}
+	}
+	if _, ok := err.(*filesystem.ErrNotSetup); !ok {
 		return err
 	}
 
-	fmt.Fprintf(w, "Metadata directories created at %q.\n", ctx.Mount.BaseDir())
+	allUsers := allUsersSetupFlag.Value
+	if !allUsers {
+		thisFilesystem := "this filesystem"
+		if ctx.Mount.Path == "/" {
+			thisFilesystem = "the root filesystem"
+		}
+		prompt := fmt.Sprintf(`Allow users other than %s to create
+fscrypt metadata on %s? (See
+https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem)`,
+			username, thisFilesystem)
+		allUsers, err = askQuestion(wrapText(prompt, 0), false)
+		if err != nil {
+			return err
+		}
+	}
+	var setupMode filesystem.SetupMode
+	if allUsers {
+		setupMode = filesystem.WorldWritable
+	} else {
+		setupMode = filesystem.SingleUserWritable
+	}
+	if err = ctx.Mount.Setup(setupMode); err != nil {
+		return err
+	}
+
+	if allUsers {
+		fmt.Fprintf(w, "Metadata directories created at %q, writable by everyone.\n",
+			ctx.Mount.BaseDir())
+	} else {
+		fmt.Fprintf(w, "Metadata directories created at %q, writable by %s only.\n",
+			ctx.Mount.BaseDir(), username)
+	}
 	return nil
 }
diff --git a/cmd/fscrypt/status.go b/cmd/fscrypt/status.go
index 255bb2b..bc8f1ee 100644
--- a/cmd/fscrypt/status.go
+++ b/cmd/fscrypt/status.go
@@ -101,7 +101,7 @@ func writeGlobalStatus(w io.Writer) error {
 	t := makeTableWriter(w, "MOUNTPOINT\tDEVICE\tFILESYSTEM\tENCRYPTION\tFSCRYPT")
 	for _, mount := range mounts {
 		// Only print mountpoints backed by devices or using fscrypt.
-		usingFscrypt := mount.CheckSetup() == nil
+		usingFscrypt := mount.CheckSetup(nil) == nil
 		if !usingFscrypt && mount.Device == "" {
 			continue
 		}
@@ -114,8 +114,11 @@ func writeGlobalStatus(w io.Writer) error {
 			continue
 		}
 
-		fmt.Fprintf(t, "%s\t%s\t%s\t%s\t%s\n", mount.Path, mount.Device,
-			mount.FilesystemType, supportString, yesNoString(usingFscrypt))
+		fmt.Fprintf(t, "%s\t%s\t%s\t%s\t%s\n",
+			filesystem.EscapeString(mount.Path),
+			filesystem.EscapeString(mount.Device),
+			filesystem.EscapeString(mount.FilesystemType),
+			supportString, yesNoString(usingFscrypt))
 
 		if supportErr == nil {
 			supportCount++
@@ -157,14 +160,27 @@ func writeFilesystemStatus(w io.Writer, ctx *actions.Context) error {
 		return err
 	}
 
-	policyDescriptors, err := ctx.Mount.ListPolicies()
+	policyDescriptors, err := ctx.Mount.ListPolicies(ctx.TrustedUser)
 	if err != nil {
 		return err
 	}
 
-	fmt.Fprintf(w, "%s filesystem %q has %s and %s\n\n", ctx.Mount.FilesystemType,
+	filterDescription := ""
+	if ctx.TrustedUser != nil {
+		filterDescription = fmt.Sprintf(" (only including ones owned by %s or root)", ctx.TrustedUser.Username)
+	}
+	fmt.Fprintf(w, "%s filesystem %q has %s and %s%s.\n", ctx.Mount.FilesystemType,
 		ctx.Mount.Path, pluralize(len(options), "protector"),
-		pluralize(len(policyDescriptors), "policy"))
+		pluralize(len(policyDescriptors), "policy"), filterDescription)
+	if setupMode, user, err := ctx.Mount.GetSetupMode(); err == nil {
+		switch setupMode {
+		case filesystem.WorldWritable:
+			fmt.Fprintf(w, "All users can create fscrypt metadata on this filesystem.\n")
+		case filesystem.SingleUserWritable:
+			fmt.Fprintf(w, "Only %s can create fscrypt metadata on this filesystem.\n", user.Username)
+		}
+	}
+	fmt.Fprintf(w, "\n")
 
 	if len(options) > 0 {
 		writeOptions(w, options)