[ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax - #5384
[ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax#5384kimyenac wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens GET /api/security/userlist/{searchText} by ensuring client-supplied search text is treated as literal text rather than LDAP filter / regex syntax, preventing filter injection and avoiding regex compilation errors in sorting.
Changes:
- Escapes LDAP filter values when constructing LDAP search filters in
ShiroAuthenticationService. - Replaces regex-based prefix prioritization in
SecurityRestApiwith astartsWith-based stable comparator. - Adds unit/integration tests covering LDAP filter rendering and userlist searches containing regex metacharacters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java | Routes LDAP filter construction through RFC 4515 escaping helpers. |
| zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java | Removes regex usage from sorting; prioritizes prefix matches via startsWith. |
| zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java | Adds tests asserting rendered LDAP filters don’t allow metacharacter injection. |
| zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java | Adds endpoint test ensuring regex metacharacters don’t trigger HTTP 500. |
Suppressed comments (1)
zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceFilterInjectionTest.java:127
- assertMetacharacterCounts currently verifies escapes for '(', ')' and '*', but not for backslash (\) and the NUL byte (\0). If those payloads are included, add assertions for the expected RFC 4515 escape sequences (\5c and \00).
if (payload.indexOf('(') >= 0) {
assertTrue(rendered.contains("\\28"), "missing \\28 in: " + rendered);
}
if (payload.indexOf(')') >= 0) {
assertTrue(rendered.contains("\\29"), "missing \\29 in: " + rendered);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static Stream<String> injectionPayloads() { | ||
| return Stream.of( | ||
| ")(uid=*", | ||
| "admin)(|(uid=*", | ||
| "*", | ||
| "admin)(cn=a*", | ||
| ")(mail=*@corp.com", | ||
| "alice)(userPassword=*"); | ||
| } |
There was a problem hiding this 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.
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
ShiroAuthenticationServicebuilds the LDAP search filter by string concatenation, ingetUserList(DefaultLdapRealm, String, int)andgetUserList(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 thatActiveDirectoryGroupRealmalready uses. The filter building is extracted into two small helpers so the rendered filter can be asserted directly in unit tests, following theexpandFilterTemplateapproach already used inLdapRealm.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
getMatchedUsersare already safe:ActiveDirectoryGroupRealmescapes the value insearchForUserName, and theJdbcRealmbranch uses aPreparedStatementwith a validated identifier.2. Regular expression in the sorting comparator
SecurityRestApisorted the matched users witho1.matches(searchText + "(.*)"), which compiles the search text as a regular expression. A search text of*therefore fails withPatternSyntaxException: 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, whichstartsWithdoes without compiling anything. The replacement is a consistent comparator as well, and the alphabetical order within each group is preserved because the sort is stable.What type of PR is it?
Improvement
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6599
How should this be tested?
SecurityRestApiTest.testGetUserListWithRegexMetacharactersreturns 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 theuidcondition 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
SecurityRestApifilters the returned list once more withcontainsIgnoreCase(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