NetBSD Problem Report #43852

From Wolfgang.Stukenbrock@nagler-company.com  Wed Sep  8 12:20:13 2010
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 9430C63BC84
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  8 Sep 2010 12:20:13 +0000 (UTC)
Message-Id: <20100908122000.700A419FBD0@s051.nagler-company.com>
Date: Wed,  8 Sep 2010 14:20:00 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: yp_passwd command may destroy NIS database entries when used on a server that includes users via netgroups
X-Send-Pr-Version: 3.95

>Number:         43852
>Category:       bin
>Synopsis:       yp_passwd command may destroy NIS database entries when used on a server that includes users via netgroups
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 08 12:25:00 +0000 2010
>Closed-Date:    Mon Nov 22 04:47:33 +0000 2010
>Last-Modified:  Mon Nov 22 04:47:33 +0000 2010
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 5.0.2
>Organization:
Sr. Nagler & Company GmbH

>Environment:


System: NetBSD s051 5.0.2 NetBSD 5.0.2 (NSW-S051) #2: Thu Aug 12 18:30:48 CEST 2010 wgstuken@s051:/usr/src/sys/arch/amd64/compile/NSW-S051 amd64
Architecture: x86_64
Machine: amd64
>Description:
	There has been a problem (in 4.x) with the same topic that "has" been fixed by PR 37863.
	I've send a patch for that, but accedently this patch has been modified prio applying it in a way,
	that the misbehaviour is not fixed.
	The password structure obtained from YP gets overwritten by the data from the local system
	(by calling getpwnam_r() with the same structure) again, so that the same bug is in there as before!

	I've checked the top-level version in CVS, the bug is still in there.

	I'm not shure why the usage of getpwnam_r() has been choosen at all.
	Prio PR37863 getpwnam() is used and my original patch also used getpwnam().
	There may be reasons why this is the better way of dooing it this way that I cannot see.
	  There is a static buffer of 1024 bytes now, that would be obsolete if getpwnam() is used. And the
	  current code assumes, that the dynamic parts like passwd, path to home, gecos and shell will fit into
	  theese 1024 bytes. 

	Neverless I've created a fix for the still existing problem based on the current source of 5.0.2 and let
	the getpwnam_r() in it.
>How-To-Repeat:
	same as in PR 37863 before ....
>Fix:
	apply the following patch to /usr/src/usr.bin/passwd/yp_passwd.c

--- yp_passwd.c	2010/09/08 11:55:11	1.1
+++ yp_passwd.c	2010/09/08 11:59:42
@@ -212,7 +212,7 @@
 	char *master;
 	int ch, r, rpcport, status;
 	struct yppasswd ypp;
-	struct passwd pwb, *pw;
+	struct passwd pwb, pwb2, *pw;
 	char pwbuf[1024];
 	struct timeval tv;
 	CLIENT *client;
@@ -284,16 +284,16 @@

 	/* Bail out if this is a local (non-yp) user, */
 	/* then get user's login identity */
-	if (!ypgetpwnam(username, pw = &pwb) ||
-	    getpwnam_r(username, &pwb, pwbuf, sizeof(pwbuf), &pw) ||
+	if (!ypgetpwnam(username, &pwb) ||
+	    getpwnam_r(username, &pwb2, pwbuf, sizeof(pwbuf), &pw) ||
 	    pw == NULL)
 		errx(1, "NIS unknown user %s", username);

-	if (uid && uid != pw->pw_uid)
+	if (uid && uid != pwb.pw_uid)
 		errx(1, "you may only change your own password: %s",
 		    strerror(EACCES));

-	makeypp(&ypp, pw);
+	makeypp(&ypp, &pwb);

 	client = clnt_create(master, YPPASSWDPROG, YPPASSWDVERS, "udp");
 	if (client == NULL)
@@ -374,7 +374,7 @@
 	char *master;
 	int r, rpcport, status;
 	struct yppasswd ypp;
-	struct passwd *pw, pwb;
+	struct passwd *pw, pwb, pwb2;
 	char pwbuf[1024];
 	struct timeval tv;
 	CLIENT *client;
@@ -418,19 +418,19 @@

 	/* Bail out if this is a local (non-yp) user, */
 	/* then get user's login identity */
-	if (!ypgetpwnam(username, pw = &pwb) ||
-	    getpwnam_r(username, &pwb, pwbuf, sizeof(pwbuf), &pw) ||
+	if (!ypgetpwnam(username, &pwb) ||
+	    getpwnam_r(username, &pwb2, pwbuf, sizeof(pwbuf), &pw) ||
 	    pw == NULL) {
 		warnx("NIS unknown user %s", username);
 		/* continuation */
 		return(-1);
 	}

-	if (uid && uid != pw->pw_uid)
+	if (uid && uid != pwb.pw_uid)
 		errx(1, "you may only change your own password: %s",
 		    strerror(EACCES));

-	makeypp(&ypp, pw);
+	makeypp(&ypp, &pwb);

 	client = clnt_create(master, YPPASSWDPROG, YPPASSWDVERS, "udp");
 	if (client == NULL) {

>Release-Note:

>Audit-Trail:
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: bin/43852: yp_passwd command may destroy NIS database entries when used on a server that includes users via netgroups
Date: Wed, 08 Sep 2010 14:56:07 +0200

 Hi again,

 sorry I'm too fast ....

 There are some parts missint in the patch I've send before ..

 The changes done to my original change are largar as I've recognized before.

 Please use the following patch instead.
 - It will access master.passwd.byname if presend - as done in the 
 getpwnam stuff
 - It will not free the still used memory returned from yp_match and that 
 is not reallocated by pw_scan()
 The only difference is in the block starting ag line 172 - that one was 
 missing
 Sorry again.

 Thanks

 --- yp_passwd.c 2010/09/08 12:20:59     1.1
 +++ yp_passwd.c 2010/09/08 12:49:59
 @@ -172,16 +172,22 @@
          int ok = 0;

          val = NULL;
 -       reason = yp_match(domain, "passwd.byname", nam, (int)strlen(nam),
 +       flags = 0;
 +       reason = yp_match(domain, "master.passwd.byname", nam, 
 (int)strlen(nam),
                            &val, &vallen);
 +       if (YPERR_MAP == reason) {
 +             reason = yp_match(domain, "passwd.byname", nam, 
 (int)strlen(nam),
 +                               &val, &vallen);
 +             flags = _PASSWORD_OLDFMT;
 +       }
          if (reason != 0)
                  goto out;

 -       flags = _PASSWORD_OLDFMT;
          if (pw_scan(val, pwd, &flags) == 0)
                  goto out;

          ok = 1;
 +       val = NULL; // do not free the memory - still in use
   out:
          if (val)
                  free(val);
 @@ -212,7 +218,7 @@
          char *master;
          int ch, r, rpcport, status;
          struct yppasswd ypp;
 -       struct passwd pwb, *pw;
 +       struct passwd pwb, pwb2, *pw;
          char pwbuf[1024];
          struct timeval tv;
          CLIENT *client;
 @@ -284,16 +290,16 @@

          /* Bail out if this is a local (non-yp) user, */
          /* then get user's login identity */
 -       if (!ypgetpwnam(username, pw = &pwb) ||
 -           getpwnam_r(username, &pwb, pwbuf, sizeof(pwbuf), &pw) ||
 +       if (!ypgetpwnam(username, &pwb) ||
 +           getpwnam_r(username, &pwb2, pwbuf, sizeof(pwbuf), &pw) ||
              pw == NULL)
                  errx(1, "NIS unknown user %s", username);

 -       if (uid && uid != pw->pw_uid)
 +       if (uid && uid != pwb.pw_uid)
                  errx(1, "you may only change your own password: %s",
                      strerror(EACCES));

 -       makeypp(&ypp, pw);
 +       makeypp(&ypp, &pwb);

          client = clnt_create(master, YPPASSWDPROG, YPPASSWDVERS, "udp");
          if (client == NULL)
 @@ -374,7 +380,7 @@
          char *master;
          int r, rpcport, status;
          struct yppasswd ypp;
 -       struct passwd *pw, pwb;
 +       struct passwd *pw, pwb, pwb2;
          char pwbuf[1024];
          struct timeval tv;
          CLIENT *client;
 @@ -418,19 +424,19 @@

          /* Bail out if this is a local (non-yp) user, */
          /* then get user's login identity */
 -       if (!ypgetpwnam(username, pw = &pwb) ||
 -           getpwnam_r(username, &pwb, pwbuf, sizeof(pwbuf), &pw) ||
 +       if (!ypgetpwnam(username, &pwb) ||
 +           getpwnam_r(username, &pwb2, pwbuf, sizeof(pwbuf), &pw) ||
              pw == NULL) {
                  warnx("NIS unknown user %s", username);
                  /* continuation */
                  return(-1);
          }

 -       if (uid && uid != pw->pw_uid)
 +       if (uid && uid != pwb.pw_uid)
                  errx(1, "you may only change your own password: %s",
                      strerror(EACCES));

 -       makeypp(&ypp, pw);
 +       makeypp(&ypp, &pwb);

          client = clnt_create(master, YPPASSWDPROG, YPPASSWDVERS, "udp");
          if (client == NULL) {




 gnats-admin@NetBSD.org wrote:

 > Thank you very much for your problem report.
 > It has the internal identification `bin/43852'.
 > The individual assigned to look at your
 > report is: bin-bug-people. 
 > 
 > 
 >>Category:       bin
 >>Responsible:    bin-bug-people
 >>Synopsis:       yp_passwd command may destroy NIS database entries when used on a server that includes users via netgroups
 >>Arrival-Date:   Wed Sep 08 12:25:00 +0000 2010
 >>
 > 


From: Christos Zoulas <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/43852 CVS commit: src/usr.bin/passwd
Date: Wed, 8 Sep 2010 09:44:44 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Wed Sep  8 13:44:44 UTC 2010

 Modified Files:
 	src/usr.bin/passwd: yp_passwd.c

 Log Message:
 PR/43852: Wolfgang Stukenbrock: yp_passwd command may destroy NIS database
 entries when used on a server that includes users via netgroups.


 To generate a diff of this commit:
 cvs rdiff -u -r1.33 -r1.34 src/usr.bin/passwd/yp_passwd.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Tue, 16 Nov 2010 04:08:40 +0000
State-Changed-Why:
pullup-5 #1476


From: "Jeff Rizzo" <riz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/43852 CVS commit: [netbsd-5] src/usr.bin/passwd
Date: Mon, 22 Nov 2010 02:41:07 +0000

 Module Name:	src
 Committed By:	riz
 Date:		Mon Nov 22 02:41:07 UTC 2010

 Modified Files:
 	src/usr.bin/passwd [netbsd-5]: yp_passwd.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1476):
 	usr.bin/passwd/yp_passwd.c: revision 1.34
 PR/43852: Wolfgang Stukenbrock: yp_passwd command may destroy NIS database
 entries when used on a server that includes users via netgroups.


 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.32.10.1 src/usr.bin/passwd/yp_passwd.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 22 Nov 2010 04:47:33 +0000
State-Changed-Why:
Patch applied and pulled up to -5; thanks!


>Unformatted:

Home	PR Database Search