bin/43852: yp_passwd command may destroy NIS database entries when used on a server that includes users via netgroups

[Wolfgang.Stukenbrock%nagler-company.com@localhost]

>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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 08 12:25:00 +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) {

>Unformatted: