Re: [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 pas

On Sun, Apr 25, 2010 at 9:39 AM, Jeff Layton <[email protected]> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Sat, 24 Apr 2010 19:21:08 -0400
> Scott Lovenberg <[email protected]> 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.
>
Hrm... those were multiple lines.  There was a hard return after the first
sentence.

>
> > Signed-off-by: Scott Lovenberg <[email protected]>
> > ---
> >  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?

Correct. Silly mistake.

> > +             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.
>
> Probably a good idea.

> > +                     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.
>
> Thanks, I forgot to synchronize with remote before I created the patch.  As
for the white space, I think that's an artifact from git, there wasn't any
in the original patch.  Still getting to the bottom of that.

> - --
> Jeff Layton <[email protected]>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
>
> iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV
> sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ
> =iFIZ
> -----END PGP SIGNATURE-----
>



-- 
Peace and Blessings,
-Scott.

_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client