[PATCH] sysfs: tightened sysfs permission checks
There were some inconsistency in restriction to VERIFY_OCTAL_PERMISSIONS().
Previously the test was "User perms >= group perms >= other perms". The
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. But logically WRITE is "more privileged" than READ.
Say for example, permission value is "0430". Here User has only READ
permission whereas Group has both WRITE and EXECUTE permission.
So, the checks could be tightened and the tests are separated to
USER_READABLE >= GROUP_READABLE >= OTHER_READABLE,
USER_WRITABLE >= GROUP_WRITABLE and OTHER_WRITABLE is not permitted.
Signed-off-by: Gobinda Charan Maji
---
include/linux/kernel.h | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3a5b48e..cd54b35 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -818,13 +818,15 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
#endif
/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
-#define VERIFY_OCTAL_PERMISSIONS(perms)
\
- (BUILD_BUG_ON_ZERO((perms) < 0) + \
-BUILD_BUG_ON_ZERO((perms) > 0777) +\
-/* User perms >= group perms >= other perms */ \
-BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \
-BUILD_BUG_ON_ZEROperms) >> 3) & 7) < ((perms) & 7)) + \
-/* Other writable? Generally considered a bad idea. */\
-BUILD_BUG_ON_ZERO((perms) & 2) + \
+#define VERIFY_OCTAL_PERMISSIONS(perms)
\
+ (BUILD_BUG_ON_ZERO((perms) < 0) +
\
+BUILD_BUG_ON_ZERO((perms) > 0777) +
\
+/* USER_READABLE >= GROUP_READABLE >= OTHER_READABLE */
\
+BUILD_BUG_ON_ZEROperms) >> 6) & 4) < (((perms) >> 3) & 4)) +
\
+BUILD_BUG_ON_ZEROperms) >> 3) & 4) < ((perms) & 4)) +
\
+/* USER_WRITABLE >= GROUP_WRITABLE */
\
+BUILD_BUG_ON_ZEROperms) >> 6) & 2) < (((perms) >> 3) & 2)) +
\
+/* OTHER_WRITABLE? Generally considered a bad idea. */
\
+BUILD_BUG_ON_ZERO((perms) & 2) +
\
(perms))
#endif
--
1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] sysfs: disallow world-writable files.
Rusty Russell 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 gmail.com>
> Cc: Dave Jones redhat.com>
> Cc: Joe Perches perches.com>
> Signed-off-by: Rusty Russell 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_ZEROperms) >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Stricter module param and sysfs permission checks
Robert Jarzmik free.fr> writes: As per the newly added restriction (User perms >= group perms >= other perms) is concerned, there is an inconsistency in the permission. Say for example, permission value is "0432". Here User has only READ permission whereas Group has both WRITE and EXECUTE permission and Other has WRITE permission. I think it is not good to give Group and Other at least 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. Please rethink about my point and let me know your opinion. Thanks, Gobinda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Stricter module param and sysfs permission checks
Gobinda Charan Maji gmail.com> writes: > > Hi All, > > I could not get any response yet. > > Hi Rusty, > > Please at least give me a reply even if my concept seems to be incorrect to > you. > > Thanks in advance, > Gobinda > > Hi All, I am new to this mail chain. I could not get any reply yet. May be I have posted my query at a wrong place. If this mail is visible to anyone of you please give me a reply and kindly suggest me the appropriate place to post my query. I'll be highly grateful you... Thanks in advance, Gobinda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Stricter module param and sysfs permission checks
Gobinda Charan Maji gmail.com> writes: > > Hi All, > > As per the newly added restriction (User perms >= group perms >= other > perms) is concerned, there is an inconsistency in the permission. Say for > example, permission value is "0432". Here User has only READ permission > whereas Group has both WRITE and EXECUTE permission and Other has WRITE > permission. I think it is not good to give Group and Other at least 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. Please rethink about my point and let me know your opinion. > > Thanks, > Gobinda > > Hi All, I could not get any response yet. Hi Rusty, Please at least give me a reply even if my concept seems to be incorrect to you. Thanks in advance, Gobinda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Stricter module param and sysfs permission checks
Robert Jarzmik free.fr> writes: > > Dave Jones redhat.com> writes: > > > On Thu, Mar 20, 2014 at 01:43:44PM +1030, Rusty Russell wrote: > > > > > drivers/mtd/devices/docg3.c: > > > __ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, dps0_insert_key), \ > > > __ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), \ > > > > > > drivers/scsi/pm8001/pm8001_ctl.c: > > > static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO, > > > pm8001_show_update_fw, pm8001_store_update_fw); > > > > Why on earth are these world writable ? > For docg3, this attributes are used to input a "password" into the flash chip, > to unlock parts of the flash memory. By unlock I mean that a sector read will > return the actual sector when unlocked, and only 0xff if not read unlocked. > > As to the "why writable" by "others", the legacy reason is that when I wrote > that code I had in mind that a casual user count : > - input the code : "echo secret > dps0_protection_key" > - mount /usermount > > That's not a good reason, I know, and changing that to remove the "other" write > permission is fine by me. > > Cheers. > Hi All, As per the newly added restriction (User perms >= group perms >= other perms) is concerned, there is an inconsistency in the permission. Say for example, permission value is "0432". Here User has only READ permission whereas Group has both WRITE and EXECUTE permission and Other has WRITE permission. I think it is not good to give Group and Other at least 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. Please rethink about my point and let me know your opinion. Thanks, Gobinda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
