[1/2] Clean up credential file parsing in mount.cifs.c.

Remove magic numbers, redundant code and extra variables from open_cred_file().
Remove check for domain length since strlcpy is safe from buffer overflows.

Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com>
---
 mount.cifs.c |   78 +++++++++++++++++++++++----------------------------------
 1 files changed, 32 insertions(+), 46 deletions(-)


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

On Fri, 23 Apr 2010 02:17:12 -0400
Scott Lovenberg <scott.lovenberg@gmail.com> wrote:

> Remove magic numbers, redundant code and extra variables from open_cred_file().
> Remove check for domain length since strlcpy is safe from buffer overflows.
> 
> Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com>
> ---
>  mount.cifs.c |   78 +++++++++++++++++++++++----------------------------------
>  1 files changed, 32 insertions(+), 46 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index ba9e206..97dae82 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,13 +511,27 @@ toggle_dac_capability(int writable, int enable)
>  #endif /* HAVE_LIBCAP */
>  #endif /* HAVE_LIBCAP_NG */
>  
> +/*
> + * Null terminate 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)
> @@ -541,50 +555,35 @@ 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 credentials 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_endl(line_buf);
>  
> -		/* NULL terminate at newline */
> -		newline = strchr(line_buf + i, '\n');
> -		if (newline)
> -			*newline = '\0';
> -
> +		/* 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++;
> -				for (length = 0; length < 4087; length++) {
> -					if ((temp_val[length] == '\n')
> -					    || (temp_val[length] == '\0')) {
> -						temp_val[length] = '\0';
> -						break;
> -					}
> -				}
> -				if (length > 4086) {
> -					fprintf(stderr,
> -						"mount.cifs failed due to malformed username in credentials file\n");
> -					memset(line_buf, 0, 4096);
> -					return EX_USAGE;
> -				}
>  				parsed_info->got_user = 1;
>  				strlcpy(parsed_info->username, temp_val,
>  					sizeof(parsed_info->username));
>  			}
> -		} 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;
> @@ -592,7 +591,10 @@ static int open_cred_file(char *file_name,
>  			i = set_password(parsed_info, temp_val);
>  			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 */
> @@ -600,22 +602,6 @@ static int open_cred_file(char *file_name,
>  				if (parsed_info->verboseflag)
>  					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) {
> -					fprintf(stderr,
> -						"mount.cifs failed: domain in credentials file too long\n");
> -					return EX_USAGE;
> -				}
> -
>  				strlcpy(parsed_info->domain, temp_val,
>  					sizeof(parsed_info->domain));
>  			}
> 
> --------------1.6.2.5--
> 
> 
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 

Looks good. Committed with some minor whitespace cleanup.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Sat, 24 Apr 2010 19:21:08 -0400
Scott Lovenberg <scott.lovenberg@gmail.com> wrote:

Maybe try not to put all of the description on the subject line. You
can use multiple lines and it makes it a little tougher to import the
patch.

> Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com>
> ---
>  mount.cifs.c |  141 ++++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 83 insertions(+), 58 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 97dae82..09d6351 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_UNPARSEABLE 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;
> @@ -511,20 +519,43 @@ toggle_dac_capability(int writable, int enable)
>  #endif /* HAVE_LIBCAP */
>  #endif /* HAVE_LIBCAP_NG */
>  
> -/*
> - * Null terminate string at first '\n'
> - */
> -static void null_terminate_endl(char* source)
> +static void null_terminate_endl(char *source)
>  {
> -	char* newline = strchr(source, '\n');
> +	char *newline = strchr(source, '\n');
>  	if (newline)
>  		*newline = '\0';
>  }
>  
> -
> +/*
> + * Parse a line from the credentials file.  Changes target to first
> + * character after '=' on 'line' and returns the value type of the line
> + * Returns CRED_UNPARSEABLE on failure or if either parameter is NULL.
> + */
> +static int parse_cred_line(char *line, char **target)
> +{
> +	if (line == NULL || target == NULL)
> +		goto parsing_err;
> +
> +	/* position target at first char of value */
> +	*target = strchr(line, '=');
> +	if (!target)
	^^^^^^^^^^^^
Should be:
	if (!*target)

...right?

> +		goto parsing_err;
> +	*target += 1;
> +
> +	/* tell the caller which value target points to */
> +	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;
> +
> +	parsing_err:
> +		return CRED_UNPARSEABLE;
> +}
>  
>  static int open_cred_file(char *file_name,
> -			  struct parsed_mount_info *parsed_info)
> +			struct parsed_mount_info *parsed_info)
>  {
>  	char *line_buf;
>  	char *temp_val;
> @@ -535,30 +566,29 @@ static int open_cred_file(char *file_name,
>  
>  	i = toggle_dac_capability(0, 1);
>  	if (i)
> -		return i;
> -
> +		goto return_i;
> +	
>  	i = access(file_name, R_OK);
>  	if (i) {
>  		toggle_dac_capability(0, 0);
> -		return i;
> +		goto return_i;
>  	}
>  
>  	fs = fopen(file_name, "r");
>  	if (fs == 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;
> -	}
> -
> -	line_buf = (char *)malloc(line_buf_size);
> +	if(i)
> +		goto return_i;
> +	
> +	line_buf = (char*)malloc(line_buf_size);
>  	if (line_buf == NULL) {
> -		fclose(fs);
> -		return EX_SYSERR;
> +		i = EX_SYSERR;
> +		goto return_i;
>  	}
>  
>  	/* parse line from credentials file */
> @@ -570,47 +600,42 @@ 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++;
> -				if (parsed_info->verboseflag)
> -					fprintf(stderr, "\nDomain %s\n",
> -						temp_val);
> -				strlcpy(parsed_info->domain, temp_val,
> -					sizeof(parsed_info->domain));
> -			}
> +		/* parse next token */
> +		switch(parse_cred_line(line_buf + i, &temp_val)) {
> +		case CRED_USER:
> +			parse_username(temp_val, parsed_info);

I think you should check the return value from parse_username here too.

> +			break;
> +		case CRED_PASS:
> +			if ((i = set_password(parsed_info, temp_val)))
> +				goto return_i;
> +			break;
> +		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;
> +		case CRED_UNPARSEABLE:
> +			if (parsed_info->verboseflag)
> +				fprintf(stderr,
> +					"Credential formatted incorrectly: %s",
> +					temp_val);
> +			break;
>  		}
> -
>  	}
> -	fclose(fs);
> -	SAFE_FREE(line_buf);
> -	return 0;
> +
> +	i = 0;
> +	/* jump point for errors */
> +	return_i:

	^^^^^^^^
Minor nit: typically labels are flush to the left, and the code below
shouldn't be indented as much.

> +		if (fs != NULL)
> +			fclose(fs);
> +
> +		/* make sure passwords are scrubbed from memory */
> +		if (line_buf != NULL)
> +			memset(line_buf, 0, line_buf_size);
> +		SAFE_FREE(line_buf);
> +		return i;
>  }
>  
>  static int

There's also some trailing whitespace and other formatting errors that
checkpatch complained about, and the patch didn't apply cleanly to the
head git commit. I cleaned up most of these problems and will send a
revised patch in a bit.

- -- 
Jeff Layton <jlayton@samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)

iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV
sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ
=iFIZ
-----END PGP SIGNATURE-----