[LDB] was ldb_dn_build_child safer than ldb_dn_add_child_fmt?

[idra at samba.org]

On Wed, Dec 27, 2006 at 02:27:15PM +1100, Andrew Bartlett wrote:
> I dug up some old code recently, and came across one puzzling part of
> the change from ldb_dn_build_child(ctx, attribute, value, dn) to
> ldb_dn_add_child_fmt(dn, fmt, ...).
> 
> Almost all the callers of ldb_dn_add_child_fmt() simply wish to add
> exactly one element to the DN.  
> 
> Eg:  
> 
> ./rpc_server/lsa/dcesrv_lsa.c:836:      
> if ( ! ldb_dn_add_child_fmt(msg->dn, "cn=%s", name)) {
> 
> name in this case is the name of a newly trusted domain.  
> 
> In these cases, I think this API is over-flexible, in what it could
> allow an attacker.  The caller expects name to be
> 
> foo
> 
> But imagine name was to become:
> 
> foo,cn=users
> 
> The resultant DN would be cn=foo,cn=users,<base>.  My claim is that this
> extra DN component could allow an attacker to play games, etc...
> 
> I was wondering if it would be worth adding the old API
> (ldb_dn_build_child) back, to ensure that these elements cannot be
> misinterpreted?
> 
> Thoughts?

This is an internal API, there is no reason I can see we should care
about this kind of problems at this level. If you blindly accept user
input without validation from the caller, then there, in the caller, is
the problem.

If you expect the name to be a single elment you can change
it this way:
if ( ! ldb_dn_add_child_fmt(msg->dn, "cn=\"%s\"", name)) {
using quotes. but I would rather do some more checks in the caller.

In any case killing the old function was one of my prioirties as it was
too ugly to survive under many point of views.

Also please before changing any function in ldb_dn.c make really really
sure you understand how it works now as there are subtle implications
about speed and how data is transformed between the linearized and
exploded forms.

Simo.

-- 
Simo Sorce       idra at samba.org
-------------------------------
Samba Team http://www.samba.org