[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 pas
Scott Lovenberg
scott.lovenberg at gmail.com
Mon Apr 26 22:13:20 MDT 2010
On Sun, Apr 25, 2010 at 9:39 AM, Jeff Layton <jlayton at samba.org> wrote:
> -----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.
>
Hrm... those were multiple lines. There was a hard return after the first
sentence.
>
> > 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?
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 <jlayton at samba.org>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
>
> iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV
> sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ
> =iFIZ
> -----END PGP SIGNATURE-----
>
--
Peace and Blessings,
-Scott.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.samba.org/pipermail/linux-cifs-client/attachments/20100427/baf1cca2/attachment-0001.html>
More information about the linux-cifs-client
mailing list