[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [vendor-sec] Some problems (fwd)



Hi all,

I've make a small patch fixes #2 and #3 from Olaf's list.
Critical notes are welcome :-)

I think a problem #1 is a pwdb's one and I have no enough time
to do complete investigations.

#4 is a hard one. I mentioned that pam_rhosts assumes IPv4 a few months ago.
But an address size is so deeply hardcoded that I suppose
only the author can make necessary changes.

Moreover, it seems that the code mixes 4-bytes integers
and longs (look at memcmp with the results of gethostbyXXX!).
I'm doubt that somebody can use the module on 64-bit architectures.

Olaf, did you really look over all PAM modules for security problems?
It's a _great_ job!

Best wishes
					Andrey V.
					Savochkin


On Mon, Jan 26, 1998 at 11:21:03AM -0800, Andrew Morgan wrote:
> Hi,
> 
> if anyone has some free time and wants to look into these and send me
> patches (by tomorrow) I would be very happy to let someone else do the
> work! ;^)
> 
> People might also like to get a heads up on the problems Olaf has
> identified.
> 
> Cheers
> 
> Andrew
> 
> ----- Forwarded message from Olaf Kirch -----
> From: Olaf Kirch
> Subject: Some problems
> 
> Hi Andrew,
> 
> I was looking into linux-pam for potential security problems about
> two weeks ago and found some minor problems with it, which I'm now
> forwarding to you FYI. However I was glad to find that most of the code
> seems very robust.
> 
> I lost the notes I made when I cleaned up my desk, so I have to collect
> them from memory. This is all wrt. pam 0.58 and pwdb 0.54.
> 
>  1.	File locking: when locking files, pam (or was it libpwdb?)
> 	tries to work some NFS magic by first creating a temp lock
> 	file, linking it to the real lock file, and making certain the
> 	link count equals 2 after the file is created.
> 
> 	The problem with this is that if the /tmp directory is on the
> 	same disk as /etc (which it is more often than not), an attacker
> 	can create a hard link to the lock file between the file's
> 	creation and the link count check.
> 
> 	Fortunately, it's not straightforward to turn this into a DoS
> 	exploit as the locking process records its pid in the file and
> 	when the next process comes around, it will check whether the
> 	process with the recorded pid is still alive, and if it isn't,
> 	simply remove the lock file. The attacker could try to fork
> 	processes until the kernel's pid counter wraps around, but there
> 	doesn't seem to be a way how he can obtain the lock pid. He may
> 	be able to do so by scanning /etc for the temp lock file name,
> 	which contains the pid. Otherwise, he may simply try to grab
> 	a range of pids.
> 
> 	[This kind of 'NFS compatibility' also makes for nice DoS
> 	attacks on user mailboxes if the mail spool is mode 1777
> 	and the mail reader performs this kind of warped check on
> 	/var/spool/mail/victimname.lock...]
> 
> 	There's also a naming problem with the lock files. Most utilities
> 	I've seen expect the name of the lock file for /etc/passwd to
> 	be /etc/ptmp, while libpwdb uses /etc/pwd.lock.
> 
>  2.	pam_access: The first problem is that when it comes to the
> 	local host name, it calls uname(2) and extracts utsname.machine
> 	-- which returns i586 on my box. This is used for user name
> 	matches of the type 'user@fromhost'. In this case it always
> 	compares fromhost to i586...  At least kind of surprising.
> 
> 	The second problem is with address patterns. If you specify
> 	1.2.3. in access.conf, the test will match any host whose
> 	FQDN starts with 1.2.3.
> 
>  3.	pam_listfile: with item=group, the code appears to do weird
> 	things. It first puts the user's primary group on itemlist,
> 	and then goes on to append member names from the first couple
> 	of groups getgrent happens to return:
> 
> 	for(i=1; (i < 255) && (grpinfo = getgrent()); i++) {
> 	    for(j=0;grpinfo->gr_mem[j];)
> 		    itemlist[i++] = xstrdup(grpinfo->gr_mem[j++]);
> 	}
> 
> 	Fortunately this code will most likely dump core anyway
> 	(note the double increment on i, and the missing bounds check
> 	in the inner loop)
> 
> 	I guess what was intended was something like this:
> 
> 	for(i=1; (i < 255) && (grpinfo = getgrent());) {
> 	    for(j=0;grpinfo->gr_mem[j];)
> 		if (is_on_list(grpinfo->gr_mem, citemp))
> 		    itemlist[i++] = xstrdup(grpinfo->gr_name);
> 	}
> 
>  4.	pam_rhost: Blindly assumes that whatever gethostbyname returns
> 	is an IPv4 address, and matches that.
> 
> I admit that I'm not very familiar with the PAM internals, so some of the
> issues raised may in fact be non-issues, or I may be wrong because I
> misunderstood the code.
> 
> Cheers
> Olaf
> ----- End of forwarded message from Olaf Kirch -----
diff -ru Linux-PAM-0.62.orig/modules/pam_access/pam_access.c Linux-PAM-0.62/modules/pam_access/pam_access.c
--- Linux-PAM-0.62.orig/modules/pam_access/pam_access.c	Mon Jun 23 00:33:59 1997
+++ Linux-PAM-0.62/modules/pam_access/pam_access.c	Tue Jan 27 19:05:07 1998
@@ -7,6 +7,11 @@
  *
  */
 
+#ifdef linux
+# define _GNU_SOURCE
+# include <features.h>
+#endif
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -20,6 +25,10 @@
 #include <errno.h>
 #include <ctype.h>
 #include <sys/utsname.h>
+#ifndef BROKEN_NETWORK_MATCH
+#include <netdb.h>
+#include <sys/socket.h>
+#endif
 
 
 /*
@@ -197,7 +206,7 @@
     struct utsname buf;
 
     uname(&buf);
-    strncpy(name,buf.machine,MAXHOSTNAMELEN);
+    strncpy(name,buf.nodename,MAXHOSTNAMELEN);
     name[MAXHOSTNAMELEN] = 0;
     return (name);
 }
@@ -281,9 +290,30 @@
     } else if (strcasecmp(tok, "LOCAL") == 0) {	/* local: no dots */
 	if (strchr(string, '.') == 0)
 	    return (YES);
+#ifdef BROKEN_NETWORK_MATCH
     } else if (tok[(tok_len = strlen(tok)) - 1] == '.'	/* network */
 	       && strncmp(tok, string, tok_len) == 0) {
 	return (YES);
+#else
+    } else if (tok[(tok_len = strlen(tok)) - 1] == '.') {
+        /*
+           The code below does a more correct check if the address specified
+           by "string" starts from "tok".
+                               1998/01/27  Andrey V. Savochkin <saw@msu.ru>
+         */
+        struct hostent *h;
+        char hn[3+1+3+1+3+1+3+1];
+        int r;
+        h = gethostbyname(string);
+        if (h == NULL) return (NO);
+        if (h->h_addrtype != AF_INET) return (NO);
+        if (h->h_length != 4) return (NO); /* only IPv4 addresses (SAW) */
+        r = snprintf(hn, sizeof(hn), "%u.%u.%u.%u",
+                (unsigned char)h->h_addr[0], (unsigned char)h->h_addr[1],
+                (unsigned char)h->h_addr[2], (unsigned char)h->h_addr[3]);
+        if (r < 0 || r >= sizeof(hn)) return (NO);
+        if (!strncmp(tok, hn, tok_len)) return (YES);
+#endif
     }
     return (NO);
 }
diff -ru Linux-PAM-0.62.orig/modules/pam_listfile/pam_listfile.c Linux-PAM-0.62/modules/pam_listfile/pam_listfile.c
--- Linux-PAM-0.62.orig/modules/pam_listfile/pam_listfile.c	Wed Jan 14 21:57:48 1998
+++ Linux-PAM-0.62/modules/pam_listfile/pam_listfile.c	Tue Jan 27 18:57:07 1998
@@ -146,7 +146,7 @@
 PAM_EXTERN
 int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, const char **argv)
 {
-    int retval, i, j, citem=0, extitem=0, onerr=PAM_SERVICE_ERR, sense=2;
+    int retval, i, citem=0, extitem=0, onerr=PAM_SERVICE_ERR, sense=2;
     const char *citemp;
     char *ifname=NULL;
     char aline[256];
@@ -304,11 +304,6 @@
 
     if(extitem) {
 	switch(extitem) {
-	    /* Group checking only checks primary groups (i.e. listed in
-	       /etc/passwd) - it doesn't check other ones.  To add full
-	       group checking capability would require a redo of things
-	       here, since the now-short main comparison loop would have to
-	       go through an array of items. */
 	    case EI_GROUP:
 		setpwent();
 		userinfo = getpwnam(citemp);
@@ -316,9 +311,10 @@
 		grpinfo = getgrgid(userinfo->pw_gid);
 		itemlist[0] = x_strdup(grpinfo->gr_name);
 		setgrent();
-		for(i=1; (i < 255) && (grpinfo = getgrent()); i++) {
-		    for(j=0;grpinfo->gr_mem[j];)
-			itemlist[i++] = x_strdup(grpinfo->gr_mem[j++]);
+		for(i=1; (i < sizeof(itemlist)/sizeof(itemlist[0])-1) &&
+		         (grpinfo = getgrent()); i++) {
+		    if(is_on_list(grpinfo->gr_mem,citemp))
+			itemlist[i++] = x_strdup(grpinfo->gr_name);
 		}
 		itemlist[i] = NULL;
 		endgrent();

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index] []