Re: (ITS#4750) libldap initialization of ~/.ldaprc and setuid

[Kurt@OpenLDAP.org]

It was my intent when ldap.conf(5) was designed that it only
provide defaults when selected by the user.  For instance,
a hostname when the user provides none.   While libldap
always read ldap.conf/.ldaprc, that wasn't considered a big
deal.  However, with various init options which are used
without program selection, programs need now to provide an
option to disable initialization.  Ugh.

I have noted that if I had to do it all over again, libldap
would never read ldap.conf(5).  Instead the program would
have to separately call out to facility to obtain defaults.

Some of RRA suggests are not workable.  For instance, as
now some options can only be set via ldap.conf(5)/.ldaprc,
disabling it for root disallows root to use all features
of programs and the library.

But this is all water under the bridge.  The library is what it
is.  Changing it would likely break other things.

I note that nss/pam-ldap setting NOINIT (or otherwise mucking
with libldap options) might break LDAP-enabled programs.  But
that's another matter.

Anyways, I think the only good fix (for this and many other
larger problems) is a library redesign/rewrite.

Kurt

At 08:04 PM 11/13/2006, quanah@stanford.edu wrote:
>Full_Name: Quanah Gibson-Mount
>Version: All
>OS: NA
>URL: 
>Submission from: (NULL) (171.66.155.86)
>
>
>The debian folks committed the following patch:
>
>Modified: openldap/trunk-2.1/libraries/libldap/init.c
>===================================================================
>--- openldap/trunk-2.1/libraries/libldap/init.c 2006-11-12 10:08:20 UTC (rev
>749)
>+++ openldap/trunk-2.1/libraries/libldap/init.c 2006-11-14 00:54:38 UTC (rev
>750)
>@@ -252,6 +252,12 @@
>        char *home;
>        char *path = NULL;
> 
>+       if (getuid() != geteuid()) {
>+               /* Caller is setuid -- don't read any per-user configs in
>+                  these circumstances, as this may not be safe */
>+               return;
>+       }
>+
>        if (file == NULL) {
>                /* no file name */
>                return;
>
>
>You can see this is against 2.1, but the code is essentially the same in OL
>HEAD.
>
>The reason was:
>
>+  * Don't check for user configuration files when the caller is setuid;
>+    addresses #387467, which is a potential security hole allowing
>+    libnss-ldap settings to be overridden.  Thanks to Stephen Frost for
>+    bringing this to my attention.
>
>
>which Howard note is not really the right way to do this.
>
>[19:07] Howard Chu: that's the wrong fix
>[19:07] Howard Chu: libnss-ldap should set NOINIT for its own usage.
>[19:09] Quanah: so this patch doesn't really fix anything?
>[19:09] Howard Chu: probably not.
>
>
>However, one of my co-workers notes:
>
>
>19:48 <rra@home> Well, I appreciate Howard's opinion here, but that's a crappy
>fix because it means that every single binary that uses the LDAP libraries
>    and may be setuid has to know to do this.
>19:49 <rra@home> libnss-ldap should *also* set NOINIT, of course.
>19:50 <rra@home> It sounded like it was reading dotfiles out of the user's home
>directory, though.
>19:50 <rra@home> Is that really what it's doing?
>19:50 <rra@home> Something along those lines?
>19:50 <Q> http://www.openldap.org/devel/cvsweb.cgi/libraries/libldap/init.c?hideattic=1&sortbydate=0
>19:50 <Q> it can read .ldaprc, yes
>19:50 <rra@home> Libraries reading dotfiles is all by itself pretty evil.
>19:51 <rra@home> That to me violates abstraction layers.
>19:51 <rra@home> Applications are not going to expect libraries to do that.
>19:51 <rra@home> The *right* solution would to me be to not do that; failing
>that, I think OpenLDAP should bear the responsibility to do it securely, which
>    would include not reading user .ldaprc files when running as root.  (There
>are other ways to prevent that, of course.)
>19:55 <rra@home> Basically, my argument is more that there should be a mode that
>loads user configuration, but it probably shouldn't be the default
>    interface.
>19:55 <rra@home> Anyway, Howard's certainly right that libnss-ldap needs to be
>fixed regardless to pass that flag.
>
>
>So, thoughts appreciated.
>
>--Quanah