Just a few little fixups for tmon, to prevent abuse of its logging facility
Best
Neil
The tmon logging system blindly opens its log file on a static path, making it
very easy for someone to redirect that log information to inappropriate places
or overwrite other users data. Do some easy checking to make sure we're not
logging to a symlink or a file owned by another user.
Signed-off-by: Neil Horman <[email protected]>
CC: Zhang Rui <[email protected]>
CC: Jacob Pan <[email protected]>
---
tools/thermal/tmon/tmon.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/tools/thermal/tmon/tmon.c b/tools/thermal/tmon/tmon.c
index b30f531..059e0be 100644
--- a/tools/thermal/tmon/tmon.c
+++ b/tools/thermal/tmon/tmon.c
@@ -142,6 +142,7 @@ static void start_syslog(void)
static void prepare_logging(void)
{
int i;
+ struct stat logstat;
if (!logging)
return;
@@ -152,6 +153,29 @@ static void prepare_logging(void)
return;
}
+ if (lstat(TMON_LOG_FILE, &logstat) < 0) {
+ syslog(LOG_ERR, "Unable to stat log file %s\n", TMON_LOG_FILE);
+ fclose(tmon_log);
+ tmon_log = NULL;
+ return;
+ }
+
+ /* The log file must be a regular file owned by us */
+ if (S_ISLNK(logstat.st_mode)) {
+ syslog(LOG_ERR, "Log file is a symlink. Will not log\n");
+ fclose(tmon_log);
+ tmon_log = NULL;
+ return;
+ }
+
+ if (logstat.st_uid != getuid()) {
+ syslog(LOG_ERR, "We don't own the log file. Not logging\n");
+ fclose(tmon_log);
+ tmon_log = NULL;
+ return;
+ }
+
+
fprintf(tmon_log, "#----------- THERMAL SYSTEM CONFIG -------------\n");
for (i = 0; i < ptdata.nr_tz_sensor; i++) {
char binding_str[33]; /* size of long + 1 */
--
1.8.3.1
current, the tmon umask value is set to 0, which mean whatever the permission
mask in the shell are when starting tmon in daemon mode are what the permissions
of any created files will be. We should likely set something more explicit, so
lets go with the usual 022
Signed-off-by: Neil Horman <[email protected]>
CC: Zhang Rui <[email protected]>
CC: Jacob Pan <[email protected]>
---
tools/thermal/tmon/tmon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/thermal/tmon/tmon.c b/tools/thermal/tmon/tmon.c
index 059e0be..09b7c32 100644
--- a/tools/thermal/tmon/tmon.c
+++ b/tools/thermal/tmon/tmon.c
@@ -355,7 +355,7 @@ static void start_daemon_mode()
disable_tui();
/* change the file mode mask */
- umask(0);
+ umask(S_IWGRP | S_IWOTH);
/* new SID for the daemon process */
sid = setsid();
--
1.8.3.1
On Tue, 17 Jun 2014 16:05:08 -0400
Neil Horman <[email protected]> wrote:
> The tmon logging system blindly opens its log file on a static path,
> making it very easy for someone to redirect that log information to
> inappropriate places or overwrite other users data. Do some easy
> checking to make sure we're not logging to a symlink or a file owned
> by another user.
>
Acked-by: Jacob Pan <[email protected]>
> Signed-off-by: Neil Horman <[email protected]>
> CC: Zhang Rui <[email protected]>
> CC: Jacob Pan <[email protected]>
> ---
> tools/thermal/tmon/tmon.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/tools/thermal/tmon/tmon.c b/tools/thermal/tmon/tmon.c
> index b30f531..059e0be 100644
> --- a/tools/thermal/tmon/tmon.c
> +++ b/tools/thermal/tmon/tmon.c
> @@ -142,6 +142,7 @@ static void start_syslog(void)
> static void prepare_logging(void)
> {
> int i;
> + struct stat logstat;
>
> if (!logging)
> return;
> @@ -152,6 +153,29 @@ static void prepare_logging(void)
> return;
> }
>
> + if (lstat(TMON_LOG_FILE, &logstat) < 0) {
> + syslog(LOG_ERR, "Unable to stat log file %s\n",
> TMON_LOG_FILE);
> + fclose(tmon_log);
> + tmon_log = NULL;
> + return;
> + }
> +
> + /* The log file must be a regular file owned by us */
> + if (S_ISLNK(logstat.st_mode)) {
> + syslog(LOG_ERR, "Log file is a symlink. Will not
> log\n");
> + fclose(tmon_log);
> + tmon_log = NULL;
> + return;
> + }
> +
> + if (logstat.st_uid != getuid()) {
> + syslog(LOG_ERR, "We don't own the log file. Not
> logging\n");
> + fclose(tmon_log);
> + tmon_log = NULL;
> + return;
> + }
> +
> +
> fprintf(tmon_log, "#----------- THERMAL SYSTEM CONFIG
> -------------\n"); for (i = 0; i < ptdata.nr_tz_sensor; i++) {
> char binding_str[33]; /* size of long + 1 */
[Jacob Pan]
--
Thanks,
Jacob
On Tue, 17 Jun 2014 16:05:09 -0400
Neil Horman <[email protected]> wrote:
> current, the tmon umask value is set to 0, which mean whatever the
Currently, ..., which means ...
> permission mask in the shell are when starting tmon in daemon mode
> are what the permissions of any created files will be. We should
> likely set something more explicit, so lets go with the usual 022
>
Just some grammar suggestion. Otherwise,
Acked-by: Jacob Pan <[email protected]>
> Signed-off-by: Neil Horman <[email protected]>
> CC: Zhang Rui <[email protected]>
> CC: Jacob Pan <[email protected]>
> ---
> tools/thermal/tmon/tmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/thermal/tmon/tmon.c b/tools/thermal/tmon/tmon.c
> index 059e0be..09b7c32 100644
> --- a/tools/thermal/tmon/tmon.c
> +++ b/tools/thermal/tmon/tmon.c
> @@ -355,7 +355,7 @@ static void start_daemon_mode()
> disable_tui();
>
> /* change the file mode mask */
> - umask(0);
> + umask(S_IWGRP | S_IWOTH);
>
> /* new SID for the daemon process */
> sid = setsid();
[Jacob Pan]
--
Thanks,
Jacob
On Tue, 17 Jun 2014 16:05:07 -0400
Neil Horman <[email protected]> wrote:
> Just a few little fixups for tmon, to prevent abuse of its logging
> facility
Looks good to me.
--
Thanks,
Jacob
On Tue, Jun 17, 2014 at 04:05:07PM -0400, Neil Horman wrote:
> Just a few little fixups for tmon, to prevent abuse of its logging facility
>
> Best
> Neil
>
Ping, Zhang, Jacob acked these, but I've not seen them go into your tree yet.
Do you have an issue with them?
Neil
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Tue, 2014-07-01 at 07:42 -0400, Neil Horman wrote:
> On Tue, Jun 17, 2014 at 04:05:07PM -0400, Neil Horman wrote:
> > Just a few little fixups for tmon, to prevent abuse of its logging facility
> >
> > Best
> > Neil
> >
> Ping, Zhang, Jacob acked these, but I've not seen them go into your tree yet.
> Do you have an issue with them?
>
Oh, next time when you send thermal related patches, please send them to
[email protected] mailing list so that I can review the patches
in https://patchwork.kernel.org/project/linux-pm/list/
Both patches applied. Thanks!
-rui
> Neil
>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
On Tue, Jul 01, 2014 at 09:49:25PM +0800, Zhang Rui wrote:
> On Tue, 2014-07-01 at 07:42 -0400, Neil Horman wrote:
> > On Tue, Jun 17, 2014 at 04:05:07PM -0400, Neil Horman wrote:
> > > Just a few little fixups for tmon, to prevent abuse of its logging facility
> > >
> > > Best
> > > Neil
> > >
> > Ping, Zhang, Jacob acked these, but I've not seen them go into your tree yet.
> > Do you have an issue with them?
> >
> Oh, next time when you send thermal related patches, please send them to
> [email protected] mailing list so that I can review the patches
> in https://patchwork.kernel.org/project/linux-pm/list/
>
> Both patches applied. Thanks!
>
Will do, thanks!
Nei
> -rui
> > Neil
> >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> > >
>
>
>