hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Miklos Szegedi (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6623) Add support to turn off launching privileged containers in the container-executor
Date Fri, 18 Aug 2017 02:53:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16131651#comment-16131651
] 

Miklos Szegedi commented on YARN-6623:
--------------------------------------

Thank you for the patch, [~vvasudev]. I had time to check only the first half so far. I hope
this helps.
{code}
/proc/mounts,/sys/fs/cgroup are always in the same place
{code}
This is actually not completely true. If you run in a container,/sys/fs/cgroup can be anywhere
{code}
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -std=c++11")
{code}
Not sure, but this might not build on old OS like centos6
{code}
490            .addReadOnlyMountLocation(CGROUPS_ROOT_DIRECTORY,
491                CGROUPS_ROOT_DIRECTORY, false);
{code}
This is actually a security issue. As opposed to lxcfs, mounting cgroups will expose information
about all the other containers to each container. This change is big enough but you might
want to whitelist this in the future.
{code}
76          printWriter.println("[docker-command-execution]");
77          for (Map.Entry<String, List<String>> entry : cmd.getCommandArguments()
78              .entrySet()) {
79            printWriter.println("  " + entry.getKey() + "=" + StringUtils
80                .join(",", entry.getValue()));
81          }
{code}
Probably it does worth to check, if entry for example contains “abc=\ndef” to avoid injection
attacks.
{code}
169          ret[i + 1] = '\0';
{code}
I think it is safe to do a single {{ret[I] = 0;}} after the loop
{code}
177    void quote_and_append_arg(char **str, size_t *size, const char* param, const char *arg)
{
178      char *tmp = escape_single_quote(arg);
179      strcat(*str, param);
180      strcat(*str, "'");
181      if(strlen(*str) + strlen(tmp) > *size) {
182        *str = (char *) realloc(*str, strlen(*str) + strlen(tmp) + 1024);
183        if(*str == NULL) {
184          exit(OUT_OF_MEMORY);
185        }
186        *size = strlen(*str) + strlen(tmp) + 1024;
187      }
188      strcat(*str, tmp);
189      strcat(*str, "' ");
190      free(tmp);
191    }
{code}
What is 1024? How about setting * size before *str, so that there is no need for duplication?
{code}
#define EXECUTOR_PATH_MAX 131072
{code}
This is lots of allocation. The OS actually needs to zero all the allocated memory before
giving it to other processes after close, so this might add to the overall CPU usage and memory
bandwidth.
{code}
35        if (command != NULL) {
36          free(command);
37        }
{code}
This is not necessary, free always ignores NULL argument.
{code}
47      if (current_len + string_len < bufflen) {
{code}
bufflen-1 to allocate space for the terminating 0
{code}
48        strncpy(buff + current_len, string, bufflen - current_len);
{code}
strncpy does not add 0 terminator at the end of the target, if strlen(string)==bufflen - current_len
resulting in a read buffer overflow later.
{code}
#  docker.binary=/bin/docker
115          docker_binary = strdup("/usr/bin/docker”);
{code}
We should choose one and use it everywhere.
{code}
165      if (0 == strncmp(container_name, CONTAINER_NAME_PREFIX, strlen(CONTAINER_NAME_PREFIX)))
{
{code}
This is a double scan. You can just use strcmp with the same effect.
{code}
249      const char *regex_str = "^(([a-zA-Z0-9.-]+)(:[0-9]+)?/)?([a-z0-9_./-]+)(:[a-zA-Z0-9_.-]+)?$”;
{code}
Image can be specified by a digest, which is more secure. I do not see that supported by the
regex. IMAGE[:TAG|@DIGEST]
{code}
477        permitted_mount_len = strlen(permitted_mounts[i]);
478        if (permitted_mounts[i][permitted_mount_len - 1] == '/') {
{code}
Buffer underflow at permitted_mount_len == 0
{code}
488    static char* normalize_mount(const char* mount) {
{code}
It would be really nice to have some comments here.
{code}
503          size_t len = strlen(real_mount);
504          if (real_mount[len - 1] != '/') {
505            ret_ptr = (char *) alloc_and_clear_memory(len + 2, sizeof(char));
506            strncpy(ret_ptr, real_mount, len);
507            ret_ptr[len] = '/';
508            ret_ptr[len + 1] = '\0';
{code}
Potential buffer underflow moreover most likely the character does not match and we end with
a normalized mount path of “/“. (!)
continued...


> Add support to turn off launching privileged containers in the container-executor
> ---------------------------------------------------------------------------------
>
>                 Key: YARN-6623
>                 URL: https://issues.apache.org/jira/browse/YARN-6623
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>            Priority: Blocker
>         Attachments: YARN-6623.001.patch, YARN-6623.002.patch, YARN-6623.003.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should add a flag
to the container-executor.cfg allowing admins to disable launching privileged containers at
the container-executor level.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message