[linux-cifs-client] [PATCH 2/2] Continued cleanup of open_cred_file and fixed a potential security risk.
Jeff Layton
jlayton at samba.org
Fri Apr 23 05:20:49 MDT 2010
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Fri, 23 Apr 2010 02:17:13 -0400
Scott Lovenberg <scott.lovenberg at 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 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
^^^ 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 at samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
iEYEARECAAYFAkvRgpQACgkQyP0gxQMdzICvXgCffgDBknwYp69lW81J3sTi1wsb
ExYAoJtp2vJDzHsD4owv08rsCJRD9ZkV
=3jXI
-----END PGP SIGNATURE-----
More information about the linux-cifs-client
mailing list