[
https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16131651#comment-16131651
]
Miklos Szegedi edited comment on YARN-6623 at 8/18/17 2:53 AM:
---------------------------------------------------------------
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 “/“. (!) This happens, when
strlen(real_mount)==0
continued...
was (Author: [email protected]):
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: [email protected]
For additional commands, e-mail: [email protected]
