All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@google.com>
Cc: fstests@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-f2fs@vger.kernel.org, "Theodore Y . Ts'o" <tytso@mit.edu>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	Richard Weinberger <richard@nod.at>,
	David Gstir <david@sigma-star.at>
Subject: Re: [PATCH 3/4] generic: test encrypted file access
Date: Mon, 21 Nov 2016 09:31:51 +1100	[thread overview]
Message-ID: <20161120223151.GK28177@dastard> (raw)
In-Reply-To: <1479412027-34416-4-git-send-email-ebiggers@google.com>

On Thu, Nov 17, 2016 at 11:47:06AM -0800, Eric Biggers wrote:
> Test accessing encrypted files with and without the encryption key.
> Access with the key is more of a sanity check, whereas access without
> the key should result in some particular behaviors.
> 
> As noted in the comment, as currently written this test is expected to
> fail on ext4 pre-4.8 and f2fs pre-4.6.  This could be avoided by using
> the filesystem-specific key prefix instead of the generic one, but I'd
> prefer to have the tests use the generic prefix.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
....
> +
> +cd $SCRATCH_MNT

Just noticed this, but it's in all the tests. We don't tend to
move into the test directories like this, because that means we
have to cd back out when we need to cycle a mount.

Just prepend things that are created with $SCRATCH_MNT/, or use
local variables for everything that hide this. e.g:

dirs="$SCRATCH_MNT/edir $SCRATCH_MNT/ref_dir"

mkdir $dirs
for dir in $dirs; do
....

> +
> +mkdir edir ref_dir
> +keydesc=$($FSCRYPT_UTIL gen_key)
> +$FSCRYPT_UTIL set_policy $keydesc edir > /dev/null
> +for dir in edir ref_dir; do
> +	touch $dir/empty > /dev/null
> +	$XFS_IO_PROG -t -f -c "pwrite 0 4k" $dir/a > /dev/null
> +	$XFS_IO_PROG -t -f -c "pwrite 0 33k" $dir/abcdefghijklmnopqrstuvwxyz > /dev/null
> +	maxname=$(yes | head -255 | tr -d '\n') # 255 character filename
> +	$XFS_IO_PROG -t -f -c "pwrite 0 1k" $dir/$maxname > /dev/null
> +	ln -s a $dir/symlink
> +	ln -s abcdefghijklmnopqrstuvwxyz $dir/symlink2
> +	ln -s $maxname $dir/symlink3
> +	mkdir $dir/subdir
> +	mkdir $dir/subdir/subsubdir
> +done
> +# Diff encrypted directory with unencrypted reference directory
> +diff -r edir ref_dir
> +# Cycle mount and diff again
> +cd $here
> +_scratch_cycle_mount
> +cd $SCRATCH_MNT
> +diff -r edir ref_dir
> +
> +# Now try accessing the files without the encryption key.
> +# It should still be possible to list the directory and remove files.
> +# But filenames should be encrypted, and it should not be possible to read
> +# regular files or to create new files or subdirectories.
> +cd $here
> +_scratch_unmount
> +$FSCRYPT_UTIL rm_key $keydesc
> +_scratch_mount
> +cd $SCRATCH_MNT
> +if [ $(ls edir | wc -l) -ne 8 ]; then
> +	echo "Wrong number of files"
> +	exit 1
> +fi

Number matching can done at the end of the test via the golden
image comparison. i.e. all you do here is:

ls edir | wc -l

And if the result is wrong then the test harness will catch it at
the end of the test.

But, then again, why wouldn't you just dump:

ls -lR edir |_filter_scratch

to the golden output file to confirm everything is exactly as you
expect it to be in the encrypted directory? It'll catch un-encrypted
names, wrong subdir depth, etc.


> +if [ -e edir/empty -o -e edir/symlink ]; then
> +	echo "Filenames were not encrypted!"
> +	exit 1
> +fi

stat edir/empty edir/symlink > /dev/null

And that will dump the error messages (No such file or directory)
to the golden output file. Again, test harness will catch
mismatches.

> +if [ $(find edir -mindepth 2 -maxdepth 2 -type d | wc -l) -ne 1 ]; then
> +	echo "Wrong number of subdirs"
> +	exit 1
> +fi

find edir -mindepth 2 -maxdepth 2 -type d | wc -l

> +cat $(find edir -maxdepth 1 -type f | head -1) 2>tmp
> +if ! egrep -q 'Required key not available' tmp; then
> +	echo "Reading encrypted file w/o key didn't fail with ENOKEY"
> +	cat tmp
> +	exit 1
> +fi

md5sum `find edir -maxdepth 1 -type f | head -1` | _filter_scratch

You'll either get a md5sum of the data or an error message
in the golden output. The wrong one will trigger a failure.

> +ls -l edir > /dev/null # should succeed

No necessary if you've already use ls -lR to validate all the above
files...

> +# There are some inconsistencies in which error codes are returned on different
> +# kernel versions and filesystems when trying to create a file or subdirectory
> +# without access to the parent directory's encryption key.  Furthermore, on some
> +# kernels correctly encoded filenames cause a different error (EACCES instead of
> +# ENOENT) because these names make it though ->lookup() and fail in ->create()
> +# or ->mkdir() instead.  For now we just accept multiple error codes.
> +
> +$XFS_IO_PROG -f edir/newfile &> tmp
> +if ! egrep -q 'Permission denied|No such file or directory' tmp; then
> +	echo "Creating file w/o key (unencoded) didn't fail with EACCES or ENOENT"
> +	cat tmp
> +	exit 1
> +fi

filter_enoent()
{
	sed -e 's/No such file or directory/Permission denied/'
}

filter_eperm()
{
	sed -e 's/Operation not permitted/Permission denied/'
}

$XFS_IO_PROG -f edir/newfile 2>&1 >/dev/null | _filter_enoent
mkdir edir/newfile  2>&1 >/dev/null | filter_enoent
$XFS_IO_PROG -f edir/0123456789abcdef 2>&1 >/dev/null | filter_eperm
mkdir edir/0123456789abcdef  2>&1 >/dev/null | filter_eperm

> +rm -r edir # should succeed
> +[ -e edir ] && echo "Directory wasn't deleted!"

ls edir

> +echo "Silence is golden."

No, not in this case. :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2016-11-20 22:32 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-17 19:47 [PATCH 0/4] Add filesystem-level encryption tests Eric Biggers
2016-11-17 19:47 ` [PATCH 1/4] generic: add utilities for testing filesystem encryption Eric Biggers
2016-11-20 21:33   ` Dave Chinner
2016-11-21 18:40     ` Eric Biggers
2016-11-21 21:08       ` Dave Chinner
2016-11-17 19:47 ` [PATCH 2/4] generic: test setting and getting encryption policies Eric Biggers
2016-11-20 22:07   ` Dave Chinner
2016-11-21 19:11     ` Eric Biggers
2016-11-21 21:21       ` Dave Chinner
2016-11-17 19:47 ` [PATCH 3/4] generic: test encrypted file access Eric Biggers
2016-11-20 22:31   ` Dave Chinner [this message]
2016-11-21 19:23     ` Eric Biggers
2016-11-21 21:23       ` Dave Chinner
2016-11-17 19:47 ` [PATCH 4/4] generic: test locking when setting encryption policy Eric Biggers
2016-11-20 22:35   ` Dave Chinner
2016-11-21 19:25     ` Eric Biggers
2016-11-21 21:32       ` Dave Chinner
2016-11-21 23:41         ` Eric Biggers
2016-11-24 23:26           ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161120223151.GK28177@dastard \
    --to=david@fromorbit.com \
    --cc=david@sigma-star.at \
    --cc=ebiggers@google.com \
    --cc=fstests@vger.kernel.org \
    --cc=jaegeuk@kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.