Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add function for proper escaping of assertion values in search filters #9

Open
majewsky opened this issue Apr 6, 2020 · 4 comments

Comments

@majewsky
Copy link

majewsky commented Apr 6, 2020

In the example proposed in #8, a search filter is constructed by

let filter = format!("(|(uid={})(mail={}))", who, who);

This is vulnerable to an LDAP injection. If an attacker manages to provide a carefully manufactured username like

let who = "doesnotmatter)(isMemberOf=cn=admins,ou=groups,ou=example,ou=org)(doesnotmatter="

they can query parts of the directory they're not supposed to see. In #8 (as of now), this could be exploited to learn about the structure of the directory because of an unrelated timing side-channel attack.

To make it easy to avoid such kinds of injections, the library should provide a function for proper escaping of assertion values in search filters. According to RFC 4515 (in section 3, implicit in the definition of the UTF1SUBSET production) at least the following escaping must be applied to assertion values in the string representation of a search filter:

  • The null byte (0x00) must be escaped as \00.
  • The left parenthesis must be escaped as \28.
  • The right parenthesis must be escaped as \29.
  • The asterisk must be escaped as \2a.
  • The backslash must be escaped as \5c.
  • Also, all non-ASCII bytes (0x80 - 0xFF) should be escaped as \xx, where xx is the hexadecimal value of the byte, but this is only a matter of correctness, not of security.
@JedimEmO
Copy link

JedimEmO commented Apr 6, 2020

I will look into this the next time I find the time to touch the code (maybe this weekend, have to stay indoors regardless :( ). Would you be willing to review the implementation when I get that far?

@majewsky
Copy link
Author

majewsky commented Apr 6, 2020

Sure, you can add me as a reviewer on the PR.

have to stay indoors regardless :(

That's my motivation, too. :)

JedimEmO added a commit to JedimEmO/rust-cldap that referenced this issue Apr 12, 2020
JedimEmO added a commit to JedimEmO/rust-cldap that referenced this issue Apr 12, 2020
…ping, and

to protect against timing attacks for user discovery
@JedimEmO
Copy link

I added a proposal for the function, called escape_filter_assertion_value. It basically calls escape_default on the incoming string, and then flat_maps the remaining characters to what FC 4515 specifies.

I also updated the example to use the new escape function on the who-parameter, and I also made it always call the simple bind (with empty username and password), as per your comment on timing attacks.

Here's the implementation:

pub fn escape_filter_assertion_value(input: &str) -> Result<String, LDAPError> {

Escape default docs: https://doc.rust-lang.org/std/ascii/fn.escape_default.html

JedimEmO added a commit to JedimEmO/rust-cldap that referenced this issue Apr 12, 2020
@JedimEmO
Copy link

Updated the function signature to return Vec instead of going through the String::from_utf8 call, now there is no error case to discuss :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants