Re: [SCM] Samba Shared Repository - branch v3-2-test updated - release-3-2-0pre2-2199-gcc23f91

[Alexander Bokovoy]

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/