diff --git a/auth/ldap_role_manager.cc b/auth/ldap_role_manager.cc index c01e830214..cc8f35ecdd 100644 --- a/auth/ldap_role_manager.cc +++ b/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; -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(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(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 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(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> ldap_role_manager::describe_role_grants() { diff --git a/auth/ldap_role_manager.hh b/auth/ldap_role_manager.hh index a68c9b96dc..908cc0ce1c 100644 --- a/auth/ldap_role_manager.hh +++ b/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); diff --git a/docs/operating-scylla/security/ldap-authorization.rst b/docs/operating-scylla/security/ldap-authorization.rst index 4c2d384dab..f35f57e855 100644 --- a/docs/operating-scylla/security/ldap-authorization.rst +++ b/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 -------- diff --git a/test/ldap/role_manager_test.cc b/test/ldap/role_manager_test.cc index bd2f51ab87..34124548c4 100644 --- a/test/ldap/role_manager_test.cc +++ b/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"); diff --git a/test/pylib/ldap_server.py b/test/pylib/ldap_server.py index ec0ddd5304..f0725ff24e 100644 --- a/test/pylib/ldap_server.py +++ b/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 """, ] diff --git a/test/resource/slapd.conf b/test/resource/slapd.conf index 1d13610367..f6505dd43d 100644 --- a/test/resource/slapd.conf +++ b/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"