Skip to content

Commit

Permalink
fix heap buffer overflow and memory leaks
Browse files Browse the repository at this point in the history 
Signed-off-by: Abby Xu <abbyxu@amazon.com>
  • Loading branch information
@xabxx
xabxx committed Apr 25, 2019
1 parent 64610d2 commit 1c1de1a
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 56 deletions.
2 changes: 1 addition & 1 deletion rcl/include/rcl/security_directory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ extern "C"
* \returns machine specific (absolute) node secure root path or NULL on failure
*/
RCL_PUBLIC
const char * rcl_get_secure_root(
char * rcl_get_secure_root(
const char * node_name,
const char * node_namespace,
const rcl_allocator_t * allocator
Expand Down
2 changes: 1 addition & 1 deletion rcl/src/rcl/node.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ rcl_node_init(
node_security_options.enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE;
} else { // if use_security
// File discovery magic here
const char * node_secure_root = rcl_get_secure_root(name, local_namespace_, allocator);
const char * node_secure_root = rcl_get_secure_root(name, local_namespace_, *allocator);
if (node_secure_root) {
RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", node_secure_root);
node_security_options.security_root_path = node_secure_root;
Expand Down
49 changes: 31 additions & 18 deletions rcl/src/rcl/security_directory.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,21 @@ typedef char * (* security_lookup_fn_t) (
const char * node_name,
const char * node_namespace,
const char * ros_secure_root_env,
const rcl_allocator_t * allocator
const rcl_allocator_t allocator
);

char * exact_match_lookup(
const char * node_name,
const char * node_namespace,
const char * ros_secure_root_env,
const rcl_allocator_t * allocator
const rcl_allocator_t allocator
);

char * prefix_match_lookup(
const char * node_name,
const char * node_namespace,
const char * ros_secure_root_env,
const rcl_allocator_t * allocator
const rcl_allocator_t allocator
);

security_lookup_fn_t g_security_lookup_fns[] = {
Expand Down Expand Up @@ -125,25 +125,25 @@ char * exact_match_lookup(
const char * node_name,
const char * node_namespace,
const char * ros_secure_root_env,
const rcl_allocator_t * allocator)
const rcl_allocator_t allocator)
{
// Perform an exact match for the node's name in directory <root dir>/<namespace>.
char * node_secure_root = NULL;
// "/" case when root namespace is explicitly passed in
if (1 == strlen(node_namespace)) {
node_secure_root = rcutils_join_path(ros_secure_root_env, node_name, *allocator);
node_secure_root = rcutils_join_path(ros_secure_root_env, node_name, allocator);
} else {
char * node_fqn = NULL;
char * node_root_path = NULL;
// Combine node namespace with node name
// TODO(ros2team): remove the hard-coded value of the root namespace
node_fqn = rcutils_format_string(*allocator, "%s%s%s", node_namespace, "/", node_name);
node_fqn = rcutils_format_string(allocator, "%s%s%s", node_namespace, "/", node_name);
// Get native path, ignore the leading forward slash
// TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead
node_root_path = rcutils_to_native_path(node_fqn + 1, *allocator);
node_secure_root = rcutils_join_path(ros_secure_root_env, node_root_path, *allocator);
allocator->deallocate(node_fqn, allocator->state);
allocator->deallocate(node_root_path, allocator->state);
node_root_path = rcutils_to_native_path(node_fqn + 1, allocator);
node_secure_root = rcutils_join_path(ros_secure_root_env, node_root_path, allocator);
allocator.deallocate(node_fqn, allocator.state);
allocator.deallocate(node_root_path, allocator.state);
}
return node_secure_root;
}
Expand All @@ -152,7 +152,7 @@ char * prefix_match_lookup(
const char * node_name,
const char * node_namespace,
const char * ros_secure_root_env,
const rcl_allocator_t * allocator)
const rcl_allocator_t allocator)
{
// Perform longest prefix match for the node's name in directory <root dir>/<namespace>.
char * node_secure_root = NULL;
Expand All @@ -162,21 +162,21 @@ char * prefix_match_lookup(
base_lookup_dir = (char *) ros_secure_root_env;
} else {
// TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead.
base_lookup_dir = rcutils_join_path(ros_secure_root_env, node_namespace + 1, *allocator);
base_lookup_dir = rcutils_join_path(ros_secure_root_env, node_namespace + 1, allocator);
}
if (get_best_matching_directory(base_lookup_dir, node_name, matched_dir)) {
node_secure_root = rcutils_join_path(base_lookup_dir, matched_dir, *allocator);
node_secure_root = rcutils_join_path(base_lookup_dir, matched_dir, allocator);
}
if (base_lookup_dir != ros_secure_root_env && NULL != base_lookup_dir) {
allocator->deallocate(base_lookup_dir, allocator->state);
allocator.deallocate(base_lookup_dir, allocator.state);
}
return node_secure_root;
}

const char * rcl_get_secure_root(
char * rcl_get_secure_root(
const char * node_name,
const char * node_namespace,
const rcl_allocator_t * allocator)
const rcl_allocator_t allocator)
{
bool ros_secure_node_override = true;

Expand Down Expand Up @@ -208,9 +208,11 @@ const char * rcl_get_secure_root(
}
}

bool ros_secure_root_env_is_assigned = false;

// found a usable environment variable, copy into our memory before overwriting with next lookup
char * ros_secure_root_env =
(char *)allocator->allocate(ros_secure_root_size + 1, allocator->state);
(char *)allocator.allocate(ros_secure_root_size + 1, allocator.state);
memcpy(ros_secure_root_env, env_buf, ros_secure_root_size + 1);
// TODO(ros2team): This make an assumption on the value and length of the root namespace.
// This should likely come from another (rcl/rmw?) function for reuse.
Expand All @@ -221,6 +223,7 @@ const char * rcl_get_secure_root(
if (ros_secure_node_override) {
node_secure_root = ros_secure_root_env;
lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_NODE_OVERRIDE];
ros_secure_root_env_is_assigned = true;

} else {
// Check which lookup method to use and invoke the relevant function.
Expand All @@ -246,12 +249,22 @@ const char * rcl_get_secure_root(
"SECURITY ERROR: unable to find a folder matching the node name in %s%s."
"Lookup strategy: %s",
ros_secure_root_env, node_namespace, lookup_strategy);
allocator.deallocate(node_secure_root, allocator.state);
if (!ros_secure_root_env_is_assigned) {
allocator.deallocate(ros_secure_root_env, allocator.state);
}
} else if (!rcutils_is_directory(node_secure_root)) {
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"SECURITY ERROR: directory %s does not exist. Lookup strategy: %s",
node_secure_root, lookup_strategy);
allocator->deallocate(node_secure_root, allocator->state);
allocator.deallocate(node_secure_root, allocator.state);
if (!ros_secure_root_env_is_assigned) {
allocator.deallocate(ros_secure_root_env, allocator.state);
}
} else {
if (!ros_secure_root_env_is_assigned && ros_secure_root_env != NULL) {
allocator.deallocate(ros_secure_root_env, allocator.state);
}
return node_secure_root;
}
return NULL;
Expand Down
79 changes: 43 additions & 36 deletions rcl/test/rcl/test_security_directory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,13 @@
#include <string>
#include "rcl/security_directory.h"
#include "rcutils/filesystem.h"
#include "rcl/error_handling.h"

#define ROOT_NAMESPACE "/"
#define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "test_security_directory"
#define TEST_NODE_NAME "dummy_node"
#define TEST_NODE_NAMESPACE ROOT_NAMESPACE TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME

char g_envstring[512] = {0};

static int putenv_wrapper(const char * env_var)
{
#ifdef _WIN32
Expand All @@ -49,33 +48,40 @@ static int unsetenv_wrapper(const char * var_name)
class TestGetSecureRoot : public ::testing::Test
{
protected:
rcl_allocator_t allocator;
char * root_path;

void SetUp() override
{
// Reset rcutil error global state in case a previously
// running test has failed.
rcl_reset_error();

// Always make sure the variable we set is unset at the beginning of a test
unsetenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME);
unsetenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME);
unsetenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME);
allocator = rcl_get_default_allocator();
root_path = "";
}
};

TEST_F(TestGetSecureRoot, failureScenarios) {
rcl_allocator_t allocator = rcl_get_default_allocator();
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator),
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, allocator),
(char *) NULL);

putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);

/* Security directory is set, but there's no matching directory */
/// Wrong namespace
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, "/some_other_namespace", &allocator),
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, "/some_other_namespace", allocator),
(char *) NULL);
/// Wrong node name
ASSERT_EQ(rcl_get_secure_root("not_" TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator),
ASSERT_EQ(rcl_get_secure_root("not_" TEST_NODE_NAME, TEST_NODE_NAMESPACE, allocator),
(char *) NULL);
}

TEST_F(TestGetSecureRoot, successScenarios) {
rcl_allocator_t allocator = rcl_get_default_allocator();
putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
/* --------------------------
* Namespace : Custom (local)
Expand All @@ -85,9 +91,9 @@ TEST_F(TestGetSecureRoot, successScenarios) {
* Namespace: /test_security_directory
* Node: dummy_node
*/
std::string secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
ASSERT_STREQ(TEST_NODE_NAME,
secure_root.substr(secure_root.size() - (sizeof(TEST_NODE_NAME) - 1)).c_str());
char * secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, allocator);
std::string secure_root_str(secure_root);
ASSERT_STREQ(TEST_NODE_NAME, secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_NODE_NAME) - 1)).c_str());

/* --------------------------
* Namespace : Custom (local)
Expand All @@ -96,35 +102,34 @@ TEST_F(TestGetSecureRoot, successScenarios) {
* Root: ${CMAKE_BINARY_DIR}/tests/resources
* Namespace: /test_security_directory
* Node: dummy_node_and_some_suffix_added */
ASSERT_STRNE(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE, allocator);
ASSERT_STRNE(root_path, secure_root);
putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE, allocator);
ASSERT_STREQ(root_path, secure_root);
allocator.deallocate(root_path, allocator.state);


/* Include the namespace as part of the root security directory and test root namespace */
char * base_lookup_dir_fqn = rcutils_join_path(TEST_RESOURCES_DIRECTORY,
TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME, allocator);
std::string putenv_input = ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=";
putenv_input += base_lookup_dir_fqn;
memcpy(g_envstring, putenv_input.c_str(), sizeof(g_envstring) - 1);
putenv_wrapper(g_envstring);
putenv_wrapper(putenv_input.c_str());
/* --------------------------
* Namespace : Root
* Match type : Exact
* --------------------------
* Root: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory
* Namespace: /
* Node: dummy_node */
ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE, allocator);
ASSERT_STREQ(root_path, secure_root);
allocator.deallocate(root_path, allocator.state);
putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_EXACT");
ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE, allocator);
ASSERT_STREQ(root_path, secure_root);
allocator.deallocate(root_path, allocator.state);

/* --------------------------
* Namespace : Root
Expand All @@ -133,32 +138,34 @@ TEST_F(TestGetSecureRoot, successScenarios) {
* Root dir: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory
* Namespace: /
* Node: dummy_node_and_some_suffix_added */
ASSERT_STRNE(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE, allocator);
ASSERT_STRNE(root_path, secure_root);
putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE,
&allocator),
secure_root.c_str());
root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE, allocator);
allocator.deallocate(root_path, allocator.state);

allocator.deallocate(secure_root, allocator.state);
allocator.deallocate(base_lookup_dir_fqn, allocator.state);
}

TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride) {
rcl_allocator_t allocator = rcl_get_default_allocator();
/* Specify a valid directory */
putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
ASSERT_STREQ(rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter",
&allocator), TEST_RESOURCES_DIRECTORY);
root_path = rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter", allocator);
ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
allocator.deallocate(root_path, allocator.state);

/* Setting root dir has no effect */
root_path = rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter", allocator);
putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
ASSERT_STREQ(rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter",
&allocator), TEST_RESOURCES_DIRECTORY);
ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
allocator.deallocate(root_path, allocator.state);

/* The override provided should exist. Providing correct node/namespace/root dir won't help
* if the node override is invalid. */
putenv_wrapper(
ROS_SECURITY_NODE_DIRECTORY_VAR_NAME
"=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail");
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator),
ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, allocator),
(char *) NULL);
}

0 comments on commit 1c1de1a

Please sign in to comment.