[SSSD] [PATCH] Sanitize ldap attributes in the config file

Sumit Bose sbose at redhat.com
Mon Nov 15 12:34:14 UTC 2010


On Mon, Nov 15, 2010 at 07:06:31AM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/15/2010 07:01 AM, Sumit Bose wrote:
> > On Fri, Nov 12, 2010 at 10:12:51AM -0500, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> https://fedorahosted.org/sssd/ticket/458
> >>
> >> Previously, it was possible to perform a sort of LDAP filter injection
> >> with careful crafting of the ldap attributes in the config file.
> >>
> >> This guarantees that any attribute specified in the config file is
> >> escaped properly, resulting in an inability to inject subfilters.
> >>
> >>
> >> Note: this was not a security issue, as it was editable only by root and
> >> even then, all checks were performed on the server, not the client.
> >>
> >> +
> >> +        if (name) {
> >> +            ret = sss_filter_sanitize(map, name, &map[i].name);
> >> +            talloc_zfree(name);
> > 
> > NACK, please check ret.
> > 
> > bye,
> > Sumit
> > 
> >> +        } else {
> >> +            map[i].name = NULL;
> >> +        }
> >> +
> 
> 
> ret is checked just after that else statement.
> 
>          if ((ret != EOK) || (map[i].def_name && !map[i].name)) {
> 

ah, sorry, I should have read the context. But after reading it I still
have comments:

@@ -50,7 +51,22 @@ int sdap_get_map(TALLOC_CTX *memctx,
         ret = confdb_get_string(cdb, map, conf_path,
                                 map[i].opt_name,
                                 map[i].def_name,
-                                &map[i].name);
+                                &name);
+        if (ret != EOK) {
+            DEBUG(0, ("Failed to retrieve value for %s\n", map[i].opt_name));
+            if (ret != EOK) {

this 'if'  is redundant

+                talloc_zfree(map);
+                return EINVAL;
+            }
+        }
+
+        if (name) {
+            ret = sss_filter_sanitize(map, name, &map[i].name);
+            talloc_zfree(name);
+        } else {
+            map[i].name = NULL;
+        }
+
         if ((ret != EOK) || (map[i].def_name && !map[i].name)) {
             DEBUG(0, ("Failed to retrieve value for %s\n", map[i].opt_name));

If I remember correctly there is no debugging output in
sss_filter_sanitize(), so I think it would be helpful to have a more
specific message if either the retrieval or the sanitation failed.

bye,
Sumit

             if (ret != EOK) {



> 
> 
> 
> - -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAkzhIkIACgkQeiVVYja6o6N1YwCdHrFW/4qDWGsmfzlDtw7hBXw8
> GZUAnjao6FzI4R8G2xuM31UbB+GnVgVJ
> =MPNC
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list