hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Lowe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-8207) Docker container launch use popen have risk of shell expansion
Date Mon, 07 May 2018 22:53:00 GMT

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

Jason Lowe commented on YARN-8207:
----------------------------------

Thanks for updating the patch!

Nit: Needing to initialize the args struct manually with '\{ 0 \}' each time is fragile to
maintain (e.g.: it could easily break as soon as someone adds a new field).  It would be good
to add an init_args function to encapsulate the initialization code or at least hide the init
value in a macro.

There's an off-by-one error and heap corruption when construct_docker_command places the terminating
NULL.  It allocates an array with buffer.length+1 elements then proceeds to write to index
buffer.length+1 which is after the end of the allocated array.

Rather than make an expensive deep copy of the arguments, construct_docker_command only needs
to copy the args vector then set the number of arguments to zero.  At that point we'd be effectively
transferring ownership of the already allocated arg strings to the caller without requiring
full copies.

Speaking of copying, given the args struct is always intended to be passed to some flavor
of execv, I think it would be easier to wield if the args struct heap-allocated its argument
vector and tracked the terminating NULL directly.  That way users don't have to perform a
cumbersome and expensive copy of the arguments or remember to terminate it before calling
execv.  It would also preclude the need for free_char_arr since the existing free_values could
be used instead.  I'm not sure the array in the structure is buying us much.

Speaking of free_char_arr, most uses should use free_values since the calls are immediately
followed by freeing the array pointer which free_values does.

launch_container_as_user does not free docker_command.

Nit: launch_container_as_user calls flatten but it's only necessary in the case where the
fork fails.

flatten adds 1 to the strlen length in the loop, but there is only a need for one NUL terminator
which is already accounted for in the {{total}} initial value.

flatten is using stpcpy incorrectly as it ignores the return values from the function.  stpcpy
returns a pointer to the terminating NUL of the resulting string which is exactly what we
need for appending, so each invocation of stpcpy should be like: {{to = stpcpy(to, ...)}}

flatten terminates the string by writing to buffer[total] but that is past the end of the
allocated array since it is only {{total} bytes in size.  It should be simply
{code}
  *to = '\0';
{code}

reset_args and free_args are a NULL-check away from being the same thing, and arguably free_args
should check for NULL if reset_args does.  That indicates we only need one of these.

Nit: The args length != 0 check in free_args is unnecessary as the {{for}} loop will essentially
do the same.

check_trusted_image should be calling free_values instead of free_char_arr() and free() on
the get_configuration_values_delimiter result.

add_param_to_command_if_allowed (and many other places) doesn't check for make_string failure,
and add_to_args will segfault when it tries to dereference the NULL argument.  Does it make
sense to have add_to_args return failure if the caller tried to add a NULL argument?

This change doesn't look related to the execv changes?  Also looks like a case that could
be simplified quite a bit with strndup and strdup.
{noformat}
@@ -195,11 +218,11 @@ static int add_param_to_command_if_allowed(const struct configuration
*command_c
           } else {
             // If permitted-Values[j] is a REGEX, use REGEX to compare
             if (is_regex(permitted_values[j])) {
-              size_t offset = tmp_ptr - values[i];
+              size_t offset = tmp_ptr - values[i] + 1;
               dst = (char *) alloc_and_clear_memory(offset, sizeof(char));
               strncpy(dst, values[i], offset);
               dst[tmp_ptr - values[i]] = '\0';
-              pattern = (char *) alloc_and_clear_memory((size_t)(strlen(permitted_values[j])
- 6), sizeof(char));
+              pattern = (char *) alloc_and_clear_memory((size_t)(strlen(permitted_values[j])
- 5), sizeof(char));
               strcpy(pattern, permitted_values[j] + 6);
               ret = execute_regex_match(pattern, dst);
             } else {
{noformat}

set_pid_namespace and set_privileged allocate an unused 1024-byte array.

Nit: The last {{goto free_and_exit}} in get_docker_kill_command is unnecessary.

In get_docker_start_command:
{code}
  ret = add_to_args(args, container_name);
  if (ret != 0) {
    goto free_and_exit;
  }
free_and_exit:
{code}
should be simplified to:
{code}
  ret = add_to_args(args, container_name);
free_and_exit:
{code}

set_group_add should be calling free_values instad of free_char_arr or the pointer array is
leaked.

get_docker_stop_command does not check for add_to_args failure after trying to add the {{time_buffer}}
argument.

check_mount_permitted should not call normalize_mount until after checking if permitted_mounts
is NULL.  Then the code doesn't have to worry about freeing what was never allocated.

Nit: The NULL check for mount_src in add_mounts is unnecessary because {{free}} is safe to
call for NULL pointers.

make_string is not checking for vsnprintf failure.



> Docker container launch use popen have risk of shell expansion
> --------------------------------------------------------------
>
>                 Key: YARN-8207
>                 URL: https://issues.apache.org/jira/browse/YARN-8207
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: yarn-native-services
>    Affects Versions: 3.0.0, 3.1.0, 3.0.1, 3.0.2
>            Reporter: Eric Yang
>            Assignee: Eric Yang
>            Priority: Blocker
>              Labels: Docker
>         Attachments: YARN-8207.001.patch, YARN-8207.002.patch, YARN-8207.003.patch, YARN-8207.004.patch,
YARN-8207.005.patch, YARN-8207.006.patch, YARN-8207.007.patch
>
>
> Container-executor code utilize a string buffer to construct docker run command, and
pass the string buffer to popen for execution.  Popen spawn a shell to run the command.  Some
arguments for docker run are still vulnerable to shell expansion.  The possible solution is
to convert from char * buffer to string array for execv to avoid shell expansion.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
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