[ZEPPELIN-6599] Stop treating the user list search text as LDAP filter and regex syntax

zeppelin-server/src/main/java/org/apache/zeppelin/rest/SecurityRestApi.java

@@ -18,6 +18,7 @@
 
 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 Response getUserList(@PathParam("searchText") final String searchText) {
     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)) {

zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java

@@ -53,6 +53,7 @@
 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 @@ private List<String> getUserList(DefaultLdapRealm r, String searchText, int numU
       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 @@ private List<String> getUserList(DefaultLdapRealm r, String searchText, int numU
     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 @@ private List<String> getUserList(LdapRealm r, String searchText, int numUsersToF
       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 @@ private List<String> getUserList(LdapRealm r, String searchText, int numUsersToF
     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.
    *

zeppelin-server/src/test/java/org/apache/zeppelin/rest/SecurityRestApiTest.java

@@ -91,6 +91,23 @@ void testGetUserList() throws IOException {
     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");

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;
+  }
+}