[PATCH] libsdp: fix security issues with log file

[Amir Vadai]

When run as regular user, must make sure logs file
is not a symbolic link or with bad owner/permissions.

Signed-off-by: Amir Vadai <amirv at mellanox.co.il>
---
 libsdp.conf |   16 ++++++++++------
 src/log.c   |   34 ++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/libsdp.conf b/libsdp.conf
index 5b03e13..65f1f83 100644
--- a/libsdp.conf
+++ b/libsdp.conf
@@ -16,8 +16,8 @@
 # Please do not forget to comment if you want to change these.
 # (the rest of this file explains the syntax and give examples)
 #
-# Get errors printed into the files /tmp/libsdp.log.<uid>
-# or /var/log/<filename> for root
+# Get errors printed into the files /tmp/libsdp.log.<uid>/log
+# or /var/log/<path> for root
 log min-level 9 destination file libsdp.log
 #
 # By default we let all servers and client try SDP first.
@@ -32,14 +32,18 @@ use both client * *:*
 # ------------------
 # The log directive allows the user to specify which and where debug and error
 # messages get sent. The log statement format is:
-# log [destination stderr|syslog|file <filename>] [min-level <1-9>]
+# log [destination stderr|syslog|file <path>] [min-level <1-9>]
 # 
 # destination - defines the destination of the log messages:
 #   stderr - messages will be forwarded to the stderr 
 #   syslog - messages sent to the syslog service
-#   file <filename> - messages will be written to the file /var/log/<filename> for root.
-#   		      for regular user, if full path is requsted <filename with path>.<uid>
-#   		      or /tmp/<filename>.<uid> if no path is requested 
+#   file <path> - messages will be written to the file /var/log/<path> for root.
+#   		      for regular user, if full path is requsted <path>.<uid>/log
+#   		      or /tmp/<path>.<uid>/log if no path is requested 
+#   		      Due to security reasons, <path> must not be:
+#   		      1. a soft link
+#   		      2. owned by other user.
+#   		      3. Other permissions except User permissions.
 #
 # min-level - defines the verbosity of the log: 
 # 9 - only errors are printed
diff --git a/src/log.c b/src/log.c
index 9b62bc3..c98ee46 100644
--- a/src/log.c
+++ b/src/log.c
@@ -176,17 +176,43 @@ __sdp_log_set_log_file(
 	if (uid == 0) {
 		if ( p ) 
 			filename = p + 1;
-		snprintf( tfilename, sizeof( tfilename ), "/var/log/%s", filename );
+		snprintf( tfilename, sizeof(tfilename), "/var/log/%s", filename );
 	} else {
+		char tdir[PATH_MAX + 1];
 		/* 
 			for regular user, allow log file to be placed in a user
 			requested path. If no path is requested the log file is
 			placed in /tmp/
 		*/ 
 		if ( p ) 
-			snprintf( tfilename, sizeof( tfilename ), "%s.%d", filename, uid );
+			snprintf(tdir, sizeof(tdir), "%s.%d", filename, uid );
 		else
-			snprintf( tfilename, sizeof( tfilename ), "/tmp/%s.%d", filename, uid );
+			snprintf(tdir, sizeof(tdir ), "/tmp/%s.%d", filename, uid );
+
+		if (mkdir(tdir, 0700)) {
+			struct stat stat;
+
+			if (errno != EEXIST) {
+				__sdp_log( 9, "Couldn't create directory '%s' for logging (%m)\n", tdir );
+				return 0;
+			}
+
+			if (lstat(tdir, &stat)) {
+				__sdp_log(9, "Couldn't lstat directory %s\n", tdir);
+				return 0;
+			}
+
+			if (!S_ISDIR(stat.st_mode) || stat.st_uid != uid ||
+					(stat.st_mode & ~(S_IFMT | S_IRWXU))) {
+				__sdp_log( 9, "Cowardly refusing to log into directory:'%s'. " 
+					  "Make sure it is not: (1) link, (2) other uid, (3) bad permissions."
+					  "thus is a security issue.\n", tdir );
+				return 0;
+			}
+		}
+
+		snprintf(tfilename, sizeof(tfilename), "%s/log", tdir);
+		printf("dir: %s file: %s\n", tdir, tfilename);
 	}
 
 	/* double check the file is not a link */
@@ -199,7 +225,7 @@ __sdp_log_set_log_file(
 		
 	f = fopen( tfilename, "a" );
 	if ( !f ) {
-		__sdp_log( 9, "Couldn't open filename '%s' for logging\n", tfilename );
+		__sdp_log( 9, "Couldn't open '%s' for logging (%m)\n", tfilename );
 		return 0;
 	}
 
-- 
1.7.0.4


--6TrnltStXW4iwmi0--