Winbind patch fixes broken client. Please have a look.

[Hannes Schmidt]

As you suggested, I checked how other NSS libraries deal with this problem.
The libnss_files module in glibc doesn't leave files open like
libnss_winbind.

E.g., getpwent() (I should say its implementation in libnss_files) and
setpwent()
open /etc/passwd and endpwent() closes it. getpwnam() and getpwuid() close
it before returning.

I didn't completely understand libnss_nis, but it seems to do likewise.

I noticed another interesting thing: all the above NSS libraries set the
close-on-exec flag on the files they open. This is a good idea for two
reasons:

(A) security - see http://www.pgp.com/research/covert/advisories/028.asp.

(B) saving resources - an exec() overwrites all data, even the
shared libraries data. Consequently, the variable which holds the pipe/file
handle gets reset too and there is no way to close the file. It will get
opened
a second time. For each exec() one file handle will be wasted.

My patch fixes both problems - it sets the close-on-exec flag for the
winbindd
pipe handle and it makes sure that the handle isn't 0, 1, 2. I tested it on
my three
production servers. No problems.

diff -u -x *.o -x *.so -x *.po samba-2.2.5/source/nsswitch/wb_common.c
samba-2.2.5-modified/source/nsswitch/wb_common.c
--- samba-2.2.5/source/nsswitch/wb_common.c Wed Jun 19 03:13:44 2002
+++ samba-2.2.5-modified/source/nsswitch/wb_common.c Wed Jul 31 15:54:10
2002
@@ -102,6 +102,7 @@
  static pid_t our_pid;
  struct stat st;
  pstring path;
+ int old_fd, result, flags;

  if (our_pid != getpid()) {
   close_sock();
@@ -159,6 +160,30 @@
   return -1;
  }

+ /* Make sure socket handle isn't stdin, stdout or stderr */
+
+ if (winbindd_fd < 3) {
+  old_fd = winbindd_fd;
+  if ((winbindd_fd = fcntl(winbindd_fd, F_DUPFD, 3)) == -1) {
+   winbindd_fd = old_fd;
+   close_sock();
+   return -1;
+  }
+  close( old_fd );
+ }
+
+ /* Socket should be closed on exec() */
+
+ result = flags = fcntl(winbindd_fd, F_GETFD, 0);
+ if (flags >= 0) {
+  flags != FD_CLOEXEC;
+  result = fcntl( winbindd_fd, F_SETFD, flags );
+ }
+ if (result < 0) {
+  close_sock();
+  return -1;
+ }
+
  if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
       sizeof(sunaddr)) == -1) {
   close_sock();
diff -u -x *.o -x *.so -x *.po
samba-2.2.5/source/nsswitch/winbind_nss_config.h
samba-2.2.5-modified/source/nsswitch/winbind_nss_config.h
--- samba-2.2.5/source/nsswitch/winbind_nss_config.h Wed Jun 19 03:13:44
2002
+++ samba-2.2.5-modified/source/nsswitch/winbind_nss_config.h Wed Jul 31
15:21:25 2002
@@ -62,6 +62,10 @@
 #include <string.h>
 #endif

+#ifdef HAVE_FCNTL_H
+#include <fcntl.h>
+#endif
+
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <errno.h>


Andrew Bartlett wrote:
> Hannes Schmidt wrote:
>>
>> Even if this is complete crap, I would appreciate any feedback
>> saying so.
>
> Firstly, don't expect instant responses - particularly over the
> weekend...
>
>> A couple of days ago I posted a problem report to the samba list
>> with some diagnostics. I seem to have found a
>> solution on my own and provide a patch here. The problem is related
>> to the fact that the winbind client code in
>> libnss_winbind.so leaves the handle to the winbind pipe open, for
>> obvious reasons. This is perfectly ok, but some
>> programs get confused if this handle is 0,1 or 2, which they expect
>> to be a handle to a different file. The patch
>> makes the winbind client (libnss_winbind.so) code more robust.
>>
>> Consider the following fragment of broken application code.
>>
>> if( fork() == 0 ) {
>>     /* child */
>>     close( 0 );             /* next open() will return 0 (stdin) */
>>     getpwent(...);        /* pid changed, so wb_common.c will
>> re-open the pipe to winbindd, the handle will be 0 */
>>     fd = open( ... );     /* returned fd is *not* 0 although the
>> code below assumes this */
>>     execve( ... );         /* executed program reads from stdin
>> which - unintentionally - is the winbind pipe */
>> } else {
>>     /* parent */
>>     ...
>> }
>>
>> Obviously, this code is broken and could be easily fixed by doing the
>> getpwent() before the close(). Unfortunately
>> the version of cron installed on my system is broken in a more
>> complex but similar way. There is probably more
>> broken code like this in other standard unix software, especially
>> since it's such a subtle mistake.
>
> It is rather - well spotted for figuring it out.
>
> How do other nss libraries deal with this?  NIS and LDAP both open
> sockets - find out if there is a standard solution to the problem.
> Personally, I don't like the idea of messing with a program's file
> descriptors - but this is a problem we will have to cope with.
>
>> The provided patch to wb_common.c makes sure that the handle to the
>> winbindd pipe is not 0, 1 or 2.
>
> For future reference, patches should be in 'diff -u' format, if
> possible.
>
>> *** /root/samba-2.2.5/source/nsswitch/wb_common.c Wed Jun 19
>> 03:13:44 2002 --- wb_common.c Thu Jul 25 16:36:39 2002
>> ***************
>> *** 94,99 ****
>> --- 94,120 ----
>>    }
>>   }
>>
>> + /* Returns a duplicate of the given file descriptor that is
>> guranteed +    not to be 0, 1 or 2 (stdin, stdout or stderr
>> respectively), unless +    an error occurs. If the given fd is
>> invalid, it will be returned +    unchanged. If a different error
>> occurs, the returned file handle +    will still be valid but it may
>> be 0, 1 or 2. If a valid file +    handle is returned and the
>> returned handle is different to the +    original one, the original
>> one will be closed. If dup() is broken, +    this function might
>> never return. */
>
> I don't like that idea.  It should fail - infinite loops are bad...
>
>> + int make_nonstd_fd(int fd)
>> + {
>> +  int saved_fd;
>> +  if (0 >= fd && fd <= 2) {
>> +      fd = dup(saved_fd = fd);
>> +      if (fd == -1) return saved_fd;
>> +      fd = make_nonstd_fd(fd);
>> +      close(saved_fd);
>> +  }
>> +  return fd;
>> + }
>> +
>>   /* Connect to winbindd socket */
>>
>>   int winbind_open_pipe_sock(void)
>> ***************
>> *** 158,164 ****
>>    if ((winbindd_fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>>     return -1;
>>    }
>> !
>>    if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
>>         sizeof(sunaddr)) == -1) {
>>     close_sock();
>> --- 179,188 ----
>>    if ((winbindd_fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>>     return -1;
>>    }
>> !
>> !  winbindd_fd = make_nonstd_fd( winbindd_fd );
>> !
>> !
>>    if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
>>         sizeof(sunaddr)) == -1) {
>>     close_sock();
>> ***************
>> *** 167,172 ****
>> --- 191,197 ----
>>
>>    return winbindd_fd;
>>   }
>> +
>>
>>   /* Write data to winbindd socket */