hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Vasudev (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (YARN-6623) Add support to turn off launching privileged containers in the container-executor
Date Mon, 21 Aug 2017 13:42:02 GMT

     [ https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Varun Vasudev updated YARN-6623:
--------------------------------
    Attachment: YARN-6623.004.patch

 Thanks for the review [~miklos.szegedi@cloudera.com]!

{quote}
/proc/mounts,/sys/fs/cgroup are always in the same place

This is actually not completely true. If you run in a container,/sys/fs/cgroup can be anywhere

490            .addReadOnlyMountLocation(CGROUPS_ROOT_DIRECTORY,
491                CGROUPS_ROOT_DIRECTORY, false);

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.
{quote}

This was already being set before this patch. Perhaps, it can be handled cleanly as part of
YARN-5534?

{quote}
SET(CMAKE_CXX_FLAGS "$\{CMAKE_CXX_FLAGS} -Wall -std=c++11")

Not sure, but this might not build on old OS like centos6
{quote}
Fixed.

{quote}
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          }

Probably it does worth to check, if entry for example contains “abc=\ndef” to avoid injection
attacks.

{quote}
Fixed.

{quote}
169          ret\[i + 1] = '\0';

I think it is safe to do a single ret\[I] = 0; after the loop
{quote}
Fixed.

{quote}
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    }

What is 1024? How about setting * size before *str, so that there is no need for duplication?
{quote}
Fixed.

{quote}
#define EXECUTOR_PATH_MAX 131072

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.
{quote}
Fixed. Set back to 4096.

{quote}
35        if (command != NULL) \{
36          free(command);
37        }

This is not necessary, free always ignores NULL argument.
{quote}
Fixed.

{quote}
47      if (current_len + string_len < bufflen) \{

bufflen-1 to allocate space for the terminating 0
{quote}
Fixed.

{quote}
48        strncpy(buff + current_len, string, bufflen - current_len);

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.
{quote}
Fixed.

{quote}
165      if (0 == strncmp(container_name, CONTAINER_NAME_PREFIX, strlen(CONTAINER_NAME_PREFIX)))
\{

This is a double scan. You can just use strcmp with the same effect.
{quote}
{quote}
249      const char *regex_str = "^((\[a-zA-Z0-9.-]+)(:\[0-9]+)?/)?(\[a-z0-9_./-]+)(:\[a-zA-Z0-9_.-]+)?$”;

Image can be specified by a digest, which is more secure. I do not see that supported by the
regex. IMAGE\[:TAG|@DIGEST]
{quote}

[~shanekumpf@gmail.com] added this, I'm just moving it to a different file. Shane can you
please comment?

{quote}
#  docker.binary=/bin/docker
115          docker_binary = strdup("/usr/bin/docker”);

We should choose one and use it everywhere.
{quote}
Fixed.

{quote}
477        permitted_mount_len = strlen(permitted_mounts\[i]);
478        if (permitted_mounts\[i]\[permitted_mount_len - 1] == '/') \{

Buffer underflow at permitted_mount_len == 0
{quote}
Fixed.

{quote}
488    static char* normalize_mount(const char* mount) \{

It would be really nice to have some comments here.
{quote}
Fixed.

{quote}
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';

Potential buffer underflow moreover most likely the character does not match and we end with
a normalized mount path of “/“. This happens, when strlen(real_mount)==0
{quote}
Fixed


> 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, YARN-6623.004.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