fix code injection vulnerability introduced by ShellUtils.getGroupsFo…

…rUserCommand

core/common/src/main/java/alluxio/util/CommonUtils.java

@@ -299,17 +299,21 @@ public static <T> T createNewClassInstance(Class<T> cls, Class<?>[] ctorClassArg
    * @return the groups list that the {@code user} belongs to. The primary group is returned first
    */
   public static List<String> getUnixGroups(String user) throws IOException {
-    String result;
+    String effectiveGroupsResult, allGroupsResult;
     List<String> groups = new ArrayList<>();
     try {
-      result = ShellUtils.execCommand(ShellUtils.getGroupsForUserCommand(user));
+      effectiveGroupsResult = ShellUtils.execCommand(ShellUtils.getEffectiveGroupsForUserCommand(user));
+      allGroupsResult = ShellUtils.execCommand(ShellUtils.getAllGroupsForUserCommand(user));
     } catch (ExitCodeException e) {
       // if we didn't get the group - just return empty list
       LOG.warn("got exception trying to get groups for user {}: {}", user, e.toString());
       return groups;
     }
-
-    StringTokenizer tokenizer = new StringTokenizer(result, ShellUtils.TOKEN_SEPARATOR_REGEX);
+    StringTokenizer tokenizer = new StringTokenizer(effectiveGroupsResult, ShellUtils.TOKEN_SEPARATOR_REGEX);
+    while (tokenizer.hasMoreTokens()) {
+      groups.add(tokenizer.nextToken());
+    }
+    tokenizer = new StringTokenizer(allGroupsResult, ShellUtils.TOKEN_SEPARATOR_REGEX);
     while (tokenizer.hasMoreTokens()) {
       groups.add(tokenizer.nextToken());
     }

core/common/src/main/java/alluxio/util/ShellUtils.java

@@ -54,11 +54,35 @@ public final class ShellUtils {
    *
    * @param user the user name
    * @return the Unix command to get a given user's groups list
+   * @deprecated as of Alluxio 2.9.3, replaced by
+   *             {@link ShellUtils#getEffectiveGroupsForUserCommand(String)} and
+   *             {@link ShellUtils#getAllGroupsForUserCommand(String)}
    */
+  @Deprecated
   public static String[] getGroupsForUserCommand(final String user) {
     return new String[] {"bash", "-c", "id -gn " + user + "; id -Gn " + user};
   }
 
+  /**
+   * Gets a Unix command to get a given user's effective groups list.
+   *
+   * @param user the user name
+   * @return the Unix command to get a given user's effective groups list
+   */
+  public static String[] getEffectiveGroupsForUserCommand(final String user){
+    return new String[] {"id", "-gn", user};
+  }
+
+  /**
+   * Gets a Unix command to get a given user's all groups list.
+   *
+   * @param user the user name
+   * @return the Unix command to get a given user's all groups list
+   */
+  public static String[] getAllGroupsForUserCommand(final String user){
+    return new String[] {"id", "-Gn", user};
+  }
+
   /**
    * Returns a Unix command to set permission.
    *

core/common/src/test/java/alluxio/util/CommonUtilsTest.java

@@ -261,15 +261,20 @@ public String toString() {
     }
   }
 
-  private void setupShellMocks(String username, List<String> groups) throws IOException {
+  private void setupShellMocks(String username, List<String> effectiveGroups, List<String> allGroups) throws IOException {
     PowerMockito.mockStatic(ShellUtils.class);
-    String shellResult = "";
-    for (String group: groups) {
-      shellResult = shellResult + " " + group;
+    StringBuilder shellResultForEffective = new StringBuilder();
+    for (String group: effectiveGroups) {
+      shellResultForEffective.append(" ").append(group);
+    }
+    StringBuilder shellResultForAll = new StringBuilder();
+    for (String group: allGroups) {
+      shellResultForAll.append(" ").append(group);
     }
     PowerMockito.when(
-        ShellUtils.execCommand(ShellUtils.getGroupsForUserCommand(Mockito.eq(username))))
-        .thenReturn(shellResult);
+        ShellUtils.execCommand(Mockito.any()))
+        .thenReturn(shellResultForEffective.toString())
+        .thenReturn(shellResultForAll.toString());
   }
 
   /**
@@ -278,19 +283,26 @@ private void setupShellMocks(String username, List<String> groups) throws IOExce
   @Test
   public void userGroup() throws Throwable {
     String userName = "alluxio-user1";
-    String userGroup1 = "alluxio-user1-group1";
-    String userGroup2 = "alluxio-user1-group2";
-    List<String> userGroups = new ArrayList<>();
-    userGroups.add(userGroup1);
-    userGroups.add(userGroup2);
-    setupShellMocks(userName, userGroups);
+    String userEffectiveGroup1 = "alluxio-user1-effective-group1";
+    String userEffectiveGroup2 = "alluxio-user1-effective-group2";
+    String userAllGroup1 = "alluxio-user1-all-group1";
+    String userAllGroup2 = "alluxio-user1-all-group2";
+    List<String> userEffectiveGroups = new ArrayList<>();
+    List<String> userAllGroups = new ArrayList<>();
+    userEffectiveGroups.add(userEffectiveGroup1);
+    userEffectiveGroups.add(userEffectiveGroup2);
+    userAllGroups.add(userAllGroup1);
+    userAllGroups.add(userAllGroup2);
+
+    setupShellMocks(userName, userEffectiveGroups, userAllGroups);
 
     List<String> groups = CommonUtils.getUnixGroups(userName);
-
     assertNotNull(groups);
-    assertEquals(groups.size(), 2);
-    assertEquals(groups.get(0), userGroup1);
-    assertEquals(groups.get(1), userGroup2);
+    assertEquals(groups.size(), 4);
+    assertEquals(groups.get(0), userEffectiveGroup1);
+    assertEquals(groups.get(1), userEffectiveGroup2);
+    assertEquals(groups.get(2), userAllGroup1);
+    assertEquals(groups.get(3), userAllGroup2);
   }
 
   /**

core/common/src/test/java/alluxio/util/ShellUtilsTest.java

@@ -56,13 +56,25 @@ public void execCommandFail() throws Exception {
   }
 
   /**
-   * Tests the {@link ShellUtils#execCommand(String...)} method for a group of commands.
+   * Tests the {@link ShellUtils#execCommand(String...)} method for effective groups command.
    *
    * @throws Throwable when the execution of the commands fails
    */
   @Test
-  public void execGetGroupCommand() throws Exception {
-    String result = ShellUtils.execCommand(ShellUtils.getGroupsForUserCommand("root"));
+  public void execGetEffectiveGroupCommand() throws Exception {
+    String result = ShellUtils.execCommand(ShellUtils.getEffectiveGroupsForUserCommand("root"));
+    // On Linux user "root" will be a part of the group "root". On OSX it will be a part of "admin".
+    assertTrue(result.contains("root") || result.contains("admin"));
+  }
+
+  /**
+   * Tests the {@link ShellUtils#execCommand(String...)} method for all groups command.
+   *
+   * @throws Throwable when the execution of the commands fails
+   */
+  @Test
+  public void execGetAllGroupCommand() throws Exception {
+    String result = ShellUtils.execCommand(ShellUtils.getAllGroupsForUserCommand("root"));
     // On Linux user "root" will be a part of the group "root". On OSX it will be a part of "admin".
     assertTrue(result.contains("root") || result.contains("admin"));
   }