[PATCH 9/9] sysfs: disallow world-writable files.

[PATCH 9/9] sysfs: disallow world-writable files.

[Rusty Russell]

This check was introduced in 2006 by Alexey Dobriyan (9774a1f54f173)
for module parameters; we removed it when we unified the check into
VERIFY_OCTAL_PERMISSIONS() as sysfs didn't have the same requirement.
Now all those users are fixed, reintroduce it.

Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/kernel.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c52907a6d8b..43e1c6a9683e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -849,5 +849,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	 /* User perms >= group perms >= other perms */			\
 	 BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
 	 BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) +	\
+	 /* Other writable?  Generally considered a bad idea. */	\
+	 BUILD_BUG_ON_ZERO((perms) & 2) +				\
 	 (perms))
 #endif
-- 
1.8.3.2

Re: [PATCH 9/9] sysfs: disallow world-writable files.

[Gobinda Maji]

Hi Rusty,

I have a small doubt about the permission restriction (User perms >=
group perms >= other perms) in VERIFY_OCTAL_PERMISSIONS(). Please Note
that permission field of User, Group or Other consists of three bits.
LSB is EXECUTE permission, MSB is READ permission and the middle bit
is WRITE permission. Say for example, permission value is "0431". Here
User has only READ permission whereas Group has both WRITE and EXECUTE
permission and Other has EXECUTE permission. I guess, it is not good
to give Group the WRITE permission whereas User itself has no WRITE
permission.
May be, it's better to check those three permissions bit wise rather than
as a whole.
I already had posted my query at:

http://thread.gmane.org/gmane.linux.kernel/1667095/focus=1737833

But could get any reply.

Please rethink about my point and let me know your opinion.

Thanks,
Gobinda

Re: [PATCH 9/9] sysfs: disallow world-writable files.

[Gobinda Charan Maji]

Rusty Russell <rusty <at> rustcorp.com.au> writes:

> 
> This check was introduced in 2006 by Alexey Dobriyan (9774a1f54f173)
> for module parameters; we removed it when we unified the check into
> VERIFY_OCTAL_PERMISSIONS() as sysfs didn't have the same requirement.
> Now all those users are fixed, reintroduce it.
> 
> Cc: Alexey Dobriyan <adobriyan <at> gmail.com>
> Cc: Dave Jones <davej <at> redhat.com>
> Cc: Joe Perches <joe <at> perches.com>
> Signed-off-by: Rusty Russell <rusty <at> rustcorp.com.au>
> ---
>  include/linux/kernel.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c52907a6d8b..43e1c6a9683e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
>  <at>  <at>  -849,5 +849,7  <at>  <at>  static inline void
ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  	 /* User perms >= group perms >= other perms */			\
>  	 BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
>  	 BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) +	\
> +	 /* Other writable?  Generally considered a bad idea. */	\
> +	 BUILD_BUG_ON_ZERO((perms) & 2) +				\
>  	 (perms))
>  #endif

Hi Rusty,

I have a small doubt about the permission restriction (User perms >= group
perms >= other perms) in VERIFY_OCTAL_PERMISSIONS(). Please Note that
permission field of User, Group or Other consists of three bits. LSB is
EXECUTE permission, MSB is READ permission and the middle bit is WRITE
permission. Say for example, permission value is "0431". Here User has only
READ permission whereas Group has both WRITE and EXECUTE permission and
Other has EXECUTE permission. I guess, it is not good to give Group the
WRITE permission whereas User itself has no WRITE permission.
May be, it's better to check those three permissions bit wise rather than 
as a whole.
I already had posted my query at:

http://thread.gmane.org/gmane.linux.kernel/1667095/focus=1737833

But could get any reply.

Please rethink about my point and let me know your opinion.

Thanks,
Gobinda

Re: [PATCH 9/9] sysfs: disallow world-writable files.

[Rusty Russell]

Gobinda Maji <gobinda.cemk07@gmail.com> writes:
> Hi Rusty,

Hi Gobinda,

> I have a small doubt about the permission restriction (User perms >=
> group perms >= other perms) in VERIFY_OCTAL_PERMISSIONS(). Please Note
> that permission field of User, Group or Other consists of three bits.
> LSB is EXECUTE permission, MSB is READ permission and the middle bit
> is WRITE permission. Say for example, permission value is "0431". Here
> User has only READ permission whereas Group has both WRITE and EXECUTE
> permission and Other has EXECUTE permission. I guess, it is not good
> to give Group the WRITE permission whereas User itself has no WRITE
> permission.

You're absolutely right, well spotted!  The checks can be tightened.  We
don't really care about execute, but logically write is "more
privileged" than read.

Best to separate the tests; OTHER_WRITABLE <= GROUP_WRITABLE <= OWNER_WRITABLE
and OTHER_READABLE <= GROUP_READABLE <= OWNER_READABLE.

A patch would be welcome!

Thanks,
Rusty.

Re: [PATCH 9/9] sysfs: disallow world-writable files.

[Gobinda Maji]

On 30 April 2015 at 07:32, Rusty Russell <rusty@rustcorp.com.au> wrote:

> You're absolutely right, well spotted!  The checks can be tightened.  We
> don't really care about execute, but logically write is "more
> privileged" than read.
>
> Best to separate the tests; OTHER_WRITABLE <= GROUP_WRITABLE <= OWNER_WRITABLE
> and OTHER_READABLE <= GROUP_READABLE <= OWNER_READABLE.
>
> A patch would be welcome!

Thanks for the suggestion. OTHER_WRITABLE is already not permitted.
So, added the checks for GROUP_WRITABLE <= OWNER_WRITABLE for write
and OTHER_READABLE <= GROUP_READABLE <= OWNER_READABLE for read.

I am just sending a separate patch for this. The subject line will be
"[PATCH] sysfs: tightened sysfs permission checks"

-- 
Thanks,
Gobinda