Merge 'auth: sanitize {USER} substitution in LDAP URL template' from Piotr Smaron

`LDAPRoleManager` interpolated usernames directly into `ldap_url_template`,
allowing LDAP filter injection and URL structure manipulation via crafted
usernames.

This PR adds two layers of encoding when substituting `{USER}`:
1. **RFC 4515 filter escaping** — neutralises `*`, `(`, `)`, `\`, NUL
2. **URL percent-encoding** — prevents `%`, `?`, `#` from breaking
   `ldap_url_parse`'s component splitting or undoing the filter escaping
It also adds `validate_query_template()` at startup to reject templates
that place `{USER}` outside the filter component (e.g. in the host or
base DN), where filter escaping would be the wrong defense.
Fixes: SCYLLADB-1309

Compatibility note:
Templates with `{USER}` in the host, base DN, attributes, or extensions
were previously silently accepted. They are now rejected at startup with
a descriptive error. Only templates with `{USER}` in the filter component
(after the third `?`) are valid.

Fixes: SCYLLADB-1309

Due to severeness, should be backported to all maintained versions.

Closes scylladb/scylladb#29388

* github.com:scylladb/scylladb:
  auth: sanitize {USER} substitution in LDAP URL templates
  test/ldap: add LDAP filter-injection reproducers

(cherry picked from commit aecb6b1d76)

Closes scylladb/scylladb#29495

auth/ldap_role_manager.cc

@@ -32,6 +32,8 @@ namespace {
 
 logger mylog{"ldap_role_manager"}; // `log` is taken by math.
 
+constexpr std::string_view user_placeholder = "{USER}";
+
 struct url_desc_deleter {
     void operator()(LDAPURLDesc *p) {
         ldap_free_urldesc(p);
@@ -40,9 +42,141 @@ struct url_desc_deleter {
 
 using url_desc_ptr = std::unique_ptr<LDAPURLDesc, url_desc_deleter>;
 
-url_desc_ptr parse_url(std::string_view url) {
+/// Escapes LDAP filter assertion value per RFC 4515 Section 3.
+/// The characters *, (, ), \, and NUL must be backslash-hex-escaped
+/// to prevent filter injection when interpolating untrusted input.
+sstring escape_filter_value(std::string_view value) {
+    size_t escapable_chars = 0;
+    for (unsigned char ch : value) {
+        switch (ch) {
+        case '*':
+        case '(':
+        case ')':
+        case '\\':
+        case '\0':
+            ++escapable_chars;
+            break;
+        default:
+            break;
+        }
+    }
+
+    if (escapable_chars == 0) {
+        return sstring(value);
+    }
+
+    sstring escaped(value.size() + escapable_chars * 2, 0);
+    size_t pos = 0;
+    for (unsigned char ch : value) {
+        switch (ch) {
+        case '*':
+            escaped[pos++] = '\\';
+            escaped[pos++] = '2';
+            escaped[pos++] = 'a';
+            break;
+        case '(':
+            escaped[pos++] = '\\';
+            escaped[pos++] = '2';
+            escaped[pos++] = '8';
+            break;
+        case ')':
+            escaped[pos++] = '\\';
+            escaped[pos++] = '2';
+            escaped[pos++] = '9';
+            break;
+        case '\\':
+            escaped[pos++] = '\\';
+            escaped[pos++] = '5';
+            escaped[pos++] = 'c';
+            break;
+        case '\0':
+            escaped[pos++] = '\\';
+            escaped[pos++] = '0';
+            escaped[pos++] = '0';
+            break;
+        default:
+            escaped[pos++] = static_cast<char>(ch);
+            break;
+        }
+    }
+
+    return escaped;
+}
+
+/// Percent-encodes characters that are not RFC 3986 "unreserved"
+/// (ALPHA / DIGIT / '-' / '.' / '_' / '~').
+///
+/// Uses explicit ASCII range checks instead of std::isalnum() because
+/// the latter is locale-dependent and could pass non-ASCII characters
+/// through unencoded under certain locale settings.
+///
+/// This is applied AFTER RFC 4515 filter escaping when the value is
+/// substituted into an LDAP URL.  It serves two purposes:
+///  1. Prevents URL-level metacharacters ('?', '#') from breaking
+///     the URL structure parsed by ldap_url_parse.
+///  2. Prevents percent-decoding (which ldap_url_parse performs on
+///     each component) from undoing the filter escaping, e.g. a
+///     literal "%2a" in the username would otherwise decode to '*'.
+sstring percent_encode_for_url(std::string_view value) {
+    static constexpr char hex[] = "0123456789ABCDEF";
+
+    size_t chars_to_encode = 0;
+    for (unsigned char ch : value) {
+        if (!((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9')
+                || ch == '-' || ch == '.' || ch == '_' || ch == '~')) {
+            ++chars_to_encode;
+        }
+    }
+
+    if (chars_to_encode == 0) {
+        return sstring(value);
+    }
+
+    sstring encoded(value.size() + chars_to_encode * 2, 0);
+    size_t pos = 0;
+    for (unsigned char ch : value) {
+        if ((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9')
+                || ch == '-' || ch == '.' || ch == '_' || ch == '~') {
+            encoded[pos++] = static_cast<char>(ch);
+        } else {
+            encoded[pos++] = '%';
+            encoded[pos++] = hex[ch >> 4];
+            encoded[pos++] = hex[ch & 0x0F];
+        }
+    }
+
+    return encoded;
+}
+
+/// Checks whether \p sentinel appears in any parsed URL component
+/// other than the filter (host, DN, attributes, extensions).
+bool sentinel_outside_filter(const LDAPURLDesc& desc, std::string_view sentinel) {
+    auto contains = [&](const char* field) {
+        return field && std::string_view(field).find(sentinel) != std::string_view::npos;
+    };
+    if (contains(desc.lud_host) || contains(desc.lud_dn)) {
+        return true;
+    }
+    if (desc.lud_attrs) {
+        for (int i = 0; desc.lud_attrs[i]; ++i) {
+            if (contains(desc.lud_attrs[i])) {
+                return true;
+            }
+        }
+    }
+    if (desc.lud_exts) {
+        for (int i = 0; desc.lud_exts[i]; ++i) {
+            if (contains(desc.lud_exts[i])) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+url_desc_ptr parse_url(const sstring& url) {
     LDAPURLDesc *desc = nullptr;
-    if (ldap_url_parse(url.data(), &desc)) {
+    if (ldap_url_parse(url.c_str(), &desc)) {
         mylog.error("error in ldap_url_parse({})", url);
     }
     return url_desc_ptr(desc);
@@ -115,6 +249,7 @@ const resource_set& ldap_role_manager::protected_resources() const {
 }
 
 future<> ldap_role_manager::start() {
+    validate_query_template();
     if (!parse_url(get_url("dummy-user"))) { // Just need host and port -- any user should do.
         return make_exception_future(
                 std::runtime_error(fmt::format("error getting LDAP server address from template {}", _query_template)));
@@ -199,7 +334,7 @@ future<> ldap_role_manager::revoke(std::string_view, std::string_view, ::service
 }
 
 future<role_set> ldap_role_manager::query_granted(std::string_view grantee_name, recursive_role_query) {
-    const auto url = get_url(grantee_name.data());
+    const auto url = get_url(grantee_name);
     auto desc = parse_url(url);
     if (!desc) {
         return make_exception_future<role_set>(std::runtime_error(format("Error parsing URL {}", url)));
@@ -331,7 +466,46 @@ future<> ldap_role_manager::remove_attribute(std::string_view role_name, std::st
 }
 
 sstring ldap_role_manager::get_url(std::string_view user) const {
-    return boost::replace_all_copy(_query_template, "{USER}", user);
+    // Two-layer encoding protects against injection:
+    // 1. RFC 4515 filter escaping neutralizes filter metacharacters (*, (, ), \, NUL)
+    // 2. URL percent-encoding prevents URL structure injection (?, #) and blocks
+    //    ldap_url_parse's percent-decoding from undoing the filter escaping (%2a -> *)
+    return boost::replace_all_copy(_query_template, user_placeholder,
+            percent_encode_for_url(escape_filter_value(user)));
+}
+
+void ldap_role_manager::validate_query_template() const {
+    if (_query_template.find(user_placeholder) == sstring::npos) {
+        return;
+    }
+
+    // Substitute {USER} with a sentinel and let ldap_url_parse tell us
+    // which URL component it landed in.  The sentinel is purely
+    // alphanumeric so it cannot affect URL parsing.
+    static constexpr std::string_view sentinel = "XLDAPSENTINELX";
+    sstring test_url = boost::replace_all_copy(_query_template, user_placeholder, sentinel);
+    auto desc = parse_url(test_url);
+    if (!desc) {
+        throw url_error(format("LDAP URL template is not a valid URL when {{USER}} is substituted: {}", _query_template));
+    }
+
+    // The sentinel must appear in the filter ...
+    if (!desc->lud_filter
+            || std::string_view(desc->lud_filter).find(sentinel) == std::string_view::npos) {
+        throw url_error(format(
+                "LDAP URL template places {{USER}} outside the filter component. "
+                "RFC 4515 filter escaping only protects the filter; other components "
+                "(e.g. the base DN) require different escaping and are not supported. "
+                "Template: {}", _query_template));
+    }
+    // ... and nowhere else (host, DN, attributes, extensions).
+    if (sentinel_outside_filter(*desc, sentinel)) {
+        throw url_error(format(
+                "LDAP URL template places {{USER}} outside the filter component. "
+                "RFC 4515 filter escaping only protects the filter; other components "
+                "(e.g. the host) require different escaping and are not supported. "
+                "Template: {}", _query_template));
+    }
 }
 
 future<std::vector<cql3::description>> ldap_role_manager::describe_role_grants() {

auth/ldap_role_manager.hh

@@ -107,6 +107,9 @@ class ldap_role_manager : public role_manager {
     /// Macro-expands _query_template, returning the result.
     sstring get_url(std::string_view user) const;
 
+    /// Validates that {USER}, if present, is used only in the LDAP filter component.
+    void validate_query_template() const;
+
     /// Used to auto-create roles returned by ldap.
     future<> create_role(std::string_view role_name);
 

docs/operating-scylla/security/ldap-authorization.rst

@@ -27,6 +27,16 @@ This configuration takes the form of a query template which is defined in the sc
 The value of ``ldap_url_template`` parameter should contain a valid LDAP URL (e.g., as returned by the ldapurl utility from OpenLDAP) representing an LDAP query that returns entries for all the user's roles.
 Scylla will replace the text ``{USER}`` in the URL with the user's Scylla username before querying LDAP.
 
+.. note:: Usernames substituted into ``{USER}`` are automatically escaped
+   using RFC 4515 filter escaping and URL percent-encoding, so LDAP filter
+   metacharacters (``*``, ``(``, ``)``, ``\``, NUL) and URL metacharacters
+   (``%``, ``?``, ``#``) in usernames are handled safely.
+   
+   ``{USER}`` must appear only in the **filter** component of the LDAP URL
+   (the part after the third ``?``).  Templates that place ``{USER}`` in the
+   host, base DN, attributes, or extensions are rejected at startup, because
+   filter escaping is not the correct encoding for those components.
+
 Workflow
 --------
 

test/ldap/role_manager_test.cc

@@ -277,6 +277,10 @@ const auto flaky_server_query_template = fmt::format(
         "ldap://localhost:{}/{}?cn?sub?(uniqueMember=uid={{USER}},ou=People,dc=example,dc=com)",
         std::stoi(ldap_port) + 2, base_dn);
 
+const auto member_uid_query_template = fmt::format(
+        "ldap://localhost:{}/dc=example,dc=com?cn?sub?(memberUid={{USER}})",
+        ldap_port);
+
 auto make_ldap_manager(cql_test_env& env, sstring query_template = default_query_template) {
     auto stop_role_manager = [] (auth::ldap_role_manager* m) {
         m->stop().get();
@@ -339,6 +343,148 @@ SEASTAR_TEST_CASE(ldap_wrong_role) {
     });
 }
 
+SEASTAR_TEST_CASE(ldap_filter_injection_with_wildcard_user) {
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, member_uid_query_template);
+        m->start().get();
+        do_with_mc(env, [&] (::service::group0_batch& b) {
+            m->create("*", auth::role_config{.is_superuser = false, .can_login = true}, b).get();
+            m->create("role1", auth::role_config{}, b).get();
+            m->create("role2", auth::role_config{.is_superuser = true, .can_login = false}, b).get();
+        });
+
+        // SCYLLADB-1309: Escaping keeps '*' literal, so it should not match any LDAP memberUid entries besides the local role itself.
+        const role_set expected{"*"};
+        BOOST_REQUIRE_EQUAL(expected, m->query_granted("*", auth::recursive_role_query::no).get());
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_filter_injection_with_parenthesis_payload) {
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, member_uid_query_template);
+        m->start().get();
+
+        // SCYLLADB-1309: With escaping, the payload becomes (memberUid=\29\28uid=\2a) - a valid filter
+        // that matches no real entry.  The user gets only its own role.
+        const sstring payload = ")(uid=*";
+        do_with_mc(env, [&] (::service::group0_batch& b) {
+            m->create(payload, auth::role_config{.is_superuser = false, .can_login = true}, b).get();
+            m->create("role1", auth::role_config{}, b).get();
+            m->create("role2", auth::role_config{.is_superuser = true, .can_login = false}, b).get();
+        });
+
+        const role_set expected{payload};
+        BOOST_REQUIRE_EQUAL(expected, m->query_granted(payload, auth::recursive_role_query::no).get());
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_filter_injection_with_percent_encoded_wildcard) {
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, member_uid_query_template);
+        m->start().get();
+
+        // SCYLLADB-1309: Without URL percent-encoding of '%', ldap_url_parse would
+        // percent-decode "%2a" -> '*', turning it into a wildcard that
+        // matches every entry.  The fix percent-encodes '%' -> "%25",
+        // so ldap_url_parse decodes "%252a" -> "%2a" (literal string).
+        const sstring payload = "%2a";
+        do_with_mc(env, [&] (::service::group0_batch& b) {
+            m->create(payload, auth::role_config{.is_superuser = false, .can_login = true}, b).get();
+            m->create("role1", auth::role_config{}, b).get();
+            m->create("role2", auth::role_config{.is_superuser = true, .can_login = false}, b).get();
+        });
+
+        const role_set expected{payload};
+        BOOST_REQUIRE_EQUAL(expected, m->query_granted(payload, auth::recursive_role_query::no).get());
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_filter_injection_with_question_mark_user) {
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, member_uid_query_template);
+        m->start().get();
+
+        // SCYLLADB-1309: '?' is the LDAP URL component separator.  Without URL
+        // percent-encoding, it would break ldap_url_parse's splitting,
+        // truncating the filter and potentially pushing the remainder
+        // into the extensions component.  The fix encodes '?' -> "%3F".
+        const sstring payload = "foo?bar";
+        do_with_mc(env, [&] (::service::group0_batch& b) {
+            m->create(payload, auth::role_config{.is_superuser = false, .can_login = true}, b).get();
+            m->create("role1", auth::role_config{}, b).get();
+            m->create("role2", auth::role_config{.is_superuser = true, .can_login = false}, b).get();
+        });
+
+        const role_set expected{payload};
+        BOOST_REQUIRE_EQUAL(expected, m->query_granted(payload, auth::recursive_role_query::no).get());
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_filter_injection_with_hash_fragment_user) {
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, member_uid_query_template);
+        m->start().get();
+
+        // SCYLLADB-1309: '#' introduces a URI fragment.  Without URL
+        // percent-encoding, ldap_url_parse would treat everything after '#' as
+        // a fragment and silently truncate the filter.  The fix encodes
+        // '#' -> "%23".
+        const sstring payload = "foo#bar";
+        do_with_mc(env, [&] (::service::group0_batch& b) {
+            m->create(payload, auth::role_config{.is_superuser = false, .can_login = true}, b).get();
+            m->create("role1", auth::role_config{}, b).get();
+            m->create("role2", auth::role_config{.is_superuser = true, .can_login = false}, b).get();
+        });
+
+        const role_set expected{payload};
+        BOOST_REQUIRE_EQUAL(expected, m->query_granted(payload, auth::recursive_role_query::no).get());
+    });
+}
+
+using exception_predicate::message_contains;
+
+SEASTAR_TEST_CASE(ldap_rejects_user_outside_filter_component) {
+    // SCYLLADB-1309: {USER} placed outside the filter component (in the base DN here) is a
+    // misconfiguration.  The substitution produces a working LDAP search rooted
+    // at the user-controlled DN, but RFC 4515 filter escaping is the wrong
+    // defense for DN values (which need RFC 4514 escaping for ',', '=', '+',
+    // etc.).  DN metacharacters in the username could restructure the base DN
+    // and redirect the search.  start() now rejects this configuration.
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, "ldap://localhost:5000/ou={USER},dc=example,dc=com?cn?sub?(objectClass=*)");
+        BOOST_REQUIRE_EXCEPTION(m->start().get(), auth::ldap_role_manager::url_error,
+                                message_contains("outside the filter component"));
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_rejects_user_in_host_component) {
+    // SCYLLADB-1309: {USER} in the host lets an attacker redirect the query to an arbitrary server.
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, "ldap://{USER}/dc=example,dc=com?cn?sub?(uid={USER})");
+        BOOST_REQUIRE_EXCEPTION(m->start().get(), auth::ldap_role_manager::url_error,
+                                message_contains("outside the filter component"));
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_rejects_user_in_attributes_component) {
+    // SCYLLADB-1309: {USER} in the attributes component would let an attacker control which
+    // attributes are returned, potentially leaking sensitive data from LDAP entries.
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, "ldap://localhost:5000/dc=example,dc=com?{USER}?sub?(uid=foo)");
+        BOOST_REQUIRE_EXCEPTION(m->start().get(), auth::ldap_role_manager::url_error,
+                                message_contains("outside the filter component"));
+    });
+}
+
+SEASTAR_TEST_CASE(ldap_rejects_user_in_extensions_component) {
+    // SCYLLADB-1309: {USER} in the extensions component could inject critical LDAP extensions.
+    return do_with_cql_env_thread([](cql_test_env& env) {
+        auto m = make_ldap_manager(env, "ldap://localhost:5000/dc=example,dc=com?cn?sub?(uid=foo)?!{USER}=bar");
+        BOOST_REQUIRE_EXCEPTION(m->start().get(), auth::ldap_role_manager::url_error,
+                                message_contains("outside the filter component"));
+    });
+}
+
 SEASTAR_TEST_CASE(ldap_reconnect) {
     return do_with_cql_env_thread([](cql_test_env& env) {
         auto m = make_ldap_manager(env, flaky_server_query_template);
@@ -355,8 +501,6 @@ SEASTAR_TEST_CASE(ldap_reconnect) {
     });
 }
 
-using exception_predicate::message_contains;
-
 SEASTAR_TEST_CASE(ldap_wrong_url) {
     return do_with_cql_env_thread([](cql_test_env& env) {
         auto m = make_ldap_manager(env, "wrong:/UR?L");

test/pylib/ldap_server.py

@@ -60,17 +60,24 @@ userid: jdoe
 userPassword: pa55w0rd
 """, """dn: cn=role1,dc=example,dc=com
 objectClass: groupOfUniqueNames
+objectClass: extensibleObject
 cn: role1
 uniqueMember: uid=jsmith,ou=People,dc=example,dc=com
 uniqueMember: uid=cassandra,ou=People,dc=example,dc=com
+memberUid: jsmith
+memberUid: cassandra
 """, """dn: cn=role2,dc=example,dc=com
 objectClass: groupOfUniqueNames
+objectClass: extensibleObject
 cn: role2
 uniqueMember: uid=cassandra,ou=People,dc=example,dc=com
+memberUid: cassandra
 """, """dn: cn=role3,dc=example,dc=com
 objectClass: groupOfUniqueNames
+objectClass: extensibleObject
 cn: role3
 uniqueMember: uid=jdoe,ou=People,dc=example,dc=com
+memberUid: jdoe
 """, ]
 
 

test/resource/slapd.conf

@@ -8,6 +8,8 @@ rootdn "cn=admin,cn=config"
 
 pidfile ./pidfile.pid
 include /etc/openldap/schema/core.schema
+include /etc/openldap/schema/cosine.schema
+include /etc/openldap/schema/nis.schema
 
 database mdb
 suffix "dc=example,dc=com"