[apparmor] [patch v3] tests: readdir - test both getdents() and getdents64() if available
Tyler Hicks
tyhicks at canonical.com
Wed Apr 5 23:59:46 UTC 2017
On 04/05/2017 06:48 PM, Steve Beattie wrote:
> On Wed, Apr 05, 2017 at 04:09:15PM -0500, Tyler Hicks wrote:
>>> +#if defined(SYS_getdents) && defined(SYS_getdents64)
>>> + if (rc != rc64) {
>>> + printf("FAIL - getdents and getdents64 returned different values: %d vs %d\n", rc, rc64);
>>
>> I feel like this should be ERROR instead of FAIL. If we use FAIL here,
>> the test will "pass" in an expected fail test case when getdents()
>> returns something different than getdents64(). I don't see a way around
>> that other than printing a different (unexpected) output than FAIL.
>>
>> swap.sh looks to be the only other test using ERROR. prologue.inc
>> doesn't know about ERROR and handles it by calling testerror().
>>
>> We're getting into extreme corner case territory here. As before, I'm
>> prepared to declare "good enough". Let me know your thoughts.
>
> Hrm, yeah, you're right on that. Though, in the read access not
> permitted case, we should be getting rejected on the open() call,
> not the getdents()/getdents64() calls.
>
> Anyway, we're probably into over-engineered testcase area here, but what
> follows is v3 of the patch.
>
> Changes since v2:
>
> - Convert to passing the expected return value to the readdir test, and
> only fail if something unexpected is generated.
> - Change the readdir.sh test to pass the expected return values,
> and expect a PASS result for all tests.
> - Add a testcase that confirms that granting write access to a
> directory does not allow reading the directory's contents.
> - Add a debug print function to readdir.c and use it in a few cases
> when DEBUG is defined at compile time.
>
> Subject: tests: readdir - test both getdents() and getdents64() if available
>
> In commit 3649, Colin King fixed the readdir test build issue where
> aarch64 only supports getdetns64(), not getdents(). Realistically,
> however, we want to ensure mediation occurs on both syscalls where
> they exist. This patch changes the test to attempt performing both
> versions of getdents(). Because we want to catch the situation where
> the result of getdents differs from getdents64, we now pass in the
> expected result.
>
> Also add a test to verify that having write access does not grant
> the ability to read a directory's contents.
>
> Bug: https://bugs.launchpad.net/bugs/1674245
>
> Signed-off-by: Steve Beattie <steve at nxnw.org>
Acked-by: Tyler Hicks <tyhicks at canonical.com>
Thanks for iterating on this so much. getdents/getdents64 mediation has
never been tested so good.
Tyler
> ---
> tests/regression/apparmor/readdir.c | 129 ++++++++++++++++++++++++++++-------
> tests/regression/apparmor/readdir.sh | 21 ++++-
> 2 files changed, 120 insertions(+), 30 deletions(-)
>
> Index: b/tests/regression/apparmor/readdir.c
> ===================================================================
> --- a/tests/regression/apparmor/readdir.c
> +++ b/tests/regression/apparmor/readdir.c
> @@ -1,14 +1,20 @@
> /*
> * Copyright (C) 2002-2006 Novell/SUSE
> + * Copyright (C) 2017 Canonical, Ltd.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> * published by the Free Software Foundation, version 2 of the
> * License.
> + *
> + * We attempt to test both getdents() and getdents64() here, but
> + * some architectures like aarch64 only support getdents64().
> */
> #define _GNU_SOURCE
>
> #include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> #include <unistd.h>
> #include <errno.h>
> #include <fcntl.h>
> @@ -18,45 +24,118 @@
> #include <string.h>
> #include <sys/syscall.h>
>
> -int main(int argc, char *argv[])
> +/* error if neither SYS_getdents or SYS_getdents64 is defined */
> +#if !defined(SYS_getdents) && !defined(SYS_getdents64)
> + #error "Neither SYS_getdents or SYS_getdents64 is defined, something has gone wrong!"
> +#endif
> +
> +/* define DEBUG to enable debugging statements i.e. gcc -DDEBUG */
> +#ifdef DEBUG
> +void pdebug(const char *format, ...)
> {
> - int fd;
> -#if defined(SYS_getdents64)
> - struct dirent64 dir;
> + va_list arg;
> +
> + va_start(arg, format);
> + vprintf(format, arg);
> + va_end(arg);
> +}
> #else
> - struct dirent dir;
> +void pdebug(const char *format, ...) { return; }
> #endif
>
> - if (argc != 2){
> - fprintf(stderr, "usage: %s dir\n",
> - argv[0]);
> - return 1;
> +#ifdef SYS_getdents
> +int my_readdir(char *dirname)
> +{
> + int fd;
> + struct dirent dir;
> +
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + pdebug("open failed: %s\n", strerror(errno));
> + return errno;
> }
>
> - fd = open(argv[1], O_RDONLY, 0);
> - if (fd == -1){
> - printf("FAIL - open %s\n", strerror(errno));
> - return 1;
> + /* getdents isn't exported by glibc, so must use syscall() */
> + if (syscall(SYS_getdents, fd, &dir, sizeof(dir)) == -1){
> + pdebug("getdents failed: %s\n", strerror(errno));
> + close(fd);
> + return errno;
> }
>
> - /*
> - if (fchdir(fd) == -1){
> - printf("FAIL - fchdir %s\n", strerror(errno));
> - return 1;
> + close(fd);
> + return 0;
> +}
> +#endif
> +
> +#ifdef SYS_getdents64
> +int my_readdir64(char *dirname)
> +{
> + int fd;
> + struct dirent64 dir;
> +
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + pdebug("open failed: %s\n", strerror(errno));
> + return errno;
> }
> - */
>
> /* getdents isn't exported by glibc, so must use syscall() */
> -#if defined(SYS_getdents64)
> - if (syscall(SYS_getdents64, fd, &dir, sizeof(struct dirent64)) == -1){
> -#else
> - if (syscall(SYS_getdents, fd, &dir, sizeof(struct dirent)) == -1){
> + if (syscall(SYS_getdents64, fd, &dir, sizeof(dir)) == -1){
> + pdebug("getdents64 failed: %s\n", strerror(errno));
> + close(fd);
> + return errno;
> + }
> +
> + close(fd);
> + return 0;
> +}
> #endif
> - printf("FAIL - getdents %s\n", strerror(errno));
> - return 1;
> +
> +int main(int argc, char *argv[])
> +{
> + int rc = 0, err = 0;
> + char *dirpath, *endptr;
> + int expected;
> +
> + if (argc != 3) {
> + fprintf(stderr, "usage: %s [dir] [expected retval]\n",
> + argv[0]);
> + err = 1;
> + goto err;
> }
>
> + dirpath = argv[1];
> +
> + errno = 0;
> + expected = (int) strtol(argv[2], &endptr, 10);
> + if (errno != 0 || endptr == argv[2]) {
> + fprintf(stderr, "ERROR: couldn't convert '%s' to an integer\n",
> + argv[2]);
> + err = 1;
> + goto err;
> + }
> + pdebug("expected = %d\n", expected);
> +
> +#ifdef SYS_getdents
> + rc = my_readdir(dirpath);
> + if (rc != expected) {
> + printf("FAIL - my_readdir returned %d, expected %d\n", rc, expected);
> + err = rc;
> + goto err;
> + }
> +#endif
> +
> +#ifdef SYS_getdents64
> + rc = my_readdir64(argv[1]);
> + if (rc != expected) {
> + printf("FAIL - my_readdir64 returned %d, expected %d\n", rc, expected);
> + err = rc;
> + goto err;
> + }
> +#endif
> +
> printf("PASS\n");
>
> - return 0;
> +err:
> + return err;
> }
> Index: b/tests/regression/apparmor/readdir.sh
> ===================================================================
> --- a/tests/regression/apparmor/readdir.sh
> +++ b/tests/regression/apparmor/readdir.sh
> @@ -1,5 +1,6 @@
> #! /bin/bash
> # Copyright (C) 2002-2005 Novell/SUSE
> +# Copyright (C) 2017 Canonical, Ltd.
> #
> # This program is free software; you can redistribute it and/or
> # modify it under the terms of the GNU General Public License as
> @@ -8,7 +9,7 @@
>
> #=NAME readdir
> #=DESCRIPTION
> -# AppArmor requires 'r' permission on a directory in order for a confined task
> +# AppArmor requires 'r' permission on a directory in order for a confined task
> # to be able to read the directory contents. This test verifies this.
> #=END
>
> @@ -26,20 +27,30 @@ badperm=ix
>
> mkdir $dir
>
> +# The readdir utility expects the return value to be passed as the
> +# second argument and returns success if the succeeding or failing calls
> +# match the expected value. It will fail the test if they don't, so for
> +# example the result differs acrorss getdents() and getdents64() this
> +# will be detected.
> +
> # READDIR TEST
> genprofile $dir/:$okperm
> -runchecktest "READDIR" pass $dir
> +runchecktest "READDIR" pass $dir 0
>
> # READDIR TEST (no perm)
> genprofile $dir/:$badperm
> -runchecktest "READDIR (no perm)" fail $dir
> +runchecktest "READDIR (no perm)" pass $dir 13
> +
> +# READDIR TEST (write perm) - ensure write perm isn't sufficient
> +genprofile $dir/:w
> +runchecktest "READDIR (write perm)" pass $dir 13
>
> # this test is to make sure the raw 'file' rule allows access
> # to directories
> genprofile file
> -runchecktest "READDIR 'file' dir" pass $dir
> +runchecktest "READDIR 'file' dir" pass $dir 0
>
> # this test is to make sure the raw 'file' rule allows access
> # to '/'
> genprofile file
> -runchecktest "READDIR 'file' '/'" pass '/'
> +runchecktest "READDIR 'file' '/'" pass '/' 0
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170405/0101f7f8/attachment-0001.pgp>
More information about the AppArmor
mailing list