[linux-cifs-client] [PATCH] 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. Temporary buffers are zeroed out before being freed to ensure passwords/credentials aren't left in released memory.
Jeff Layton
jlayton at samba.org
Sun Apr 25 07:39:01 MDT 2010
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Sat, 24 Apr 2010 19:21:08 -0400
Scott Lovenberg <scott.lovenberg at 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 at 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 at samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV
sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ
=iFIZ
-----END PGP SIGNATURE-----
More information about the linux-cifs-client
mailing list