[
https://issues.apache.org/jira/browse/YARN-7590?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16300387#comment-16300387
]
Miklos Szegedi commented on YARN-7590:
--------------------------------------
Thank you for the patch, [~eyang]. I have a few minor comments left.
{code}
717 int caller_uid = 0;
{code}
Just in case I would have an invalid init value like -1.
{code}
712 int result = check_nm_local_dir(caller_uid, container_log_dir);
713 if (result != 0) {
714 container_log_dir = NULL;
715 }
...
1056 int result = check_nm_local_dir(caller_uid, *log_root);
1057 if (result != 0) {
1058 app_log_dir = NULL;
1059 }
{code}
I am missing here a useful comment like below. You may also want to mention the
faulting directory.
{code}
fprintf(LOGFILE, "Permission mismatch for %s for uid: %d.\n", nm_root,
caller_uid);
{code}
Even better a log in check_nm_local_dir in case of failure would help a lot to
diagnose problems.
{code}
531 int check_nm_local_dir(int caller_uid, const char *nm_root) {
532 struct stat info;
533 stat(nm_root, &info);
534 if (caller_uid != info.st_uid) {
535 return 1;
536 }
537 return 0;
538 }
{code}
There is no error check on the stat call.
{code}
711 char *container_log_dir = get_app_log_directory(*log_dir_ptr,
combined_name);
712 int result = check_nm_local_dir(caller_uid, container_log_dir);
713 if (result != 0) {
714 container_log_dir = NULL;
715 }
{code}
{{create_container_directories()}} needs to check for {{log_dir_ptr}} not
{{container_log_dir}} that does not exist, yet.
Also a note. If the check succeeds, we do an mkdirs() that walks up the stack
and may create parent directories. It may be good to put the check into mkdirs
as well (or only there), when we need to create a directory.
> Improve container-executor validation check
> -------------------------------------------
>
> Key: YARN-7590
> URL: https://issues.apache.org/jira/browse/YARN-7590
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: security, yarn
> Affects Versions: 2.0.1-alpha, 2.2.0, 2.3.0, 2.4.0, 2.5.0, 2.6.0, 2.7.0,
> 2.8.0, 2.8.1, 3.0.0-beta1
> Reporter: Eric Yang
> Assignee: Eric Yang
> Attachments: YARN-7590.001.patch, YARN-7590.002.patch,
> YARN-7590.003.patch, YARN-7590.004.patch
>
>
> There is minimum check for prefix path for container-executor. If YARN is
> compromised, attacker can use container-executor to change system files
> ownership:
> {code}
> /usr/local/hadoop/bin/container-executor spark yarn 0 etc /home/yarn/tokens
> /home/spark / ls
> {code}
> This will change /etc to be owned by spark user:
> {code}
> # ls -ld /etc
> drwxr-s---. 110 spark hadoop 8192 Nov 21 20:00 /etc
> {code}
> Spark user can rewrite /etc files to gain more access. We can improve this
> with additional check in container-executor:
> # Make sure the prefix path is owned by the same user as the caller to
> container-executor.
> # Make sure the log directory prefix is owned by the same user as the caller.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
