This is an automated email from the ASF dual-hosted git repository.
jongyoul pushed a commit to branch branch-0.12
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/branch-0.12 by this push:
new c560f3b819 [ZEPPELIN-6599] Stop treating the user list search text as
LDAP filter and regex syntax
c560f3b819 is described below
commit c560f3b8190329e64abdc4ce1bfa43d4ffbb0682
Author: κΉμλ <[email protected]>
AuthorDate: Thu Aug 6 17:12:05 2026 +0900
[ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and
regex syntax
### What is this PR for?
`GET /api/security/userlist/{searchText}` interprets the client-supplied
search text as syntax in two places. This PR fixes both.
**1. LDAP search filters**
`ShiroAuthenticationService` builds the LDAP search filter by string
concatenation, in `getUserList(DefaultLdapRealm, String, int)` and
`getUserList(LdapRealm, String, int)`. The filter metacharacters `(`, `)`, `*`,
`\` and NUL are therefore interpreted as filter syntax instead of literal text.
Both sites now go through `LdapFilterEncoder.escapeFilterValue`, the RFC
4515 escape utility that `ActiveDirectoryGroupRealm` already uses. The filter
building is extracted into two small helpers so the rendered filter can be
asserted directly in unit tests, following the `expandFilterTemplate` approach
already used in `LdapRealm`.
The wildcards that Zeppelin adds around the search text stay outside the
escaped value, so substring matching behaves exactly as before. Only an
asterisk typed by the user becomes a literal character. The configured
attribute name and object class are escaped as well for defense in depth, while
the raw attribute name is still used for `setReturningAttributes`.
The remaining realm branches of `getMatchedUsers` are already safe:
`ActiveDirectoryGroupRealm` escapes the value in `searchForUserName`, and the
`JdbcRealm` branch uses a `PreparedStatement` with a validated identifier.
**2. Regular expression in the sorting comparator**
`SecurityRestApi` sorted the matched users with `o1.matches(searchText +
"(.*)")`, which compiles the search text as a regular expression. A search text
of `*` therefore fails with `PatternSyntaxException: Dangling meta character
'*'` and the endpoint responds with HTTP 500. That comparator only wants to
list the users whose name starts with the search text first, which `startsWith`
does without compiling anything. The replacement is a consistent comparator as
well, and the alphabetic [...]
### What type of PR is it?
Improvement
### Todos
* [x] Escape the search text at both LDAP filter building sites
* [x] Unit tests for the rendered filters (16)
* [x] Sort the user list without compiling the search text as a regular
expression
* [x] Endpoint test for search texts containing regex metacharacters
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6599
### How should this be tested?
```
./mvnw test -pl zeppelin-server
-Dtest=ShiroAuthenticationServiceFilterInjectionTest,SecurityRestApiTest
```
`SecurityRestApiTest.testGetUserListWithRegexMetacharacters` returns HTTP
500 without the comparator change and HTTP 200 with it. The existing LDAP realm
and Shiro service tests keep passing.
The LDAP part was also verified manually against an in-memory LDAP server.
With the search text `*)(cn=Bob`, the directory server received
`(&(objectclass=person)(uid=*)(cn=Bob*))` before this change, so the `uid`
condition was neutralized and an extra condition was appended. After this
change the same input arrives as
`(&(objectclass=person)(uid=*\2a\29\28cn=Bob*))`, while a normal search text
produces exactly the same filter and the same results as before.
Worth noting that the entries matched by an injected filter are not
disclosed in the REST response today, because `SecurityRestApi` filters the
returned list once more with `containsIgnoreCase(user, searchText)`. So the
LDAP part is hardening of the filter building rather than a fix for an
information leak.
### Screenshots (if appropriate)
Not applicable.
### Questions
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5384 from kimyenac/ZEPPELIN-6599.
Signed-off-by: Jongyoul Lee <[email protected]>
(cherry picked from commit b90e68634b0f2b6e389a84925ea0657457253a9f)
Signed-off-by: Jongyoul Lee <[email protected]>
---
.../org/apache/zeppelin/rest/SecurityRestApi.java | 15 +-
.../service/ShiroAuthenticationService.java | 34 ++++-
.../apache/zeppelin/rest/SecurityRestApiTest.java | 17 +++
...roAuthenticationServiceFilterInjectionTest.java | 170 +++++++++++++++++++++
4 files changed, 218 insertions(+), 18 deletions(-)
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java
index 89bc317734..5eec6e7713 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java
@@ -18,6 +18,7 @@ package org.apache.zeppelin.rest;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -102,16 +103,10 @@ public class SecurityRestApi extends AbstractRestApi {
List<String> autoSuggestRoleList = new ArrayList<>();
Collections.sort(usersList);
Collections.sort(rolesList);
- Collections.sort(
- usersList,
- (o1, o2) -> {
- if (o1.matches(searchText + "(.*)") && o2.matches(searchText +
"(.*)")) {
- return 0;
- } else if (o1.matches(searchText + "(.*)")) {
- return -1;
- }
- return 0;
- });
+ // List the users whose name starts with the search text first, keeping
the alphabetical order
+ // within each group. The search text comes from the client, so it must
not be compiled as a
+ // regular expression here.
+ usersList.sort(Comparator.comparing((String user) ->
!user.startsWith(searchText)));
int maxLength = 0;
for (String user : usersList) {
if (StringUtils.containsIgnoreCase(user, searchText)) {
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
index 21219c6e2e..3797624a01 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
@@ -53,6 +53,7 @@ import org.apache.shiro.util.JdbcUtils;
import org.apache.shiro.util.ThreadContext;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
import org.apache.zeppelin.realm.ActiveDirectoryGroupRealm;
+import org.apache.zeppelin.realm.LdapFilterEncoder;
import org.apache.zeppelin.realm.LdapRealm;
import org.apache.zeppelin.realm.jwt.KnoxJwtRealm;
import org.slf4j.Logger;
@@ -315,7 +316,7 @@ public class ShiroAuthenticationService implements
AuthenticationService {
String[] attrIDs = {userDnPrefix};
constraints.setReturningAttributes(attrIDs);
NamingEnumeration<SearchResult> result =
- ctx.search(userDnSuffix, "(" + userDnPrefix + "=*" + searchText +
"*)", constraints);
+ ctx.search(userDnSuffix, buildUserSearchFilter(userDnPrefix,
searchText), constraints);
while (result.hasMore()) {
Attributes attrs = result.next().getAttributes();
if (attrs.get(userDnPrefix) != null) {
@@ -330,6 +331,17 @@ public class ShiroAuthenticationService implements
AuthenticationService {
return userList;
}
+ /**
+ * Builds the user search filter for {@link DefaultLdapRealm}. The attribute
name and the search
+ * text are escaped per RFC 4515; the wildcards Zeppelin adds around the
search text stay outside
+ * the escaped value so that substring matching keeps working.
+ */
+ static String buildUserSearchFilter(String userDnPrefix, String searchText) {
+ return String.format("(%s=*%s*)",
+ LdapFilterEncoder.escapeFilterValue(userDnPrefix),
+ LdapFilterEncoder.escapeFilterValue(searchText));
+ }
+
/** Function to extract users from Zeppelin LdapRealm. */
private List<String> getUserList(LdapRealm r, String searchText, int
numUsersToFetch) {
List<String> userList = new ArrayList<>();
@@ -348,13 +360,7 @@ public class ShiroAuthenticationService implements
AuthenticationService {
NamingEnumeration<SearchResult> result =
ctx.search(
userSearchRealm,
- "(&(objectclass="
- + userObjectClass
- + ")("
- + userAttribute
- + "=*"
- + searchText
- + "*))",
+ buildUserSearchFilterWithObjectClass(userObjectClass,
userAttribute, searchText),
constraints);
while (result.hasMore()) {
Attributes attrs = result.next().getAttributes();
@@ -377,6 +383,18 @@ public class ShiroAuthenticationService implements
AuthenticationService {
return userList;
}
+ /**
+ * Builds the user search filter for Zeppelin {@link LdapRealm}. Follows the
same escaping rules
+ * as {@link #buildUserSearchFilter(String, String)}, with the user object
class escaped as well.
+ */
+ static String buildUserSearchFilterWithObjectClass(String userObjectClass,
String userAttribute,
+ String searchText) {
+ return String.format("(&(objectclass=%s)(%s=*%s*))",
+ LdapFilterEncoder.escapeFilterValue(userObjectClass),
+ LdapFilterEncoder.escapeFilterValue(userAttribute),
+ LdapFilterEncoder.escapeFilterValue(searchText));
+ }
+
/**
* * Get user roles from shiro.ini for Zeppelin LdapRealm.
*
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java
index b7e01b1fec..25912b063f 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java
@@ -91,6 +91,23 @@ class SecurityRestApiTest extends AbstractTestRestApi {
notUser.close();
}
+ @Test
+ void testGetUserListWithRegexMetacharacters() throws IOException {
+ // The search text is not a regular expression. Metacharacters must not
break the endpoint.
+ for (String searchText : new String[] {"%2A", "%28", "%2B"}) {
+ CloseableHttpResponse get = httpGet("/security/userlist/" + searchText,
"admin", "password1");
+ assertThat("Status code for search text " + searchText,
+ get.getStatusLine().getStatusCode(), CoreMatchers.equalTo(200));
+ Map<String, Object> resp = gson.fromJson(
+ EntityUtils.toString(get.getEntity(), StandardCharsets.UTF_8),
+ new TypeToken<Map<String, Object>>(){}.getType());
+ List<String> userList = (List) ((Map) resp.get("body")).get("users");
+ assertThat("Search result size for search text " + searchText,
userList.size(),
+ CoreMatchers.equalTo(0));
+ get.close();
+ }
+ }
+
@Test
void testRolesEscaped() throws IOException {
CloseableHttpResponse get = httpGet("/security/ticket", "admin",
"password1");
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java
new file mode 100644
index 0000000000..14dca43b90
--- /dev/null
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zeppelin.service;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * Tests verifying that the search text supplied to
+ * {@code GET /api/security/userlist/{searchText}} cannot inject LDAP filter
metacharacters into
+ * the filters that {@link ShiroAuthenticationService} builds for the LDAP
realms. The rendered
+ * filter must never contain unescaped {@code (}, {@code )} or {@code *}
characters that
+ * originated in the search text.
+ */
+class ShiroAuthenticationServiceFilterInjectionTest {
+
+ private static final String USER_ATTRIBUTE = "uid";
+ private static final String USER_OBJECT_CLASS = "person";
+
+ // "(uid=*%s*)" contributes 1 '(', 1 ')' and the 2 wildcards Zeppelin adds
itself.
+ private static final int DEFAULT_LDAP_OPEN_PARENS = 1;
+ private static final int DEFAULT_LDAP_CLOSE_PARENS = 1;
+ private static final int DEFAULT_LDAP_ASTERISKS = 2;
+
+ // "(&(objectclass=person)(uid=*%s*))" contributes 3 '(', 3 ')' and the same
2 wildcards.
+ private static final int LDAP_REALM_OPEN_PARENS = 3;
+ private static final int LDAP_REALM_CLOSE_PARENS = 3;
+ private static final int LDAP_REALM_ASTERISKS = 2;
+
+ static Stream<String> injectionPayloads() {
+ return Stream.of(
+ ")(uid=*",
+ "admin)(|(uid=*",
+ "*",
+ "admin)(cn=a*",
+ ")(mail=*@corp.com",
+ "alice)(userPassword=*",
+ "alice\\",
+ "alice\\2a",
+ "alice\\29\\28uid=\\2a",
+ "\\",
+ "\0");
+ }
+
+ @ParameterizedTest
+ @MethodSource("injectionPayloads")
+ void defaultLdapRealmFilterNeutralizesPayload(String payload) {
+ String rendered =
ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE, payload);
+
+ assertMetacharacterCounts(rendered, payload,
+ DEFAULT_LDAP_OPEN_PARENS, DEFAULT_LDAP_CLOSE_PARENS,
DEFAULT_LDAP_ASTERISKS);
+ }
+
+ @ParameterizedTest
+ @MethodSource("injectionPayloads")
+ void ldapRealmFilterNeutralizesPayload(String payload) {
+ String rendered =
ShiroAuthenticationService.buildUserSearchFilterWithObjectClass(
+ USER_OBJECT_CLASS, USER_ATTRIBUTE, payload);
+
+ assertMetacharacterCounts(rendered, payload,
+ LDAP_REALM_OPEN_PARENS, LDAP_REALM_CLOSE_PARENS, LDAP_REALM_ASTERISKS);
+ }
+
+ @Test
+ void normalSearchTextKeepsSubstringMatching() {
+ assertEquals("(uid=*alice*)",
+ ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE,
"alice"));
+ assertEquals("(&(objectclass=person)(uid=*alice*))",
+ ShiroAuthenticationService.buildUserSearchFilterWithObjectClass(
+ USER_OBJECT_CLASS, USER_ATTRIBUTE, "alice"));
+ }
+
+ @Test
+ void asteriskInSearchTextBecomesLiteral() {
+ // The wildcards Zeppelin adds stay wildcards, the one typed by the user
does not.
+ assertEquals("(uid=*a\\2ab*)",
+ ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE,
"a*b"));
+ assertEquals("(&(objectclass=person)(uid=*a\\2ab*))",
+ ShiroAuthenticationService.buildUserSearchFilterWithObjectClass(
+ USER_OBJECT_CLASS, USER_ATTRIBUTE, "a*b"));
+ }
+
+ @Test
+ void emptySearchTextKeepsExistingBehaviour() {
+ assertEquals("(uid=**)",
+ ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE, ""));
+ assertEquals("(&(objectclass=person)(uid=**))",
+ ShiroAuthenticationService.buildUserSearchFilterWithObjectClass(
+ USER_OBJECT_CLASS, USER_ATTRIBUTE, ""));
+ }
+
+ @Test
+ void configuredAttributeNamesAreEscapedAsWell() {
+ assertEquals("(&(objectclass=per\\29son)(u\\28id=*alice*))",
+
ShiroAuthenticationService.buildUserSearchFilterWithObjectClass("per)son",
"u(id",
+ "alice"));
+ }
+
+ @Test
+ void backslashAndNulInSearchTextAreEscaped() {
+ assertEquals("(uid=*alice\\5c*)",
+ ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE,
"alice\\"));
+ assertEquals("(uid=*alice\\00*)",
+ ShiroAuthenticationService.buildUserSearchFilter(USER_ATTRIBUTE,
"alice\0"));
+ assertEquals("(&(objectclass=person)(uid=*alice\\5c\\00*))",
+ ShiroAuthenticationService.buildUserSearchFilterWithObjectClass(
+ USER_OBJECT_CLASS, USER_ATTRIBUTE, "alice\\\0"));
+ }
+
+ private static void assertMetacharacterCounts(String rendered, String
payload,
+ int expectedOpenParens, int expectedCloseParens, int expectedAsterisks) {
+ assertEquals(expectedOpenParens, count(rendered, '('),
+ "extra unescaped '(' from payload: " + rendered);
+ assertEquals(expectedCloseParens, count(rendered, ')'),
+ "extra unescaped ')' from payload: " + rendered);
+ assertEquals(expectedAsterisks, count(rendered, '*'),
+ "extra unescaped '*' from payload: " + rendered);
+
+ if (payload.indexOf('(') >= 0) {
+ assertTrue(rendered.contains("\\28"), "missing \\28 in: " + rendered);
+ }
+ if (payload.indexOf(')') >= 0) {
+ assertTrue(rendered.contains("\\29"), "missing \\29 in: " + rendered);
+ }
+ if (payload.indexOf('*') >= 0) {
+ assertTrue(rendered.contains("\\2a"), "missing \\2a in: " + rendered);
+ }
+ if (payload.indexOf('\\') >= 0) {
+ assertTrue(rendered.contains("\\5c"), "missing \\5c in: " + rendered);
+ }
+ if (payload.indexOf('\0') >= 0) {
+ assertTrue(rendered.contains("\\00"), "missing \\00 in: " + rendered);
+ }
+ }
+
+ /**
+ * Counts occurrences of {@code ch} in the rendered filter. {@code
LdapFilterEncoder} replaces
+ * every metacharacter with a hex escape such as {@code \2a}, so a
metacharacter that is still
+ * present as itself is by definition an unescaped one.
+ */
+ private static int count(String s, char ch) {
+ int count = 0;
+ for (int i = 0; i < s.length(); i++) {
+ if (s.charAt(i) == ch) {
+ count++;
+ }
+ }
+ return count;
+ }
+}
