system.c bug / possible fix

[Elrond]

Hi everybody,

system.c (the current version in head 1.66, the version in
2.2.0 release, previous version in TNG) has a flaw in the
getpw*-cache code.

The general problem was found by Mirko Manea
<mami at arena.sci.univr.it>, who found that using another
version fixed problems he had with LDAP and nt home
directories.

After reading the interesting code many times, I found the
bug:

The current code made a copy of the struct passwd returned
by getpw*(), and since libc might overwrite strings, that
were used in that struct, pw_name and pw_eturned to allow the caller to modify. When were copied
into local memory too (and to allow overwriting those
strings).

The problem is now, that pass->pw_dir (the home dir),
wasn't copied, but left as pointer into libc-memory. When
libc was doing a new getpw*, the memory was probably
overwritten.

You might ask, why THIS error didn't appear in the first
cache-version. Because in that version, a pointer to the
passwd-struct in libc was saved. When libc has read a new
entry into that memory, the memory ALSO contained a new uid
and pw_name, which the cache-compare code used and either
got the right values or a "cache miss".

This bug is somewhat related to the bug fixed in the last
commit in HEAD:
"
  lib/system.c (Finally) fixed all insure errors in password caching code. We can't
  stop libc routines from calling getpwXXX functions, so caching a pointer to them
  is impossible. This new code now makes two copies of the returned struct passwd
  struct - one used as a cache, one returned to allow the caller to modify. When
  doing a lookup we compare against the cached copy. Code is now easier to understand
  also.
"

I have appended a patch (for samba.org, TNG has this
already in cvs) for this problem.

I haven't fully tested this patch, but at least my PDC
works fine with it (it worked before, so I don't know, if
it fixes the actual problem.)


    Elrond


Index: lib/system.c
===================================================================
RCS file: /work/cvs/tng/source/lib/system.c,v
retrieving revision 1.6
diff -u -p -r1.6 system.c
--- lib/system.c	2001/04/21 14:10:53	1.6
+++ lib/system.c	2001/04/28 09:24:28
@@ -21,7 +21,6 @@
 
 #include "includes.h"
 
-extern int DEBUGLEVEL;
 
 /*
    The idea is that this file will eventually have wrappers around all
@@ -574,6 +573,10 @@ int sys_setgroups(int setlen, gid_t *gid
 /*
  * We only wrap pw_name and pw_passwd for now as these
  * are the only potentially modified fields.
+ * We need to wrap pw_gecos and pw_dir too, because we
+ * otherwise keep pointers to old strings in libc.
+ * pw_shell should be wrapped for the same reason, but it
+ * isn't used by anyone yet.
  */
 
 /**************************************************************************
@@ -583,9 +586,39 @@ int sys_setgroups(int setlen, gid_t *gid
 struct saved_pw {
 	fstring 	pw_name;
 	fstring 	pw_passwd;
+	fstring 	pw_gecos;
+	fstring 	pw_dir;
 	struct passwd pass;
 };
 
+/*
+ * when "pass" in a saved_pw is copied over, it already
+ * contains all pointers, so we can copy the strings over
+ * from there.
+ */
+static void fill_saved_pw(struct saved_pw *pw)
+{
+	struct passwd *pass = &pw->pass;
+
+	if (pass->pw_name) {
+		fstrcpy(pw->pw_name, pass->pw_name);
+		pass->pw_name = pw->pw_name;
+	}
+	if (pass->pw_passwd) {
+		fstrcpy(pw->pw_passwd, pass->pw_passwd);
+		pass->pw_passwd = pw->pw_passwd;
+	}
+	if (pass->pw_gecos) {
+		fstrcpy(pw->pw_gecos, pass->pw_gecos);
+		pass->pw_gecos = pw->pw_gecos;
+	}
+	if (pass->pw_dir) {
+		fstrcpy(pw->pw_dir, pass->pw_dir);
+		pass->pw_dir = pw->pw_dir;
+	}
+}
+
+
 static struct saved_pw pw_mod; /* This is the structure returned - can be modified. */
 static struct saved_pw pw_cache; /* This is the structure saved - used to check cache. */
 
@@ -598,30 +631,24 @@ static struct passwd *setup_pwret(struct
 {
 	if (pass == NULL) {
 		/* Clear the caches. */
-		memset(&pw_cache, '\0', sizeof(struct saved_pw));
-		memset(&pw_mod, '\0', sizeof(struct saved_pw));
+		ZERO_STRUCT(pw_cache);
+		ZERO_STRUCT(pw_mod);
 		num_lookups = 0;
 		return NULL;
 	}
-
-	/* this gets the uid, gid and null pointers */
 
-	memcpy((char *)&pw_mod.pass, pass, sizeof(struct passwd));
-	fstrcpy(pw_mod.pw_name, pass->pw_name);
-	pw_mod.pass.pw_name = pw_mod.pw_name;
-	fstrcpy(pw_mod.pw_passwd, pass->pw_passwd);
-	pw_mod.pass.pw_passwd = pw_mod.pw_passwd;
+	/* this gets the uid, gid and pointers */
+	pw_mod.pass = *pass;
 
+	/* this copies the strings */
+	fill_saved_pw(&pw_mod);
 
 	if (pass != &pw_cache.pass) {
 
 		/* If it's a cache miss we must also refill the cache. */
 
-		memcpy((char *)&pw_cache.pass, pass, sizeof(struct passwd));
-		fstrcpy(pw_cache.pw_name, pass->pw_name);
-		pw_cache.pass.pw_name = pw_cache.pw_name;
-		fstrcpy(pw_cache.pw_passwd, pass->pw_passwd);
-		pw_cache.pass.pw_passwd = pw_cache.pw_passwd;
+		pw_cache.pass = *pass;
+		fill_saved_pw(&pw_cache);
 
 		num_lookups = 1;