inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Clarify where escaping is needed #12

Closed golddranks closed 7 years ago

golddranks commented 7 years ago

For example simple_bind provides fields for bind_dn and password. The docs of the escaping function ldap_escape talk only about filters. Do I need to escape bind_dn and password?

It would also be a very good practice to have a type representing "checked" values (a wrapper around a string), and by default, force the users to use that. Maybe we could have a function/macro that spits out static checked strings with placeholders, and provide an API for providing the values for the placeholders, that would be escaped.

inejge commented 7 years ago

For example simple_bind provides fields for bind_dn and password. The docs of the escaping function ldap_escape talk only about filters. Do I need to escape bind_dn and password?

The docs only talk about escaping in filter literals since that's the only place where it makes sense. You're right that it could be made more clear, and I'll append a note to that effect to ldap_escape()'s docs.

As for safe wrappers and autoescaping: it would certainly be possible to make a templating system akin to SQL prepared statements, or something ORM-like which would hide the filter strings entirely, but I'd prefer to keep the library outwardly compatible with existing practice, for now. (I'm aware of "LDAP injection" attacks, I just don't think they've been so numerous and harmful to justify compatibility-breaking measures.)

golddranks commented 7 years ago

Are you sure that distinguished names don't need escaping? There are libraries that escape them: https://www.npmjs.com/package/ldap-escape

As the DN can be something like uid=test_user,ou=Users,dc=myldapdomain,dc=myorg,dc=com, surely characters like "," must be considered as special characters?

inejge commented 7 years ago

Yes, DNs need escaping if they contain RDNs with characters that are special for DN syntax, as defined in RFC 4514. But that escaping is completely independent of the one used for filter literals. I haven't provided any DN-escaping functions in the library, since I treat DNs as opaque.

golddranks commented 7 years ago

If you get the DN as a whole, it can be considered as opaque, but if you construct it by yourself from information from unreliable sources, every "field" or attribute value needs to be escaped. I'm going to send a PR about this too, since it's not good if people just roll their own escaping mechanisms without consulting the RFC (and even then, it's better to have these collected under the relevant library, more eyes seeing the code etc.)

inejge commented 7 years ago

Right, but constructing a DN from unreliable input is quite rare. LDAP directories are mostly read, in which case the principal operation is a search, with a controlled base DN and escaped filters; DNs and DN-valued attributes arrive pre-escaped from the server. When a user-supplied value is used to create a directory entry, it's often something like a username, which has a restricted alphabet, verified beforehand.

Which is not to say that DN-escaping is pointless. I've merged your PR (thanks!)

inejge commented 7 years ago

After re-review: now that there's a DN-escaping function, all needed escaping mechanisms are provided, and I don't see the need to emphasize that other types of values may be used as-is (it's part of general LDAP knowledge, which I've already noted as a pre-requisite for using this crate). Hence I won't be amending ldap_escape() docs, and I'm closing this issue.