PATCH 0/2 tmon: fix a few minor security issues

PATCH 0/2 tmon: fix a few minor security issues

[Neil Horman]

Just a few little fixups for tmon, to prevent abuse of its logging facility

Best
Neil

[PATCH 1/2] tmon: Check log file for common secuirty issues

[Neil Horman]

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 <nhorman@tuxdriver.com>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 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

Re: [PATCH 1/2] tmon: Check log file for common secuirty issues

[Jacob Pan]

On Tue, 17 Jun 2014 16:05:08 -0400
Neil Horman <nhorman@tuxdriver.com> 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 <jacob.jun.pan@linux.intel.com>

> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Zhang Rui <rui.zhang@intel.com>
> CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  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

[PATCH 2/2] tmon: set umask to a reasonable value

[Neil Horman]

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 <nhorman@tuxdriver.com>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 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

Re: [PATCH 2/2] tmon: set umask to a reasonable value

[Jacob Pan]

On Tue, 17 Jun 2014 16:05:09 -0400
Neil Horman <nhorman@tuxdriver.com> 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 <jacob.jun.pan@linux.intel.com>

> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Zhang Rui <rui.zhang@intel.com>
> CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  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

Re: PATCH 0/2 tmon: fix a few minor security issues

[Jacob Pan]

On Tue, 17 Jun 2014 16:05:07 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:

> Just a few little fixups for tmon, to prevent abuse of its logging
> facility
Looks good to me.

-- 
Thanks,

Jacob

Re: PATCH 0/2 tmon: fix a few minor security issues

[Neil Horman]

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 majordomo@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 0/2 tmon: fix a few minor security issues

[Zhang Rui]

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
linux-pm@vger.kernel.org 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 majordomo@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 0/2 tmon: fix a few minor security issues

[Neil Horman]

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
> linux-pm@vger.kernel.org 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 majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > > 
> 
> 
>