Jeremy,

Jeremy Allison wrote:
> @@ -67,6 +68,13 @@
>
> #define CONST_DISCARD(type, ptr) ((type) ((void *) (ptr)))
>
> +#ifndef SAFE_FREE
> +#define SAFE_FREE(x) do { if ((x) != NULL) {free(x); x=NULL;} } while(0)
> +#endif
> +
> +#define MOUNT_PASSWD_SIZE 64
> +#define DOMAIN_SIZE 64
> +
> const char *thisprogram;
> int verboseflag = 0;
> static int got_password = 0;
> @@ -152,10 +160,7 @@ static void mount_cifs_usage(void)
> printf("\nTo display the version number of the mount helper:");
> printf("\n\t%s -V\n",thisprogram);
>
> - if(mountpassword) {
> - memset(mountpassword,0,64);
> - free(mountpassword);
> - }
> + SAFE_FREE(mountpassword);

Here we have non-equal behavior. Previously, the mountpassword content
was zeroed before freeing it due to security reasons. As
mount_cifs_usage() could be called multiple times (its call is in the
getopt_long()'s loop) and, particulary, after password has been filled
in, mountpassword's memory could still keep the password. Thus, memset()
is still needed.

> @@ -289,10 +285,7 @@ static int open_cred_file(char * file_name)
>
> }
> fclose(fs);
> - if(line_buf) {
> - memset(line_buf,0,4096);
> - free(line_buf);
> - }
> + SAFE_FREE(line_buf);

Same here.

> return 0;
> }
>
> @@ -303,9 +296,9 @@ static int get_password_from_file(int file_descript, char * filename)
> char c;
>
> if(mountpassword == NULL)
> - mountpassword = (char *)calloc(65,1);
> + mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1);

There is still one place where (char *)calloc(65,1); is left after this
commit: line 1197 (see below).


> @@ -1116,9 +1109,6 @@ int main(int argc, char ** argv)
> MOUNT_CIFS_VERSION_MAJOR,
> MOUNT_CIFS_VERSION_MINOR,
> MOUNT_CIFS_VENDOR_SUFFIX);
> - if(mountpassword) {
> - memset(mountpassword,0,64);
> - }

Again, mountpassword could be already filled in after some loops with
the getopt_long().

> @@ -1206,7 +1196,7 @@ int main(int argc, char ** argv)
> if(mountpassword == NULL)
> mountpassword = (char *)calloc(65,1);
> if(mountpassword) {
> - strncpy(mountpassword,getenv("PASSWD"),64);
> + strlcpy(mountpassword,getenv("PASSWD"),MOUNT_PASSWD_SIZE);
> got_password = 1;
> }
> } else if (getenv("PASSWD_FD")) {

Here it is: calloc(65,1) isn't replaced by calloc(MOUNT_PASSWD_SIZE+1,1).


--
/ Alexander Bokovoy
Samba Team http://www.samba.org/
ALT Linux Team http://www.altlinux.org/
Midgard Project Ry http://www.midgard-project.org/