Full_Name: Ted C. Cheng Version: HEAD OS: Solaris 8 URL: https://dl.dropboxusercontent.com/u/94235048/fix_send_cli_cred.patch Submission from: (NULL) (76.174.250.179) Importing fix from Symas #2230: On platforms, such as Solaris 8, that do not support functions for getting client credential, client sends a message and fd for server to derive euid/egid. The server-side logic for deriving client credential was buggy. This patch introduces sockaddr_un_cmp() to compare the socket path names, ignoring redundant "/" in the path, and only checks S_IFIFO mode. This patch has been tested on Solaris 8 and regression-tested fine on Solaris 10.
tedcheng@symas.com writes: > URL: https://dl.dropboxusercontent.com/u/94235048/fix_send_cli_cred.patch > > Importing fix from Symas #2230: > > On platforms, such as Solaris 8, that do not support functions for getting > client credential, client sends a message and fd for server to derive euid/egid. > The server-side logic for deriving client credential was buggy. > > This patch introduces sockaddr_un_cmp() to compare the socket path names, > ignoring redundant "/" in the path, and only checks S_IFIFO mode. This patch has > been tested on Solaris 8 and regression-tested fine on Solaris 10. This patch is wrong: - You cannot trust the path which the client wrote to the socket *by hand* (label sendcred: in libldap/os-local.c:ldap_pvt_connect()), if someone else than the owner and root has write access to the socket. Could comment that in getpeereid.c, to prevent such a future patch. - An initial "//" is significant on some systems and must not match "/". Posix does not guarantee that sun_path[] is \0-terminated. What problem is the complicated compare solving? Why not require that the user spells the path like the server does, similar to how TLS hostnames must be spelled the same way? You are not catching all possible spellings of the path anyway: YOu do not reduce "/./" to "/" and must not reduce "foo/../ to "" since foo can be a symlink. -- Hallvard
Looking again at the st_mode compare: I'm pretty sure the old code worked somewhere, at some time. Maybe somewhere it still does, and expecting a different fstat():st_mode will break that. Unless something changed in OpenLDAP itself? Anyway, after re-reading ITS#4893 I'd be very wary about tweaking this code. Looks like at least I just gave up at the end, but followup#9 suggests using Solaris "doors". IIRC I did look at at the supposedly more general STREAMS pipes and found myself wading into portability issues or kernel tuning params or whateveer. -- Hallvard
On Apr 13, 2013, at 12:24 AM, Hallvard Breien Furuseth wrote: > Looking again at the st_mode compare: I'm pretty sure the old > code worked somewhere, at some time. Maybe somewhere it still > does, and expecting a different fstat():st_mode will break that. > Unless something changed in OpenLDAP itself? > > Anyway, after re-reading ITS#4893 I'd be very wary about tweaking > this code. Looks like at least I just gave up at the end, but > followup#9 suggests using Solaris "doors". IIRC I did look at at > the supposedly more general STREAMS pipes and found myself wading > into portability issues or kernel tuning params or whateveer. > > -- > Hallvard There are several issues on Solaris 8: 1. getsockname(s, (struct sockaddr *)&lname, &llen) The llen parameter does not return the actual size of the socket address, but the original buffer size. As a result, the comparison !memcmp(&lname, &rname, llen) compares the whole buffer. Since rname is not initialized to 0 for the entire buffer, the garbage characters following the socket path name always fail the comparison. + if( err == 0 && st.st_mode == mode && llen == rlen + && !memcmp(&lname, &rname, llen)) 2. The checking "llen == rlen" always fails as well. 3. The fchmod( fds[0], S_ISUID|S_IRWXU) on the client side does not change the mode to include S_ISUID|S_IRWXU. The mode stays as only S_IFIFO: (gdb) next 81 if (rc = fchmod( fds[0], S_ISUID|S_IRWXU ) < 0) { (gdb) print st $2 = {...st_mode = 4096, ...} (gdb) next 85 (void) fstat(fds[0], &st); (gdb) next 86 write( fds[1], sa, salen ); (gdb) print st $3 = {...st_mode = 4096, ...} Trying to compare (st.st_mode == S_IFIFO|S_ISUID|S_IRWXU) always fails. I have revised the patch to (1) initialize rname buffer, (2) ignore the comparison "llen == rlen", (3) compare st.st_mode only against S_IFIFO, but not S_ISUID|S_IRWXU. The socket path comparison demands full-path match, e.g., "/var/suum/run/socket" won't match against "/var/suum/run//socket". The revised patch is at: https://dl.dropboxusercontent.com/u/94235048/ITS7575.patch Ted C. Cheng Symas Corporation
tedcheng@symas.com wrote: > On Apr 13, 2013, at 12:24 AM, Hallvard Breien Furuseth wrote: > >> Looking again at the st_mode compare: I'm pretty sure the old >> code worked somewhere, at some time. Maybe somewhere it still >> does, and expecting a different fstat():st_mode will break that. >> Unless something changed in OpenLDAP itself? >> >> Anyway, after re-reading ITS#4893 I'd be very wary about tweaking >> this code. Looks like at least I just gave up at the end, but >> followup#9 suggests using Solaris "doors". IIRC I did look at at >> the supposedly more general STREAMS pipes and found myself wading >> into portability issues or kernel tuning params or whateveer. >> >> -- >> Hallvard > > > There are several issues on Solaris 8: > > 1. getsockname(s, (struct sockaddr *)&lname, &llen) > > The llen parameter does not return the actual size of the socket address, > but the original buffer size. As a result, the comparison > !memcmp(&lname, &rname, llen) compares the whole buffer. > Since rname is not initialized to 0 for the entire buffer, the garbage > characters following the socket path name always fail the comparison. > > + if( err == 0 && st.st_mode == mode && llen == rlen > + && !memcmp(&lname, &rname, llen)) > > 2. The checking "llen == rlen" always fails as well. > > 3. The fchmod( fds[0], S_ISUID|S_IRWXU) on the client side does not change > the mode to include S_ISUID|S_IRWXU. The mode stays as only > S_IFIFO: > > (gdb) next > 81 if (rc = fchmod( fds[0], S_ISUID|S_IRWXU ) < 0) { > (gdb) print st > $2 = {...st_mode = 4096, ...} > (gdb) next > 85 (void) fstat(fds[0], &st); > (gdb) next > 86 write( fds[1], sa, salen ); > (gdb) print st > $3 = {...st_mode = 4096, ...} > > Trying to compare (st.st_mode == S_IFIFO|S_ISUID|S_IRWXU) > always fails. As Hallvard points out, all of this code was known to work at one time. Apparently a subsequent Solaris patchlevel has broken it. I don't see a portable way to conditionalize this behavior now. > I have revised the patch to (1) initialize rname buffer, (2) ignore > the comparison "llen == rlen", (3) compare st.st_mode only against > S_IFIFO, but not S_ISUID|S_IRWXU. > > The socket path comparison demands full-path match, e.g., "/var/suum/run/socket" > won't match against "/var/suum/run//socket". Nor should it. In this case whoever is using "//" in the path is misconfigured, as I already said before in private email. > > The revised patch is at: > https://dl.dropboxusercontent.com/u/94235048/ITS7575.patch > > Ted C. Cheng > Symas Corporation -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Ted C. Cheng writes: > I have revised the patch to (1) initialize rname buffer, (2) ignore > the comparison "llen == rlen", Might be OK, but... > (3) compare st.st_mode only against S_IFIFO, but not S_ISUID|S_IRWXU. No. You're getting proper use to work on Solaris, but that's the easy part. That fchmod and corresponding mode test is there for a reason: The code must also reject attempts to impersonate other users or groups. A user could find and open a named socket/pipe owned by some other user, or start someone's setuid program which opens a Unix domain socket and then runs his program as his own user, or something like that. I don't remember exactly. But OSes and OS releases differ about which of these impersonation attempts might work if this code were insufficiently paranoid, so you have quite some exporation work ahead of you if you want to tweak this FD passing safely. You can instead look for a mechanism with built-in credential passing, apparently like Solaris "doors". Or look at what some other well-tested and portable package does and suggest we steal its code. Or live with the fact that SASL/EXTERNAL over ldapi:// is supported on your platform. > The socket path comparison demands full-path match, e.g., > "/var/suum/run/socket" won't match against "/var/suum/run//socket". -- Hallvard
On Apr 14, 2013, at 6:39 AM, Hallvard Breien Furuseth wrote: > > You can instead look for a mechanism with built-in credential passing, > apparently like Solaris "doors". The sample client-server programs, see link below, show an experiment on Solaris 8 that server creates and listens to door calls, while client invokes them. When client invokes a door_call, server gets the euid and egid, among others, of the client: https://dl.dropboxusercontent.com/u/94235048/door_call.tgz http://docs.oracle.com/cd/E18752_01/html/816-5171/door-call-3door.html # ./server successfully created a door euid (101) egid (1) ruid (101) rgid (1) pid (8947) $ id uid=101(tedcheng) gid=1(other) $ ./client pid (8947): door_call succeeded There is the situation in which we are sending/getting client credentials from a door call through say /tmp/door, while service requests, such as nssov/nslcd (nss-pam-ldapd), through a separate Unix domain socket. There is therefore the need to tie client credentials with their respective (name) service requests; "doors" implements its own threading support. The work to integrate doors for client credential support into a server with threading support, such as slapd, may get complicated fast. "Doors" does not seem to be a feasible solution for sending client credentials in a context such as nssov/slapd. > Or look at what some other well-tested > and portable package does and suggest we steal its code. > This may be the only option, if there exists one, for older-system support (Solaris 8). Ted C. Cheng Symas Corporation
changed notes moved from Incoming to Software Bugs
Review if this should just be abandoned, since it was dealing with the expired solaris 8. Solaris 8 is long dead
changed notes changed state Open to Closed