Bug(s) fixed: 179137

Bug Description: recursion causes OOM with bad DN in dn2ancestor
Reviewed by: All (Thanks!)
Fix Description:
The fix looks scary, but I thought it would be best to get rid of
recursion entirely (ugh - recursion in a multi threaded server - this
isn't lisp . . .).  Along with eliminating recursion, I created a new
function called slapi_dn_find_parent that just returns a pointer to the
beginning of the parent of the given dn, rather than returning a copy
(as in slapi_dn_parent), to eliminate malloc/free in cases where it is
unnecessary such as iterating through the parents in an DN.  The new
function is basically just the guts of slapi_dn_parent with one twist,
specifically to address the bug in question - it skips through
consecutive runs of DN separator characters.  We should probably have a
function like const char *slapi_dn_is_valid(const char *) that returns
NULL if the given DN is valid or returns a pointer to the first invalid
character if not.  We could probably save a lot of time in processing
bad or malicious client requests.

Anyway, back to dn2ancestor.  The given ancestordn must contain the
_unnormalized_ parent DN, since some clients get irritated when they get
back an DN in a different form than given.  However, we need to have a
normalized DN to pass to dn2entry, and we cannot use a single Slapi_DN
that has both a dn and a ndn that are passed in byval (unless we add a
new API or skip the API altogether), so the variable ancestorndn holds
the normalized DN.  Using the original pointer to the given sdn also
allows us to avoid malloc/free entirely.
Platforms tested: Fedora Core 4
Flag Day: no
Doc impact: no
QA impact: should be covered by regular nightly and manual testing
New Tests integrated into TET: We need a test case that calls moddn and
modify operations with really bad DNs, consisting of nothing but
thousands of ',', '+', and '=' chars.

ldap/servers/slapd/back-ldbm/dn2entry.c

@@ -108,59 +108,13 @@ dn2entry(
 	return( e );
 }
 
-/*
- * dn2entry_or_ancestor - look up dn in the cache/indexes and return the
- * corresponding entry. If the entry is not found, this function returns NULL
- * and sets ancestordn to the DN of highest entry in the tree matched.
- *
- * ancestordn should be initialized before calling this function.
- * 
- * When the caller is finished with the entry returned, it should return it
- * to the cache:
- *  e = dn2entry_or_ancestor( ... );
- *  if ( NULL != e ) {
- *		cache_return( &inst->inst_cache, &e );
- *	}
- */
-struct backentry *
-dn2entry_or_ancestor(
-    Slapi_Backend	*be,
-    const Slapi_DN	*sdn,
-    Slapi_DN 	*ancestordn,
-    back_txn		*txn,
-    int			*err
-)
-{
-	struct backentry *e;
-
-	LDAPDebug( LDAP_DEBUG_TRACE, "=> dn2entry_or_ancestor \"%s\"\n", slapi_sdn_get_dn(sdn), 0, 0 );
-
-	/*
-	 * Fetch the entry asked for.
-	 */
-
-	e= dn2entry(be,sdn,txn,err);
-
-	if(e==NULL)
-	{
-		/*
-		 * could not find the entry named. crawl back up the dn and
-		 * stop at the first ancestor that does exist, or when we get
-		 * to the suffix.
-		 */
-		e= dn2ancestor(be,sdn,ancestordn,txn,err);
-	}
-
-	LDAPDebug( LDAP_DEBUG_TRACE, "<= dn2entry_or_ancestor %p\n", e, 0, 0 );
-	return( e );
-}
-
 /*
  * Use the DN to fetch the parent of the entry.
  * If the parent entry doesn't exist, keep working
  * up the DN until we hit "" or an backend suffix.
  *
- * ancestordn should be initialized before calling this function.
+ * ancestordn should be initialized before calling this function, and
+ * should be empty
  *
  * Returns NULL for no entry found.
  *
@@ -184,18 +138,64 @@ dn2ancestor(
 
 	LDAPDebug( LDAP_DEBUG_TRACE, "=> dn2ancestor \"%s\"\n", slapi_sdn_get_dn(sdn), 0, 0 );
 
-	/* stop when we get to "", or a backend suffix point */
-	slapi_sdn_done(ancestordn);	/* free any previous contents */
-    slapi_sdn_get_backend_parent(sdn,ancestordn,be);
-	if ( !slapi_sdn_isempty(ancestordn) )
-	{
-		Slapi_DN *newsdn = slapi_sdn_dup(ancestordn);
-		e = dn2entry_or_ancestor( be, newsdn, ancestordn, txn, err );
-		slapi_sdn_free(&newsdn);
-	}
+    /* first, check to see if the given sdn is empty or a root suffix of the
+       given backend - if so, it has no parent */
+    if (!slapi_sdn_isempty(sdn) && !slapi_be_issuffix( be, sdn )) {
+        Slapi_DN ancestorndn;
+        const char *ptr;
 
-	LDAPDebug( LDAP_DEBUG_TRACE, "<= dn2ancestor %p\n", e, 0, 0 );
-	return( e );
+        /* assign ancestordn to the parent of the given dn - ancestordn will contain
+           the "raw" unnormalized DN from the caller, so we can give back the DN
+           in the same format as we received it */
+        ptr = slapi_dn_find_parent(slapi_sdn_get_dn(sdn));
+        /* assign the ancestordn dn pointer to the parent of dn from sdn - sdn "owns"
+           the memory, but ancestordn points to it */
+        slapi_sdn_set_dn_byref(ancestordn, ptr); /* free any previous contents */
+        /* now, do the same for the normalized version */
+        /* ancestorndn holds the normalized version for iteration purposes and
+           because dn2entry needs the normalized dn */
+        ptr = slapi_dn_find_parent(slapi_sdn_get_ndn(sdn));
+        slapi_sdn_init_ndn_byref(&ancestorndn, ptr);
+
+        /*
+          At this point you may be wondering why I need both ancestorndn and
+          ancestordn.  Because, with the slapi_sdn interface, you cannot set both
+          the dn and ndn byref at the same time.  Whenever you call set_dn or set_ndn,
+          it calls slapi_sdn_done which wipes out the previous contents.  I suppose I
+          could have added another API to allow you to pass them both in.  Also, using
+          slapi_sdn_get_ndn(ancestordn) every time would result in making a copy then
+          normalizing the copy every time - not efficient.
+          So, why not just use a char* for the ancestorndn?  Because dn2entry requires
+          a Slapi_DN with the normalized dn.
+        */
+
+        /* stop when we get to "", or a backend suffix point */
+        while (!e && !slapi_sdn_isempty(&ancestorndn) && !slapi_be_issuffix( be, &ancestorndn )) {
+            /* find the entry - it uses the ndn, so no further conversion is necessary */
+            e= dn2entry(be,&ancestorndn,txn,err);
+            if (!e) {
+                /* not found, so set ancestordn to its parent and try again */
+                ptr = slapi_dn_find_parent(slapi_sdn_get_ndn(&ancestorndn));
+                /* keep in mind that ptr points to the raw ndn pointer inside
+                   ancestorndn which is still the ndn string "owned" by sdn, the
+                   original dn we started with - we are careful not to touch
+                   or change it */
+                slapi_sdn_set_ndn_byref(&ancestorndn, ptr); /* wipe out the previous contents */
+                /* now do the same for the unnormalized one */
+                ptr = slapi_dn_find_parent(slapi_sdn_get_dn(ancestordn));
+                slapi_sdn_set_dn_byref(ancestordn, ptr); /* wipe out the previous contents */
+            }
+        }
+
+        slapi_sdn_done(&ancestorndn);
+    }
+
+    /* post conditions:
+       e is the entry of the ancestor of sdn OR e is the suffix entry
+       OR e is NULL
+       ancestordn contains the unnormalized DN of e or is empty */
+    LDAPDebug( LDAP_DEBUG_TRACE, "<= dn2ancestor %p\n", e, 0, 0 );
+    return( e );
 }
 
 /*

ldap/servers/slapd/dn.c

@@ -593,8 +593,26 @@ slapi_dn_beparent(
 	return r;
 }
 
-char*
-slapi_dn_parent( const char *dn )
+/*
+ * This function is used for speed.  Instead of returning a newly allocated
+ * dn string that contains the parent, this function just returns a pointer
+ * to the address _within_ the given string where the parent dn of the
+ * given dn starts e.g. if you call this with "dc=example,dc=com", the
+ * function will return "dc=com" - that is, the char* returned will be the
+ * address of the 'd' after the ',' in "dc=example,dc=com".  This function
+ * also checks for bogus things like consecutive ocurrances of unquoted
+ * separators e.g. DNs like cn=foo,,,,,,,,,,,cn=bar,,,,,,,
+ * This function is useful for "interating" over a DN returning the ancestors
+ * of the given dn e.g.
+ *
+ * const char *dn = somedn;
+ * while (dn = slapi_dn_find_parent(dn)) {
+ *   see if parent exists
+ *   etc.
+ * }
+ */
+const char*
+slapi_dn_find_parent( const char *dn )
 {
 	const char *s;
 	int	inquote;
@@ -621,14 +639,34 @@ slapi_dn_parent( const char *dn )
 		} else {
 			if ( *s == '"' )
 				inquote = 1;
-			else if ( DNSEPARATOR( *s ) )
-				return( slapi_ch_strdup( s + 1 ) );
+			else {
+                if ( DNSEPARATOR( *s ) ) {
+                    while ( *s && DNSEPARATOR( *s ) ) {
+                        ++s;
+                    }
+                    if (*s) {
+                        return( s );
+                    }
+                }
+            }
 		}
 	}
 
 	return( NULL );
 }
 
+char*
+slapi_dn_parent( const char *dn )
+{
+	const char *s = slapi_dn_find_parent(dn);
+
+	if ( s == NULL || *s == '\0' ) {
+		return( NULL );
+	}
+
+    return( slapi_ch_strdup( s ) );
+}
+
 /*
  * slapi_dn_issuffix - tells whether suffix is a suffix of dn.  both dn
  * and suffix must be normalized.

ldap/servers/slapd/slapi-plugin.h

@@ -362,6 +362,7 @@ char *slapi_dn_normalize_to_end( char *dn, char *end );
 char *slapi_dn_ignore_case( char *dn );
 char *slapi_dn_normalize_case( char *dn );
 char *slapi_dn_beparent( Slapi_PBlock *pb, const char *dn );
+const char *slapi_dn_find_parent( const char *dn );
 char *slapi_dn_parent( const char *dn );
 int slapi_dn_issuffix( const char *dn, const char *suffix );
 int slapi_dn_isparent( const char *parentdn, const char *childdn );