Merge pull request #16712 from rabbitmq/rabbitmq-server-16101-follow-up

A follow-up to #16101

Author: michaelklishin

deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap.erl

@@ -25,7 +25,8 @@
 -export([get_connections/0]).
 
 %% for tests
--export([purge_connections/0]).
+-export([purge_connections/0,
+         fill_user_dn_pattern/1, escaped_user_dn/1]).
 
 -define(L(F, A),  log("LDAP "         ++ F, A)).
 -define(L1(F, A), log("    LDAP "     ++ F, A)).
@@ -895,9 +896,7 @@ username_to_dn_prebind(Username) ->
               fun (LDAP) -> dn_lookup(Username, LDAP) end).
 
 username_to_dn(Username, LDAP,  postbind) -> dn_lookup(Username, LDAP);
-username_to_dn(Username, _LDAP, _When)    ->
-    ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
-    fill_dn(env(user_dn_pattern), [{username, Username}] ++ ADArgs).
+username_to_dn(Username, _LDAP, _When)    -> escaped_user_dn(Username).
 
 dn_lookup(Username, LDAP) ->
     Filled = fill_user_dn_pattern(Username),
@@ -916,7 +915,8 @@ dn_lookup(Username, LDAP) ->
             ?LOG_WARNING("Searching for DN for ~ts, got back ~tp",
                          [Filled, Entries],
                          #{domain => ?RMQLOG_DOMAIN_LDAP}),
-            Filled;
+            %% Used as a bind DN, so RFC 4514-escape substituted values.
+            escaped_user_dn(Username);
         {error, _} = E ->
             exit(E)
     end.
@@ -925,12 +925,17 @@ fill_user_dn_pattern(Username) ->
     ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
     fill(env(user_dn_pattern), [{username, Username}] ++ ADArgs).
 
+%% The user DN built from `user_dn_pattern' with RFC 4514-escaped
+%% substitutions. A no-op for usernames without DN-special characters.
+escaped_user_dn(Username) ->
+    ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
+    fill_dn(env(user_dn_pattern), [{username, Username}] ++ ADArgs).
+
 simple_bind_fill_pattern(Username) ->
     simple_bind_fill_pattern(env(user_bind_pattern), Username).
 
 simple_bind_fill_pattern(none, Username) ->
-    ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
-    fill_dn(env(user_dn_pattern), [{username, Username}] ++ ADArgs);
+    escaped_user_dn(Username);
 simple_bind_fill_pattern(Pattern, Username) ->
     ADArgs = rabbit_auth_backend_ldap_util:get_active_directory_args(Username),
     fill_dn(Pattern, [{username, Username}] ++ ADArgs).

deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap_util.erl

@@ -9,6 +9,9 @@
 
 -export([fill/2, get_active_directory_args/1, parse_query/1]).
 
+%% Raw substitution without escaping. Safe only for non-DN sinks (ACL string
+%% comparisons, eldap:equalityMatch/2 filter values). To build a DN, use
+%% rabbit_ldap_rfc4514:fill_dn/2, which RFC 4514-escapes substituted values.
 fill(Fmt, []) ->
     binary_to_list(iolist_to_binary(Fmt));
 

deps/rabbitmq_auth_backend_ldap/src/rabbit_ldap_rfc4514.erl

@@ -8,8 +8,8 @@
 %% RFC 4514 — String Representation of Distinguished Names.
 %%
 %% This module escapes values for safe inclusion in DN attribute value
-%% positions, preventing DN injection when untrusted input (usernames,
-%% vhost names, resource names) is substituted into DN templates.
+%% positions when input such as usernames, vhost names, or resource names is
+%% substituted into DN templates.
 -module(rabbit_ldap_rfc4514).
 
 -export([escape_value/1, fill_dn/2]).

deps/rabbitmq_auth_backend_ldap/test/unit_SUITE.erl

@@ -19,6 +19,7 @@ all() ->
      ad_fill,
      rfc4514_escape_value,
      rfc4514_fill_dn,
+     dn_lookup_fallback_dn_escaping,
      user_dn_pattern_gh_7161,
      format_different_types_of_ldap_attribute_values,
      ldap_log_domain_routing,
@@ -84,7 +85,7 @@ rfc4514_fill_dn(_Config) ->
     F = fun(Fmt, Args, Res) ->
                 ?assertEqual(Res, rabbit_ldap_rfc4514:fill_dn(Fmt, Args))
         end,
-    %% DN injection prevented
+    %% A comma in the substituted value is escaped
     F("cn=${username},ou=People", [{username, "user,ou=Evil"}],
       "cn=user\\,ou=Evil,ou=People"),
     %% user_dn is NOT escaped (it is already a complete DN)
@@ -97,6 +98,36 @@ rfc4514_fill_dn(_Config) ->
       "cn=x\\,y,dc=b"),
     ok.
 
+dn_lookup_fallback_dn_escaping(_Config) ->
+    PrevPattern = application:get_env(rabbitmq_auth_backend_ldap, user_dn_pattern),
+    PrevLog = application:get_env(rabbitmq_auth_backend_ldap, log),
+    ok = application:set_env(rabbitmq_auth_backend_ldap, log, false),
+    ok = application:set_env(rabbitmq_auth_backend_ldap, user_dn_pattern,
+                             "cn=${username},ou=People,dc=example,dc=com"),
+    try
+        %% No DN-special characters: escaping is a no-op
+        ?assertEqual(rabbit_auth_backend_ldap:fill_user_dn_pattern("alice"),
+                     rabbit_auth_backend_ldap:escaped_user_dn("alice")),
+        ?assertEqual("cn=alice,ou=People,dc=example,dc=com",
+                     rabbit_auth_backend_ldap:escaped_user_dn("alice")),
+        %% A comma in the substituted value is escaped
+        ?assertEqual("cn=evil\\,ou=admins,ou=People,dc=example,dc=com",
+                     rabbit_auth_backend_ldap:escaped_user_dn("evil,ou=admins")),
+        %% A binary username (the form used at runtime) is handled identically
+        ?assertEqual("cn=evil\\,ou=admins,ou=People,dc=example,dc=com",
+                     rabbit_auth_backend_ldap:escaped_user_dn(<<"evil,ou=admins">>)),
+        %% Bare fill leaves the substituted value unescaped
+        ?assertEqual("cn=evil,ou=admins,ou=People,dc=example,dc=com",
+                     rabbit_auth_backend_ldap:fill_user_dn_pattern("evil,ou=admins"))
+    after
+        restore_env(user_dn_pattern, PrevPattern),
+        restore_env(log, PrevLog)
+    end,
+    ok.
+
+restore_env(Key, {ok, V}) -> application:set_env(rabbitmq_auth_backend_ldap, Key, V);
+restore_env(Key, undefined) -> application:unset_env(rabbitmq_auth_backend_ldap, Key).
+
 ad_fill(_Config) ->
     F = fun(Fmt, Args, Res) ->
                 ?assertEqual(Res, rabbit_auth_backend_ldap_util:fill(Fmt, Args))