LDAP: escape DN template values, not full bind values

Project: rabbitmq/rabbitmq-server | Language: Erlang | Concepts: security, error-handling | Difficulty: mid
Link: https://github.com/rabbitmq/rabbitmq-server/pull/17278

The problem

LDAP authentication in RabbitMQ was escaping all bind values using RFC 4514 DN escaping, even when the bind value was not a DN (Distinguished Name). This caused issues with Active Directory and other LDAP setups where the username or bind value is not a DN but something like a UPN (user@domain) or a down-level logon name (DOMAIN\user). Escaping these non-DN values corrupted them, breaking authentication in common scenarios.

Why it's non-trivial

A naive fix would simply stop escaping bind values, but this would reintroduce security vulnerabilities like DN injection when the bind value really is a DN template. The code must distinguish between patterns that produce DNs (which need escaping) and those that do not (which must not be escaped), and apply escaping only where appropriate. The logic also needs to handle edge cases such as usernames with special characters or ambiguous separators.

The approach

The PR rewrites how and when escaping is applied, introducing a distinction between DN patterns and non-DN patterns.

1. Only escape values in DN templates, not all bind values

Previously, the code always escaped the username when filling patterns for binds. Now, it checks if the pattern is a DN (contains an = sign, as in attr=value), and only escapes values in that case:

fill_bind_dn(Pattern, Username) ->
    case is_dn_pattern(Pattern) of
        true  -> fill_dn_pattern(Pattern, Username, []);
        false -> fill_raw(Pattern, Username)
    end.

This code checks if the pattern is a DN (using is_dn_pattern/1). If so, it calls fill_dn_pattern/3 to apply escaping. Otherwise, it uses fill_raw/2, which does not escape.

2. Detect DN patterns by looking for an = character

is_dn_pattern(Pattern) ->
    lists:member($=, rabbit_data_coercion:to_list(Pattern)).

This function returns true if the pattern contains an equals sign, indicating an attr=value pair typical of a DN.

3. When not a DN, substitute values as-is

fill_raw(Pattern, Username) ->
    ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
    fill(Pattern, [{username, Username}] ++ ADArgs).

This function fills the pattern with the username and AD arguments, with no escaping.

4. When a DN, escape only the values being substituted

fill_dn_pattern(Pattern, Username, OtherArgs) ->
    ADArgs = safe_ad_args(
               rabbit_auth_backend_ldap_util:get_active_directory_args(Username)),
    fill_dn(Pattern, [{username, Username}] ++ ADArgs ++ OtherArgs).

This function prepares the arguments and uses fill_dn/3 to escape only the values being inserted into the DN template.

5. Remove old logic that always escaped the username

The old function that always escaped the username, even for non-DN patterns, is deleted:

-escaped_username_value(_Username, [{ad_domain, Domain}, {ad_user, User}]) ->
-    rabbit_ldap_rfc4514:escape_value(rabbit_data_coercion:to_list(Domain)) ++
-        "\\" ++ rabbit_ldap_rfc4514:escape_value(rabbit_data_coercion:to_list(User));
-escaped_username_value(Username, []) ->
-    rabbit_ldap_rfc4514:escape_value(Username).

This code is removed, so escaping is not applied unconditionally.

6. Add and update tests to verify correct escaping behavior

New and updated tests check that:

• DN patterns escape values (including special chars and backslashes).
• Non-DN patterns do not escape values.
• Supplying a full DN as username is not corrupted.
• Edge cases (e.g., usernames with commas, plus, spaces, unicode) are handled.

Example from the tests:

?assertEqual("O'Brien,Sean@example.com",
             rabbit_auth_backend_ldap:escaped_user_dn(<<"O'Brien,Sean">>)),

This test shows that when the pattern is not a DN, the username is not escaped.

From the review thread

No notable review discussion on this PR.

Takeaways

• Apply escaping only where it is semantically required. Escaping should be context-sensitive; DN escaping must only be used for DN templates, not for all bind values.
• Pattern detection is crucial for safe substitution. Before applying transformations or escaping, inspect the pattern to determine its type and required handling.
• Tests should cover both positive and negative cases. Include tests that verify correct behavior for both DN and non-DN patterns, as well as edge cases, to prevent regressions and security issues.