From f9aa4e912bfdb6c5bda14d574509d25803b9f801 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 25 Aug 2020 11:47:30 -0300 Subject: [PATCH 1/2] Cleanup rcl_get_secure_root() implementation. * Do not assume allocation will succeed. * Do not leak allocated memory on failure. Signed-off-by: Michel Hidalgo --- rcl/src/rcl/security.c | 43 +++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index e895fb0f7..da90f4109 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -132,36 +132,35 @@ char * rcl_get_secure_root( const char * name, const rcl_allocator_t * allocator) { - bool ros_secure_enclave_override = true; + RCL_CHECK_ARGUMENT_FOR_NULL(name, NULL); + RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "allocator is invalid", return NULL); // find out if either of the configuration environment variables are set const char * env_buf = NULL; - if (NULL == name) { - return NULL; - } - const char * get_env_error_str = NULL; + // check if enclave override environment variable is empty - get_env_error_str = rcutils_get_env(ROS_SECURITY_ENCLAVE_OVERRIDE, &env_buf); + const char * get_env_error_str = rcutils_get_env(ROS_SECURITY_ENCLAVE_OVERRIDE, &env_buf); if (NULL != get_env_error_str) { - RCUTILS_LOG_ERROR("rcutils_get_env failed: %s\n", get_env_error_str); - return NULL; - } - if (!env_buf) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "rcutils_get_env failed: %s", get_env_error_str); return NULL; } - if (0 == strcmp("", env_buf)) { - ros_secure_enclave_override = false; + + char * ros_secure_enclave_override_env = NULL; + if (0 != strcmp("", env_buf)) { + ros_secure_enclave_override_env = rcutils_strdup(env_buf, *allocator); + if (NULL == ros_secure_enclave_override_env) { + RCL_SET_ERROR_MSG("failed to duplicate enclave override string"); + return NULL; + } } - char * ros_secure_enclave_override_env = rcutils_strdup(env_buf, *allocator); // check if keystore environment variable is empty + env_buf = NULL; get_env_error_str = rcutils_get_env(ROS_SECURITY_KEYSTORE_VAR_NAME, &env_buf); if (NULL != get_env_error_str) { - RCUTILS_LOG_ERROR("rcutils_get_env failed: %s\n", get_env_error_str); - allocator->deallocate(ros_secure_enclave_override_env, allocator->state); - return NULL; - } - if (!env_buf) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "rcutils_get_env failed: %s", get_env_error_str); allocator->deallocate(ros_secure_enclave_override_env, allocator->state); return NULL; } @@ -170,10 +169,15 @@ char * rcl_get_secure_root( return NULL; // environment variable was empty } char * ros_secure_keystore_env = rcutils_strdup(env_buf, *allocator); + if (NULL == ros_secure_keystore_env) { + RCL_SET_ERROR_MSG("failed to duplicate enclave override string"); + allocator->deallocate(ros_secure_enclave_override_env, allocator->state); + return NULL; + } // given usable environment variables, overwrite with next lookup char * secure_root = NULL; - if (ros_secure_enclave_override) { + if (NULL != ros_secure_enclave_override_env) { secure_root = exact_match_lookup( ros_secure_enclave_override_env, ros_secure_keystore_env, @@ -200,6 +204,7 @@ char * rcl_get_secure_root( allocator->deallocate(secure_root, allocator->state); return NULL; } + allocator->deallocate(ros_secure_enclave_override_env, allocator->state); allocator->deallocate(ros_secure_keystore_env, allocator->state); return secure_root; } From cf7877eeac03eef21c660e406dce024c3d81b6f1 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 25 Aug 2020 17:22:23 -0300 Subject: [PATCH 2/2] Address peer review comments. Signed-off-by: Michel Hidalgo --- rcl/src/rcl/security.c | 90 +++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 44 deletions(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index da90f4109..bc688398d 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -128,6 +128,24 @@ char * exact_match_lookup( return secure_root; } +static const char * +dupenv(const char * name, const rcl_allocator_t * allocator, char ** value) +{ + const char * buffer = NULL; + const char * error = rcutils_get_env(name, &buffer); + if (NULL != error) { + return error; + } + *value = NULL; + if (0 != strcmp("", buffer)) { + *value = rcutils_strdup(buffer, *allocator); + if (NULL == *value) { + return "string duplication failed"; + } + } + return NULL; +} + char * rcl_get_secure_root( const char * name, const rcl_allocator_t * allocator) @@ -135,48 +153,32 @@ char * rcl_get_secure_root( RCL_CHECK_ARGUMENT_FOR_NULL(name, NULL); RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "allocator is invalid", return NULL); - // find out if either of the configuration environment variables are set - const char * env_buf = NULL; + char * secure_root = NULL; + char * ros_secure_keystore_env = NULL; + char * ros_secure_enclave_override_env = NULL; - // check if enclave override environment variable is empty - const char * get_env_error_str = rcutils_get_env(ROS_SECURITY_ENCLAVE_OVERRIDE, &env_buf); - if (NULL != get_env_error_str) { + // check keystore environment variable + const char * error = + dupenv(ROS_SECURITY_KEYSTORE_VAR_NAME, allocator, &ros_secure_keystore_env); + if (NULL != error) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "rcutils_get_env failed: %s", get_env_error_str); + "failed to get %s: %s", ROS_SECURITY_KEYSTORE_VAR_NAME, error); return NULL; } - char * ros_secure_enclave_override_env = NULL; - if (0 != strcmp("", env_buf)) { - ros_secure_enclave_override_env = rcutils_strdup(env_buf, *allocator); - if (NULL == ros_secure_enclave_override_env) { - RCL_SET_ERROR_MSG("failed to duplicate enclave override string"); - return NULL; - } + if (NULL == ros_secure_keystore_env) { + return NULL; // environment variable was empty } - // check if keystore environment variable is empty - env_buf = NULL; - get_env_error_str = rcutils_get_env(ROS_SECURITY_KEYSTORE_VAR_NAME, &env_buf); - if (NULL != get_env_error_str) { + // check enclave override environment variable + error = dupenv(ROS_SECURITY_ENCLAVE_OVERRIDE, allocator, &ros_secure_enclave_override_env); + if (NULL != error) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "rcutils_get_env failed: %s", get_env_error_str); - allocator->deallocate(ros_secure_enclave_override_env, allocator->state); - return NULL; - } - if (0 == strcmp("", env_buf)) { - allocator->deallocate(ros_secure_enclave_override_env, allocator->state); - return NULL; // environment variable was empty - } - char * ros_secure_keystore_env = rcutils_strdup(env_buf, *allocator); - if (NULL == ros_secure_keystore_env) { - RCL_SET_ERROR_MSG("failed to duplicate enclave override string"); - allocator->deallocate(ros_secure_enclave_override_env, allocator->state); - return NULL; + "failed to get %s: %s", ROS_SECURITY_ENCLAVE_OVERRIDE, error); + goto leave_rcl_get_secure_root; } // given usable environment variables, overwrite with next lookup - char * secure_root = NULL; if (NULL != ros_secure_enclave_override_env) { secure_root = exact_match_lookup( ros_secure_enclave_override_env, @@ -189,21 +191,21 @@ char * rcl_get_secure_root( allocator); } - if (NULL == secure_root || !rcutils_is_directory(secure_root)) { - // Check secure_root is not NULL before checking directory - if (NULL == secure_root) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "SECURITY ERROR: unable to find a folder matching the name '%s' in '%s'. ", - name, ros_secure_keystore_env); - } else { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "SECURITY ERROR: directory '%s' does not exist.", secure_root); - } - allocator->deallocate(ros_secure_enclave_override_env, allocator->state); - allocator->deallocate(ros_secure_keystore_env, allocator->state); + if (NULL == secure_root) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "SECURITY ERROR: unable to find a folder matching the name '%s' in '%s'. ", + name, ros_secure_keystore_env); + goto leave_rcl_get_secure_root; + } + + if (!rcutils_is_directory(secure_root)) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "SECURITY ERROR: directory '%s' does not exist.", secure_root); allocator->deallocate(secure_root, allocator->state); - return NULL; + secure_root = NULL; } + +leave_rcl_get_secure_root: allocator->deallocate(ros_secure_enclave_override_env, allocator->state); allocator->deallocate(ros_secure_keystore_env, allocator->state); return secure_root;