You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "kimyenac (via GitHub)" <gi...@apache.org> on 2026/08/05 02:45:57 UTC
[PR] [ZEPPELIN-6599] Escape user-supplied search text in LDAP user search filters [zeppelin]
kimyenac opened a new pull request, #5384:
URL: https://github.com/apache/zeppelin/pull/5384
### What is this PR for?
`GET /api/security/userlist/{searchText}` passes the search text into the LDAP search filters that `ShiroAuthenticationService` builds 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.
This PR routes both sites 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.
### 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)
### 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`
The existing LDAP realm and Shiro service tests keep passing.
Verified manually against an in-memory LDAP server as well. 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 this change 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax [zeppelin]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5384:
URL: https://github.com/apache/zeppelin/pull/5384#discussion_r3717627598
##########
zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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=*");
+ }
Review Comment:
The injection payload list doesn’t include the remaining RFC 4515 metacharacters mentioned in the PR description (backslash and NUL). Adding them here will ensure the new filter-building helpers are exercised for those cases too.
This issue also appears on line 123 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax [zeppelin]
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul merged PR #5384:
URL: https://github.com/apache/zeppelin/pull/5384
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax [zeppelin]
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on PR #5384:
URL: https://github.com/apache/zeppelin/pull/5384#issuecomment-5202112910
Merged into master (b90e6863).
Cherry-picked into branch-0.12.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax [zeppelin]
Posted by "kimyenac (via GitHub)" <gi...@apache.org>.
kimyenac commented on code in PR #5384:
URL: https://github.com/apache/zeppelin/pull/5384#discussion_r3717813975
##########
zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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=*");
+ }
Review Comment:
Good catch, added in c7310e0. The payload list now includes a bare backslash and a NUL, plus two payloads that look like already escaped sequences so double escaping is covered as well. `assertMetacharacterCounts` asserts the `\5c` and `\00` escape sequences too, and a separate test pins the exact rendered filter for both characters since the parameterized display name cannot show them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org