linux-kernel - Re: [PATCH 1/1] fanotify: check file flags passed in fanotify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5365E0A5.6040302@gmail.com>
Date:	Sun, 04 May 2014 08:39:33 +0200
From:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
To:	Heinrich Schuchardt <xypron.glpk@....de>,
	Eric Paris <eparis@...hat.com>
CC:	mtk.manpages@...il.com, linux-kernel@...r.kernel.org,
	Jan Kara <jack@...e.cz>, Al Viro <viro@...IV.linux.org.uk>
Subject: Re: [PATCH 1/1] fanotify: check file flags passed in fanotify_init

Hi Heinrich,

On 05/01/2014 05:35 PM, Heinrich Schuchardt wrote:
> Without this patch fanotify_init does not validate the value passed in
> event_f_flags.
> 
> When a fanotify event is read from the fanotify file descriptor a new file
> descriptor is created where file.f_flags = event_f_flags.
> 
> Internal and external open flags are stored together in field f_flags of
> struct file. Hence, an application might create file descriptors with
> internal flags like FMODE_EXEC, FMODE_NOCMTIME set.
> 
> With the patch the value of event_f_flags is checked. Only external open flags
> are allowed. When specifying an invalid value error EINVAL is returned.

Probably, with this patch, it's best to reference the past conversation 
on the topic, where Eric and Jan agree with you that there is a problem:

http://marc.info/?t=139739799900002&r=1&w=2

I agree with the idea of this patch, but the implementation needs work. 

For example, it seems to me very easy that someone would add a new 
open flag and fail to update FANOTIFY_INIT_ALL_EVENT_F_FLAGS, which
would result in some inconsistency that might not be found for a while.
But, that point may be mitigated by the following:

Do we need to allow all of the open flags, or just a subset? 
Certainly,it seems incorrect to allow file creation flags such 
as O_CREAT, which are meaningless in this context. (Quoting open(2),
the file creation flags are O_CLOEXEC, O_CREAT, O_DIRECTORY, O_EXCL,
O_NOCTTY, O_NOFOLLOW, O_TRUNC, and O_TTY_INIT. Furthermore, allowing
flags such as __O_TMPFILE, O_PATH, FASYNC, O_DIRECT, O_NDELAY seems
of dubious value. And I kind of wonder whether O_DIRECT is useful.
I'm unsure about O_SYNC, O_DSYNC, O_NOATIME, O_APPEND; perhaps 
there is a use case.

So, I suspect the allowed set of file status flags might be as small
as (or even smaller than):

     __O_SYNC | O_DSYNC | O_LARGEFILE | O_NOATIME | O_CLOEXEC

Finally, note that O_RDONLY, O_WRONLY, and O_RDWR are not bit masks
(see open(e)) so you can't just  OR those bits, See the definition of
O_ACCMODE in include/uapi/asm-generic/fcntl.h ; there's probably some 
suitable macro based on that constant that you can use.
Basically, you should I think be ANDing against O_ACCMODE
and texting whether the result of the AND matches O_RDONLY or matches 
O_WRONLY or matches O_RDWR.

Cheers,

Michael

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@....de>
> ---
>  fs/notify/fanotify/fanotify_user.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 4e565c8..3e456d7 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -25,6 +25,21 @@
>  #define FANOTIFY_DEFAULT_MAX_MARKS	8192
>  #define FANOTIFY_DEFAULT_MAX_LISTENERS	128
>  
> +/*
> + * All flags that may be specified in parameter event_f_flags of fanotify_init.
> + * 
> + * Internal and external open flags are stored together in field f_flags of
> + * struct file. Only external open flags shall be allowed in event_f_flags.
> + * Internal flags like FMODE_EXEC shall be excluded.
> + */
> +#define FANOTIFY_INIT_ALL_EVENT_F_FLAGS				( \
> +		O_RDONLY	| O_WRONLY	| O_RDWR	| \
> +		O_CREAT		| O_EXCL	| O_NOCTTY	| \
> +		O_TRUNC		| O_APPEND	| O_NONBLOCK	| \
> +		__O_SYNC	| O_DSYNC	| FASYNC	| \
> +		O_DIRECT	| O_LARGEFILE	| O_DIRECTORY	| \
> +		O_NOFOLLOW	| O_NOATIME	| O_CLOEXEC	| \
> +		O_PATH		| __O_TMPFILE	| O_NDELAY	)
>  extern const struct fsnotify_ops fanotify_fsnotify_ops;
>  
>  static struct kmem_cache *fanotify_mark_cache __read_mostly;
> @@ -669,6 +684,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  	if (flags & ~FAN_ALL_INIT_FLAGS)
>  		return -EINVAL;
>  
> +	if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_FLAGS)
> +		return -EINVAL;
> +
>  	user = get_current_user();
>  	if (atomic_read(&user->fanotify_listeners) > FANOTIFY_DEFAULT_MAX_LISTENERS) {
>  		free_uid(user);
> 

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/