[linux-cifs-client] [PATCH 2/2] Continued cleanup of open_cred_file and fixed a potential security risk.

[Scott Lovenberg]

The parsing for values has been moved to its own function and is a bit cleaner, IMHO.
With the parsing de-coupled from the open_cred_file, this should work on a broader scope (e.g. - Jeff's multimount?).

Temporary buffers are zeroed out before being freed to ensure passwords/credentials aren't left in released memory.

Signed-off-by: Scott Lovenberg <scott.lovenberg at gmail.com>
---
 mount.cifs.c |  141 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 87 insertions(+), 54 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 97dae82..cf79bce 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -121,6 +121,14 @@
  */
 #define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV)
 
+/*
+ * Values for parsing a credentials file.
+ */
+#define CRED_UNPARSABLE 0
+#define CRED_USER 1
+#define CRED_PASS 2
+#define CRED_DOM 4
+
 /* struct for holding parsed mount info for use by privleged process */
 struct parsed_mount_info {
 	unsigned long flags;
@@ -521,44 +529,64 @@ static void null_terminate_endl(char* source)
 		*newline = '\0';
 }
 
-
+/*
+ * Parse a line from the credentials file.  Points 'target' to the first character
+ * after '=' on 'line' and returns the value type (CRED_) of the line.
+ * Returns CRED_UNPARSABLE on failure or if line is NULL.
+ */
+static int parse_cred_line(char* line, char* target)
+{
+	if (line == NULL)
+		goto parsing_err;
+
+	/* position target at first char of value */
+	target = strchr(line, '=');
+	if (!target)
+		goto parsing_err;
+	target++;
+	
+	if (strncasecmp("user", line, 4) == 0)
+		return CRED_USER;
+	if (strncasecmp("pass", line, 4) == 0)
+		return CRED_PASS;
+	if (strncasecmp("dom", line, 3) == 0)
+		return CRED_DOM;
+
+	/* if we're here the line, or target, wasn't sane */
+	parsing_err:
+		return CRED_UNPARSABLE;
+}
 
 static int open_cred_file(char *file_name,
 			  struct parsed_mount_info *parsed_info)
 {
 	char *line_buf;
-	char *temp_val;
+	char *temp_val = NULL;
 	FILE *fs = NULL;
 	int i;
 	const int line_buf_size = 4096;
 	const int min_non_white = 10;
 
-	i = toggle_dac_capability(0, 1);
-	if (i)
-		return i;
 
-	i = access(file_name, R_OK);
-	if (i) {
+	if ((i = toggle_dac_capability(0, 1)))
+		goto return_i;
+	if ((i = access(file_name, R_OK))) {
 		toggle_dac_capability(0, 0);
-		return i;
+		goto return_i;
 	}
 
-	fs = fopen(file_name, "r");
-	if (fs == NULL) {
+	if ((fs = fopen(file_name, "r")) == NULL) {
 		toggle_dac_capability(0, 0);
-		return errno;
+		i = errno;
+		goto return_i;
 	}
 
-	i = toggle_dac_capability(0, 0);
-	if (i) {
-		fclose(fs);
-		return i;
-	}
+	if((i = toggle_dac_capability(0, 0)))
+		goto return_i;
 
-	line_buf = (char *)malloc(line_buf_size);
-	if (line_buf == NULL) {
-		fclose(fs);
-		return EX_SYSERR;
+	if ((line_buf = (char*)malloc(line_buf_size)) == NULL) {
+		i = EX_SYSERR;
+		goto return_i;
 	}
 
 	/* parse line from credentials file */
@@ -570,47 +598,52 @@ static int open_cred_file(char *file_name,
 		}
 		null_terminate_endl(line_buf);
 
-		/* parse user */
-		if (strncasecmp("user", line_buf + i, 4) == 0) {
-			temp_val = strchr(line_buf + i, '=');
-			if (temp_val) {
-				/* go past equals sign */
-				temp_val++;
-				parsed_info->got_user = 1;
-				strlcpy(parsed_info->username, temp_val,
-					sizeof(parsed_info->username));
-			}
-		} 
-
-		/* parse password */
-		else if (strncasecmp("pass", line_buf + i, 4) == 0) {
-			temp_val = strchr(line_buf + i, '=');
-			if (!temp_val)
-				continue;
-			++temp_val;
-			i = set_password(parsed_info, temp_val);
-			if (i)
-				return i;
-		} 
-
-		/* parse domain */
-		else if (strncasecmp("dom", line_buf + i, 3) == 0) {
-			temp_val = strchr(line_buf + i, '=');
-			if (temp_val) {
-				/* go past equals sign */
-				temp_val++;
+		/* parse next token */
+		switch(parse_cred_line(line_buf + i, temp_val)) {
+			/* user */
+			case CRED_USER:
+				parse_username(temp_val, parsed_info);
+			break;
+
+			/* password */
+			case CRED_PASS:
+				if ((i = set_password(parsed_info, temp_val)))
+					goto return_i;
+			break;
+
+			/* domain */
+			case CRED_DOM:
 				if (parsed_info->verboseflag)
 					fprintf(stderr, "\nDomain %s\n",
 						temp_val);
 				strlcpy(parsed_info->domain, temp_val,
 					sizeof(parsed_info->domain));
-			}
-		}
+			break;
 
+			/* error */
+			case CRED_UNPARSABLE:
+				if (parsed_info->verboseflag)
+					fprintf(stderr, 
+					"Bad line in credentials file, ignoring");
+			break;
+				
+		}
 	}
-	fclose(fs);
-	SAFE_FREE(line_buf);
-	return 0;
+	/* if we've gotten to this line there are no error */
+	i = 0;
+
+	/* jump point for errors */
+	return_i:
+		if (fs != NULL)
+			fclose(fs);
+		/* make sure passwords are scrubbed from memory  */
+		if (line_buf != NULL)
+			memset(line_buf, 0, line_buf_size); 
+		if (temp_val != NULL)
+			memset(temp_val, 0, strlen(temp_val)); 
+		SAFE_FREE(line_buf);
+		SAFE_FREE(temp_val);
+		return i;
 }
 
 static int

--------------1.6.2.5--