Patchwork Clean up open_cred_file()

login
register
mail settings
Submitter Scott Lovenberg
Date April 18, 2010, 7:57 a.m.
Message ID <j2vb1fc6a561004180057y3b277826z87eac4e1cee01c02@mail.gmail.com>
Download mbox | patch
Permalink /patch/50390/
State New
Headers show

Comments

Scott Lovenberg - April 18, 2010, 7:57 a.m.
This patch cleans up the open_cred_file() function in mount.cifs.c.  It also
zeros out the password buffer.
I made the buffer length a static int since macro'ing it seemed like leaky
abstraction (it only makes sense within the scope of the function, IMHO).
Is this acceptable?

P.S. - I'm still getting used to git, please let me know if this patch is
formatted incorrectly.
Jeff Layton - April 18, 2010, 11:26 a.m.
On Sun, 18 Apr 2010 03:57:31 -0400
Scott Lovenberg <scott.lovenberg@gmail.com> wrote:

> diff --git a/mount.cifs.c b/mount.cifs.c
> index 1aa3329..54feeaf 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -98,7 +98,7 @@
>  #endif
>  
>  #define MOUNT_PASSWD_SIZE 128
> -#define DOMAIN_SIZE 64
> +#define MAX_DOMAIN_SIZE 64
>  
>  /*
>   * value of the ver= option that gets passed to the kernel. Used to indicate
> @@ -128,7 +128,7 @@ struct parsed_mount_info {
>  	char share[MAX_SHARE_LEN + 1];
>  	char prefix[PATH_MAX + 1];
>  	char options[MAX_OPTIONS_LEN];
> -	char domain[DOMAIN_SIZE + 1];
> +	char domain[MAX_DOMAIN_SIZE + 1];
>  	char username[MAX_USERNAME_SIZE + 1];
>  	char password[MOUNT_PASSWD_SIZE + 1];
>  	char addrlist[MAX_ADDR_LIST_LEN];
> @@ -511,14 +511,26 @@ toggle_dac_capability(int writable, int enable)
>  #endif /* HAVE_LIBCAP */
>  #endif /* HAVE_LIBCAP_NG */
>  
> +/*
> + * Null terminates string at first '\n'.
> + */
> +static void null_terminate_endl(char* source)
> +{
> +	char* newline = strchr(source, '\n');
> +	if (newline)
> +		*newline = '\0';
> +}
> +
>  static int open_cred_file(char *file_name,
>  			  struct parsed_mount_info *parsed_info)
>  {
>  	char *line_buf;
> -	char *temp_val, *newline;
> +	char *temp_val;
>  	FILE *fs = NULL;
> -	int i, length;
> -
> +	int i;
> +	const int line_buf_size = 4096;
> +	const int min_non_white = 10;
> +	
>  	i = toggle_dac_capability(0, 1);
>  	if (i)
>  		return i;
> @@ -541,58 +553,54 @@ static int open_cred_file(char *file_name,
>  		return i;
>  	}
>  
> -	line_buf = (char *)malloc(4096);
> +	line_buf = (char *)malloc(line_buf_size);
>  	if (line_buf == NULL) {
>  		fclose(fs);
>  		return EX_SYSERR;
>  	}
>  
> -	while (fgets(line_buf, 4096, fs)) {
> -		/* parse line from credential file */
> -
> +	/* parse line from credential file*/
> +	while (fgets(line_buf, line_buf_size, fs)) {
>  		/* eat leading white space */
> -		for (i = 0; i < 4086; i++) {
> +		for (i = 0; i < line_buf_size - min_non_white + 1; i++) {
>  			if ((line_buf[i] != ' ') && (line_buf[i] != '\t'))
>  				break;
> -			/* if whitespace - skip past it */
>  		}
>  
> -		/* NULL terminate at newline */
> -		newline = strchr(line_buf + i, '\n');
> -		if (newline)
> -			*newline = '\0';
> -
> +		/* parse user */
> +		null_terminate_endl(line_buf);
>  		if (strncasecmp("user", line_buf + i, 4) == 0) {
>  			temp_val = strchr(line_buf + i, '=');
>  			if (temp_val) {
>  				/* go past equals sign */
>  				temp_val++;
> -				for (length = 0; length < 4087; length++) {
> -					if ((temp_val[length] == '\n')
> -					    || (temp_val[length] == '\0')) {
> -						temp_val[length] = '\0';
> -						break;
> -					}
> -				}
> -				if (length > 4086) {
> +				null_terminate_endl(temp_val);

				^^^ is this needed? You've already
				wiped out the newline in the earlier
				call to this.

> +				if (strlen(temp_val) > MAX_USERNAME_SIZE) {
					^^^^^^
			Wonder if we really need this check?
			parse_username shouldn't allow it to overflow
			the buffer. It won't return an error -- it'll
			just put what it can in the buffer, but
			presumably the kernel would reject the
			malformed username on the mount attempt.

>  					fprintf(stderr,
>  						"mount.cifs failed due to malformed username in credentials file\n");
> -					memset(line_buf, 0, 4096);
> +					memset(line_buf, 0, line_buf_size);
>  					return EX_USAGE;
>  				}
> -				parsed_info->got_user = 1;
> -				strlcpy(parsed_info->username, temp_val,
> -					sizeof(parsed_info->username));
> +				parse_username(temp_val, parsed_info);

	You should probably also zero out the buffer here in case the
	user= field held a password too.

>  			}
> -		} else if (strncasecmp("pass", line_buf + i, 4) == 0) {
> +		}
> +		
> +		/* parse password */ 
> +		else if (strncasecmp("pass", line_buf + i, 4) == 0) {
>  			temp_val = strchr(line_buf + i, '=');
>  			if (!temp_val)
>  				continue;
> -			++temp_val;
> +                        /* go past equals sign */
> +			temp_val++;
>  			i = set_password(parsed_info, temp_val);
> +			/* zero out password from buffer */
> +			memset(line_buf, 0, line_buf_size);
>  			if (i)
>  				return i;
> -		} else if (strncasecmp("dom", line_buf + i, 3) == 0) {
> +		}
> +		
> +		/* parse domain */ 
> +		else if (strncasecmp("dom", line_buf + i, 3) == 0) {
>  			temp_val = strchr(line_buf + i, '=');
>  			if (temp_val) {
>  				/* go past equals sign */
> @@ -601,16 +609,8 @@ static int open_cred_file(char *file_name,
>  					fprintf(stderr, "\nDomain %s\n",
>  						temp_val);
>  
> -				for (length = 0; length < DOMAIN_SIZE + 1;
> -				     length++) {
> -					if ((temp_val[length] == '\n')
> -					    || (temp_val[length] == '\0')) {
> -						temp_val[length] = '\0';
> -						break;
> -					}
> -				}
> -
> -				if (length > DOMAIN_SIZE) {
> +				null_terminate_endl(temp_val);
				^^^ I don't think this is needed.
> +				if (strlen(temp_val) > MAX_DOMAIN_SIZE) {
>  					fprintf(stderr,
>  						"mount.cifs failed: domain in credentials file too long\n");
>  					return EX_USAGE;
> @@ -620,7 +620,6 @@ static int open_cred_file(char *file_name,
>  					sizeof(parsed_info->domain));
>  			}
>  		}
> -
>  	}
>  	fclose(fs);
>  	SAFE_FREE(line_buf);
> 

Other than the stuff above, the patch looks reasonable.

Patch

diff --git a/mount.cifs.c b/mount.cifs.c
index 1aa3329..54feeaf 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -98,7 +98,7 @@ 
 #endif
 
 #define MOUNT_PASSWD_SIZE 128
-#define DOMAIN_SIZE 64
+#define MAX_DOMAIN_SIZE 64
 
 /*
  * value of the ver= option that gets passed to the kernel. Used to indicate
@@ -128,7 +128,7 @@  struct parsed_mount_info {
 	char share[MAX_SHARE_LEN + 1];
 	char prefix[PATH_MAX + 1];
 	char options[MAX_OPTIONS_LEN];
-	char domain[DOMAIN_SIZE + 1];
+	char domain[MAX_DOMAIN_SIZE + 1];
 	char username[MAX_USERNAME_SIZE + 1];
 	char password[MOUNT_PASSWD_SIZE + 1];
 	char addrlist[MAX_ADDR_LIST_LEN];
@@ -511,14 +511,26 @@  toggle_dac_capability(int writable, int enable)
 #endif /* HAVE_LIBCAP */
 #endif /* HAVE_LIBCAP_NG */
 
+/*
+ * Null terminates string at first '\n'.
+ */
+static void null_terminate_endl(char* source)
+{
+	char* newline = strchr(source, '\n');
+	if (newline)
+		*newline = '\0';
+}
+
 static int open_cred_file(char *file_name,
 			  struct parsed_mount_info *parsed_info)
 {
 	char *line_buf;
-	char *temp_val, *newline;
+	char *temp_val;
 	FILE *fs = NULL;
-	int i, length;
-
+	int i;
+	const int line_buf_size = 4096;
+	const int min_non_white = 10;
+	
 	i = toggle_dac_capability(0, 1);
 	if (i)
 		return i;
@@ -541,58 +553,54 @@  static int open_cred_file(char *file_name,
 		return i;
 	}
 
-	line_buf = (char *)malloc(4096);
+	line_buf = (char *)malloc(line_buf_size);
 	if (line_buf == NULL) {
 		fclose(fs);
 		return EX_SYSERR;
 	}
 
-	while (fgets(line_buf, 4096, fs)) {
-		/* parse line from credential file */
-
+	/* parse line from credential file*/
+	while (fgets(line_buf, line_buf_size, fs)) {
 		/* eat leading white space */
-		for (i = 0; i < 4086; i++) {
+		for (i = 0; i < line_buf_size - min_non_white + 1; i++) {
 			if ((line_buf[i] != ' ') && (line_buf[i] != '\t'))
 				break;
-			/* if whitespace - skip past it */
 		}
 
-		/* NULL terminate at newline */
-		newline = strchr(line_buf + i, '\n');
-		if (newline)
-			*newline = '\0';
-
+		/* parse user */
+		null_terminate_endl(line_buf);
 		if (strncasecmp("user", line_buf + i, 4) == 0) {
 			temp_val = strchr(line_buf + i, '=');
 			if (temp_val) {
 				/* go past equals sign */
 				temp_val++;
-				for (length = 0; length < 4087; length++) {
-					if ((temp_val[length] == '\n')
-					    || (temp_val[length] == '\0')) {
-						temp_val[length] = '\0';
-						break;
-					}
-				}
-				if (length > 4086) {
+				null_terminate_endl(temp_val);
+				if (strlen(temp_val) > MAX_USERNAME_SIZE) {
 					fprintf(stderr,
 						"mount.cifs failed due to malformed username in credentials file\n");
-					memset(line_buf, 0, 4096);
+					memset(line_buf, 0, line_buf_size);
 					return EX_USAGE;
 				}
-				parsed_info->got_user = 1;
-				strlcpy(parsed_info->username, temp_val,
-					sizeof(parsed_info->username));
+				parse_username(temp_val, parsed_info);
 			}
-		} else if (strncasecmp("pass", line_buf + i, 4) == 0) {
+		}
+		
+		/* parse password */ 
+		else if (strncasecmp("pass", line_buf + i, 4) == 0) {
 			temp_val = strchr(line_buf + i, '=');
 			if (!temp_val)
 				continue;
-			++temp_val;
+                        /* go past equals sign */
+			temp_val++;
 			i = set_password(parsed_info, temp_val);
+			/* zero out password from buffer */
+			memset(line_buf, 0, line_buf_size);
 			if (i)
 				return i;
-		} else if (strncasecmp("dom", line_buf + i, 3) == 0) {
+		}
+		
+		/* parse domain */ 
+		else if (strncasecmp("dom", line_buf + i, 3) == 0) {
 			temp_val = strchr(line_buf + i, '=');
 			if (temp_val) {
 				/* go past equals sign */
@@ -601,16 +609,8 @@  static int open_cred_file(char *file_name,
 					fprintf(stderr, "\nDomain %s\n",
 						temp_val);
 
-				for (length = 0; length < DOMAIN_SIZE + 1;
-				     length++) {
-					if ((temp_val[length] == '\n')
-					    || (temp_val[length] == '\0')) {
-						temp_val[length] = '\0';
-						break;
-					}
-				}
-
-				if (length > DOMAIN_SIZE) {
+				null_terminate_endl(temp_val);
+				if (strlen(temp_val) > MAX_DOMAIN_SIZE) {
 					fprintf(stderr,
 						"mount.cifs failed: domain in credentials file too long\n");
 					return EX_USAGE;
@@ -620,7 +620,6 @@  static int open_cred_file(char *file_name,
 					sizeof(parsed_info->domain));
 			}
 		}
-
 	}
 	fclose(fs);
 	SAFE_FREE(line_buf);