[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    [PATCH] adduser can create duplicate users
From:       Tito <farmatito () tiscali ! it>
Date:       2007-10-27 13:59:25
Message-ID: 200710271559.25601.farmatito () tiscali ! it
[Download RAW message or body]

Hi,
this patch fixes a problem in adduser spotted by Ralf Friedl:

>Also, I think it is possible that this implementation results in 
>duplicate user ids:
>The first loop, "while (!fgetpwent_r(..." find a free user id.
>The second loop, "while (getgrgid(p->pw_uid)) p->pw_uid++;" increments 
>uid until it finds an unused group id.
>It is possible that this incremented uid is in use as a user id.

The patch is tested and works for me but I would like it to be  reviewed on the list
to avoid some of mine silly errors ;-) .
As side effect the patch also reduces size:

root@localhost:~/Desktop/busybox.latest# scripts/bloat-o-meter busybox_old busybox_unstripped
function                                             old     new   delta
bb_internal_fgetpwent_r                               51       -     -51
adduser_main                                         909     679    -230
------------------------------------------------------------------------------
(add/remove: 0/1 grow/shrink: 0/1 up/down: 0/-281)           Total: -281 bytes

The patch changes the passwd_study function:

static int passwd_study(const char *filename, struct passwd *p)
{
	enum { min = 500, max = 65000 };
	FILE *passwd;

	passwd = xfopen(filename, "r");

	/* EDR if uid is out of bounds, set to min */
	if ((p->pw_uid > max) || (p->pw_uid < min))
		p->pw_uid = min;

	/* check for an already in use login name */
	if (getpwnam(p->pw_name))
		 return 1;

	/* search a free uid */
	while (getpwuid(p->pw_uid))
		p->pw_uid++;

	if (!p->pw_gid) {
		/* check for an already in use group name */
		if (getgrnam(p->pw_name))
			return 3;
		/* search a free gid and free uid with the same value */
		while (getgrgid(p->pw_uid) || getpwuid(p->pw_uid))
			p->pw_uid++;
		/* set gid = uid */
		p->pw_gid = p->pw_uid;
	}

	/* EDR bounds check, could not be less than min */
	if (p->pw_uid > max)
		return 2;

	return 0;
}

Ciao,
Tito

["adduser_fix.patch" (text/x-diff)]

--- loginutils/adduser.c.orig	2007-10-27 08:57:23.000000000 +0200
+++ loginutils/adduser.c	2007-10-27 15:54:25.000000000 +0200
@@ -20,11 +20,6 @@
 {
 	enum { min = 500, max = 65000 };
 	FILE *passwd;
-	/* We are using reentrant fgetpwent_r() in order to avoid
-	 * pulling in static buffers from libc (think static build here) */
-	char buffer[256];
-	struct passwd pw;
-	struct passwd *result;
 
 	passwd = xfopen(filename, "r");
 
@@ -32,39 +27,29 @@
 	if ((p->pw_uid > max) || (p->pw_uid < min))
 		p->pw_uid = min;
 
-	/* stuff to do:
-	 * make sure login isn't taken;
-	 * find free uid and gid;
-	 */
-	while (!fgetpwent_r(passwd, &pw, buffer, sizeof(buffer), &result)) {
-		if (strcmp(pw.pw_name, p->pw_name) == 0) {
-			/* return 0; */
-			return 1;
-		}
-		if ((pw.pw_uid >= p->pw_uid) && (pw.pw_uid < max)
-			&& (pw.pw_uid >= min)) {
-			p->pw_uid = pw.pw_uid + 1;
-		}
-	}
-
-	if (p->pw_gid == 0) {
-		/* EDR check for an already existing gid */
-		while (getgrgid(p->pw_uid) != NULL)
-			p->pw_uid++;
-
-		/* EDR also check for an existing group definition */
-		if (getgrnam(p->pw_name) != NULL)
+	/* check for an already in use login name */
+	if (getpwnam(p->pw_name))
+		 return 1;
+
+	/* search a free uid */
+	while (getpwuid(p->pw_uid))
+		p->pw_uid++;
+
+	if (!p->pw_gid) {
+		/* check for an already in use group name */
+		if (getgrnam(p->pw_name))
 			return 3;
-
-		/* EDR create new gid always = uid */
+		/* search a free gid and free uid with the same value */
+		while (getgrgid(p->pw_uid) || getpwuid(p->pw_uid))
+			p->pw_uid++;
+		/* set gid = uid */
 		p->pw_gid = p->pw_uid;
 	}
 
-	/* EDR bounds check */
-	if ((p->pw_uid > max) || (p->pw_uid < min))
+	/* EDR bounds check, could not be less than min */
+	if (p->pw_uid > max)
 		return 2;
 
-	/* return 1; */
 	return 0;
 }
 


_______________________________________________
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]