vyommani commented on code in PR #903:
URL: https://github.com/apache/ranger/pull/903#discussion_r3025715075
##########
ugsync/src/main/java/org/apache/ranger/unixusersync/process/UnixUserGroupBuilder.java:
##########
@@ -330,6 +340,13 @@ private void buildUnixUserList(String command) throws
Throwable {
continue;
}
+ if (validateUserName) {
+ if (!isValidUserName(userName)) {
+ LOG.warn("Ignoring Unix Username: [{}]: failed to
confirm to validation-pattern: [{}]", userName, regExUserNameValidator);
Review Comment:
I had already addressed this vulnerability in an earlier change by updating
the command execution from:
{"bash", "-c", "id -G " + userName}
to the safer:
{"id", "-G", userName}
This change ensures that the shell is never invoked. As a result,
metacharacters in the username have no special meaning at the OS execve()
level, which fully mitigates the command injection risk.
The validation logic proposed in this PR is a good attempt at adding
defense-in-depth. However, I'm concerned that the regex-based approach (even
with the updated broader patterns) could unintentionally reject many valid
usernames — for example, legitimate usernames written in Korean, Japanese,
Chinese, or other languages that use non-ASCII characters.
Could you please take a look at the fix in RANGER-5508 and let me know what
you think about the above analysis? If this turns out to be a completely
unrelated issue, please feel free to let me know as well.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
