[2/2] Continued cleanup of open_cred_file and fixed a potential security risk.

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@gmail.com>
---
 mount.cifs.c |  141 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 87 insertions(+), 54 deletions(-)


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

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

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

> 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@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

		^^^ minor nit...should be "UNPARSEABLE".

> +#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++;

^^^^
I don't think this will work. C does pass by value. Here you're passing
in "target" as a pointer. That value will be updated in the local copy
of that pointer in parse_cred_line, but caller will not see that you've
changed it. If you want to do this, you need to pass "target" as a
pointer to a pointer and fix the references to it accordingly.

> +	
> +	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));
		^^^^^
		line_buf and temp_val are different pointers into the
		same buffer. I think it's sufficient to just zero out
		line_buf.

> +		SAFE_FREE(line_buf);
> +		SAFE_FREE(temp_val);
		^^^ that would be a double-free.

> +		return i;
>  }
>  
>  static int
> 
> --------------1.6.2.5--
> 
> 

There are also some minor whitespace issues with these patches too
(trailing whitespace on a few lines). Since you'll need to fix the
above anyway, it would be good to fix those as well.

You might want to run the checkpatch.pl script from the Linux kernel
sources against these patches and fix the problems it says too. I'd
like to see this code follow the Linux kernel coding standards for the
most part.

Other than the problems above, this looks like a reasonable change.
- -- 
Jeff Layton <jlayton@samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)

iEYEARECAAYFAkvRgpQACgkQyP0gxQMdzICvXgCffgDBknwYp69lW81J3sTi1wsb
ExYAoJtp2vJDzHsD4owv08rsCJRD9ZkV
=3jXI
-----END PGP SIGNATURE-----

>
> > +#define CRED_UNPARSABLE 0
>
>                 ^^^ minor nit...should be "UNPARSEABLE".
>
> Not nitpicking at all, spelling mistakes in code are embarrassing.  I had a
feeling I should have double checked the spelling on that.


>
> > +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++;
>
> ^^^^
> I don't think this will work. C does pass by value. Here you're passing
> in "target" as a pointer. That value will be updated in the local copy
> of that pointer in parse_cred_line, but caller will not see that you've
> changed it. If you want to do this, you need to pass "target" as a
> pointer to a pointer and fix the references to it accordingly.
>
D'oh.  It's been a few years since I've touched pointers at all.  Easy
enough change.


>
>
> There are also some minor whitespace issues with these patches too
> (trailing whitespace on a few lines).

Yeah, I found out that the formatting problems that I've been having are due
to UTF-8 and interaction between git-format-patch and git-send-email.  I
added a format section to my .gitconfig (and git-format-patch seemed happy),
but I guess I've still got an issue. :/


> Since you'll need to fix the
> above anyway, it would be good to fix those as well.
>
> You might want to run the checkpatch.pl script from the Linux kernel
> sources against these patches and fix the problems it says too. I'd
> like to see this code follow the Linux kernel coding standards for the
> most part.
>
 Will do.

On Fri, 23 Apr 2010 11:30:56 -0400
Scott Lovenberg <scott.lovenberg@gmail.com> wrote:


> >
> >
> > There are also some minor whitespace issues with these patches too
> > (trailing whitespace on a few lines).
> 
> Yeah, I found out that the formatting problems that I've been having are due
> to UTF-8 and interaction between git-format-patch and git-send-email.  I
> added a format section to my .gitconfig (and git-format-patch seemed happy),
> but I guess I've still got an issue. :/
> 

Mostly it was trailing whitespace on some of the lines. checkpatch will
generally catch that. Another way to see it is to colorize your git
diff output. I have this in my .gitconfig, and with it trailing
whitespace shows up in bright red:

[color]
        ui = auto
[color "branch"]
        current = yellow reverse
        local = yellow
        remote = green
[color "diff"]
        meta = yellow bold
        frag = magenta bold
        old = red bold
        new = green bold
[color "status"]
        added = yellow
        changed = green
        untracked = cyan

...I think there are also git options to automatically remove trailing
whitespace, but I'm a little more wary of that. YMMV.