diff --git a/build_dependencies.sh b/build_dependencies.sh index 86d6c353..b890ec13 100755 --- a/build_dependencies.sh +++ b/build_dependencies.sh @@ -10,7 +10,7 @@ cd ${GITHUB_WORKSPACE} #1. Install Dependencies and packages apt update -apt install -y ninja-build meson curl libsoup2.4-dev libxml2-dev libglib2.0-dev gobject-introspection libgirepository1.0-dev libgtk-3-dev valac pandoc +apt install -y git python3 python3-pip cmake ninja-build meson curl libsoup2.4-dev libxml2-dev libglib2.0-dev gobject-introspection libgirepository1.0-dev libgtk-3-dev libcurl4-openssl-dev libcunit1-dev valac pandoc pip install jsonref ############################ @@ -28,17 +28,25 @@ cd .. rm -rf iarmbus ThunderTools Thunder entservices-apis entservices-testframework gssdp +THUNDER_TOOLS_COMMIT_SHA="d5dd83c7c19c49c7f25c558c126500bd2d64f7a4" +THUNDER_COMMIT_SHA="2c0fcc5529e7da734be558ca6efa05d934dcce31" git clone https://github.com/rdkcentral/iarmbus.git export IARMBUS_PATH=$GITHUB_WORKSPACE/iarmbus -git clone --branch R4.4.3 https://github.com/rdkcentral/ThunderTools.git +git clone --branch R4_4-RDK https://github.com/rdkcentral/ThunderTools.git +cd ThunderTools +git checkout $THUNDER_TOOLS_COMMIT_SHA +cd .. -git clone --branch R4.4.1 https://github.com/rdkcentral/Thunder.git +git clone --branch R4_4-RDK https://github.com/rdkcentral/Thunder.git +cd Thunder +git checkout $THUNDER_COMMIT_SHA +cd .. -git clone --branch main https://github.com/rdkcentral/entservices-apis.git +git clone --branch develop https://github.com/rdkcentral/entservices-apis.git -git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git +git clone --branch 2.0.0 https://github.com/rdkcentral/entservices-testframework.git git clone --branch gssdp-1.2.3 https://gitlab.gnome.org/GNOME/gssdp.git @@ -60,7 +68,6 @@ cd - echo "======================================================================================" echo "buliding thunderTools" cd ThunderTools -patch -p1 < $GITHUB_WORKSPACE/entservices-testframework/patches/00010-R4.4-Add-support-for-project-dir.patch cd - @@ -79,10 +86,6 @@ echo "========================================================================== echo "buliding thunder" cd Thunder -patch -p1 < $GITHUB_WORKSPACE/entservices-testframework/patches/Use_Legact_Alt_Based_On_ThunderTools_R4.4.3.patch -patch -p1 < $GITHUB_WORKSPACE/entservices-testframework/patches/error_code_R4_4.patch -patch -p1 < $GITHUB_WORKSPACE/entservices-testframework/patches/1004-Add-support-for-project-dir.patch -patch -p1 < $GITHUB_WORKSPACE/entservices-testframework/patches/RDKEMW-733-Add-ENTOS-IDS.patch cd - cmake -G Ninja -S Thunder -B build/Thunder \ diff --git a/server/gdial-app.c b/server/gdial-app.c index bd5c8572..0d2687fc 100644 --- a/server/gdial-app.c +++ b/server/gdial-app.c @@ -49,6 +49,19 @@ enum { static GList *application_instances_ = NULL; +static gboolean gdial_app_name_is_safe(const gchar *app_name) { + const gchar *cursor; + if (!app_name || !app_name[0] || !g_utf8_validate(app_name, -1, NULL)) { + return FALSE; + } + for (cursor = app_name; *cursor; ++cursor) { + if (!(g_ascii_isalnum(*cursor) || *cursor == '.' || *cursor == '_' || *cursor == '-')) { + return FALSE; + } + } + return g_strcmp0(app_name, ".") != 0 && g_strcmp0(app_name, "..") != 0; +} + static guint gdial_app_signals[N_SIGNALS] = {0}; G_DEFINE_TYPE_WITH_PRIVATE(GDialApp, gdial_app, G_TYPE_OBJECT) @@ -408,6 +421,7 @@ GDialApp *gdial_app_find_instance_by_instance_id(gint instance_id) { } GDIAL_STATIC gboolean gdial_app_write_additional_dial_data(const gchar *app_name, const gchar *data, size_t length) { + g_return_val_if_fail(gdial_app_name_is_safe(app_name), FALSE); gboolean result = FALSE; GError *err = NULL; gchar *filename = g_build_filename(GDIAL_APP_DIAL_DATA_DIR, app_name, NULL); @@ -439,6 +453,7 @@ GDIAL_STATIC gboolean gdial_app_write_additional_dial_data(const gchar *app_name } GDIAL_STATIC gboolean gdial_app_read_additional_dial_data(const gchar *app_name, gchar **data, size_t *length) { + g_return_val_if_fail(gdial_app_name_is_safe(app_name), FALSE); gboolean result = FALSE; GError *err = NULL; @@ -487,6 +502,7 @@ GDIAL_STATIC gboolean gdial_app_read_additional_dial_data(const gchar *app_name, } GDIAL_STATIC gboolean gdial_app_remove_additional_dial_data_file(const gchar *app_name) { + g_return_val_if_fail(gdial_app_name_is_safe(app_name), FALSE); gboolean result = FALSE; gchar *filename = g_build_filename(GDIAL_APP_DIAL_DATA_DIR, app_name, NULL); GFile *gfile = g_file_new_for_path(filename); diff --git a/server/gdial-rest.c b/server/gdial-rest.c index 8af53e50..acf202f6 100644 --- a/server/gdial-rest.c +++ b/server/gdial-rest.c @@ -812,6 +812,10 @@ static void gdial_rest_http_server_apps_callback(SoupServer *server, for (i = 0; elements[i] != NULL; i++) { gsize ret; /* do not allow any element to be empty, stop on first one */ + if (elements[i][0] == '\0') { + g_strfreev(elements); + gdial_rest_server_http_return_if_fail(FALSE, msg, SOUP_STATUS_NOT_IMPLEMENTED); + } if (j == 0) { ret = g_strlcpy(base, elements[i], sizeof(base)); if (ret >= sizeof(base)) { @@ -844,12 +848,8 @@ static void gdial_rest_http_server_apps_callback(SoupServer *server, */ const gchar *copied_str[] = {base, app_name, instance, last_elem}; i = 0; j = 0; - while (i < element_num && (unsigned int)i < sizeof(copied_str)/sizeof(copied_str[0])) { + while (elements[j] != NULL && i < element_num && (unsigned int)i < sizeof(copied_str)/sizeof(copied_str[0])) { bool flag = false; - if (strlen(elements[j]) == 0) { - j++; - continue; - } invalid_uri = invalid_uri || g_strcmp0(copied_str[i], elements[j]); gdial_rest_server_http_check_if_fail(!invalid_uri, msg, SOUP_STATUS_NOT_IMPLEMENTED, flag); if(flag){ diff --git a/server/gdialservice.cpp b/server/gdialservice.cpp index 97606bf2..1ebd102f 100644 --- a/server/gdialservice.cpp +++ b/server/gdialservice.cpp @@ -122,6 +122,7 @@ static void signal_handler_rest_server_gmainloop_quit(GDialRestServer *dial_rest //g_main_loop_quit(m_gdialServiceImpl->m_main_loop); } static GDialRestServer *dial_rest_server = NULL; +static GMainContext *server_main_context = NULL; static void server_activation_handler(gboolean status, const gchar *friendlyname) { @@ -133,9 +134,18 @@ static void server_activation_handler(gboolean status, const gchar *friendlyname } } -static void server_register_application(gpointer data) +struct RegisterApplicationsDispatch { + explicit RegisterApplicationsDispatch(gpointer value) : data(value) {} + gpointer data; + std::mutex mutex; + std::condition_variable condition; + bool complete{false}; +}; + +static gboolean server_register_application_on_main(gpointer data) { - GList* g_app_list = (GList*) data; + RegisterApplicationsDispatch *dispatch = static_cast(data); + GList* g_app_list = static_cast(dispatch->data); GDIAL_LOGTRACE("Entering ..."); GDIAL_LOGINFO("server_register_application callback "); if(g_app_list) { @@ -164,6 +174,28 @@ static void server_register_application(gpointer data) } } GDIAL_LOGTRACE("Exiting ..."); + { + std::lock_guard lock(dispatch->mutex); + dispatch->complete = true; + } + dispatch->condition.notify_one(); + return G_SOURCE_REMOVE; +} + +static void server_register_application(gpointer data) +{ + if (!server_main_context || g_main_context_is_owner(server_main_context)) { + RegisterApplicationsDispatch dispatch{data}; + server_register_application_on_main(&dispatch); + return; + } + RegisterApplicationsDispatch dispatch{data}; + GSource *source = g_idle_source_new(); + g_source_set_callback(source, server_register_application_on_main, &dispatch, NULL); + g_source_attach(source, server_main_context); + g_source_unref(source); + std::unique_lock lock(dispatch.mutex); + dispatch.condition.wait(lock, [&dispatch]{ return dispatch.complete; }); } static void server_friendlyname_handler(const gchar * friendlyname) @@ -272,6 +304,7 @@ int gdialServiceImpl::start_GDialServer(int argc, char *argv[]) //m_main_loop_context = g_main_context_default(); m_main_loop_context = g_main_context_new(); + server_main_context = m_main_loop_context; g_main_context_push_thread_default(m_main_loop_context); m_main_loop = g_main_loop_new(m_main_loop_context, FALSE); gdial_plat_init(m_main_loop_context); @@ -597,6 +630,7 @@ bool gdialServiceImpl::stop_GDialServer() if (m_main_loop_context) { + server_main_context = NULL; g_main_context_unref(m_main_loop_context); m_main_loop_context = NULL; } diff --git a/server/plat/gdial.cpp b/server/plat/gdial.cpp index 4876b746..c37c81dc 100644 --- a/server/plat/gdial.cpp +++ b/server/plat/gdial.cpp @@ -513,6 +513,23 @@ map parse_query(const char* query_string) { return ret; } +static bool is_authorized_system_action(const map& parsed_query) { + const char *system_key = getenv("SYSTEM_SLEEP_REQUEST_KEY"); + auto supplied = parsed_query.find("key"); + if (!system_key || system_key[0] == '\0' || supplied == parsed_query.end()) { + return false; + } + const string expected(system_key); + if (expected.size() != supplied->second.size()) { + return false; + } + unsigned char difference = 0; + for (size_t i = 0; i < expected.size(); ++i) { + difference |= static_cast(expected[i] ^ supplied->second[i]); + } + return difference == 0; +} + int gdial_os_application_start(const char *app_name, const char *payload, const char *query_string, const char *additional_data_url, int *instance_id) { GDIAL_LOGTRACE("Entering ..."); GDIAL_LOGINFO("App launch req: appName[%s] query[%s], payload[%s], additionalDataUrl [%s] instance[%p]", @@ -521,9 +538,8 @@ int gdial_os_application_start(const char *app_name, const char *payload, const if (strcmp(app_name,"system") == 0) { auto parsed_query{parse_query(query_string)}; if (parsed_query["action"] == "sleep") { - const char *system_key = getenv("SYSTEM_SLEEP_REQUEST_KEY"); - if (system_key && parsed_query["key"] != system_key) { - GDIAL_LOGINFO("system app request to change device to sleep mode, key comparison failed: user provided '%s'", parsed_query["key"].c_str()); + if (!is_authorized_system_action(parsed_query)) { + GDIAL_LOGINFO("system app request to change device to sleep mode, authorization failed"); GDIAL_LOGTRACE("Exiting ..."); return GDIAL_APP_ERROR_INTERNAL; } @@ -533,9 +549,8 @@ int gdial_os_application_start(const char *app_name, const char *payload, const return GDIAL_APP_ERROR_NONE; } else if (parsed_query["action"] == "togglepower") { - const char *system_key = getenv("SYSTEM_SLEEP_REQUEST_KEY"); - if (system_key && parsed_query["key"] != system_key) { - GDIAL_LOGINFO("system app request to toggle the power state, key comparison failed: user provided '%s'", parsed_query["key"].c_str()); + if (!is_authorized_system_action(parsed_query)) { + GDIAL_LOGINFO("system app request to toggle the power state, authorization failed"); GDIAL_LOGTRACE("Exiting ..."); return GDIAL_APP_ERROR_INTERNAL; } diff --git a/server/plat/gdial_app_registry.c b/server/plat/gdial_app_registry.c index 2ef69c77..950b1fb1 100644 --- a/server/plat/gdial_app_registry.c +++ b/server/plat/gdial_app_registry.c @@ -27,6 +27,19 @@ #define GDIAL_STR_ENDS_WITH(s1, s2) ((s1 != NULL) && (s2 != NULL) && ((strlen(s2) == 0) || (g_str_has_suffix(s1, s2)))) +static gboolean gdial_app_name_is_safe(const gchar *app_name) { + const gchar *cursor; + if (!app_name || !app_name[0] || !g_utf8_validate(app_name, -1, NULL)) { + return FALSE; + } + for (cursor = app_name; *cursor; ++cursor) { + if (!(g_ascii_isalnum(*cursor) || *cursor == '.' || *cursor == '_' || *cursor == '-')) { + return FALSE; + } + } + return g_strcmp0(app_name, ".") != 0 && g_strcmp0(app_name, "..") != 0; +} + void gdial_app_regstry_dispose (GDialAppRegistry *app_registry) { g_return_if_fail(app_registry != NULL); GDIAL_LOGINFO("freeing app name:%s", app_registry->name); @@ -65,7 +78,7 @@ gboolean gdial_app_registry_is_allowed_origin(GDialAppRegistry *app_registry, co } GDialAppRegistry* gdial_app_registry_new (const gchar *app_name, const GList *app_prefixes, GHashTable *properties, gboolean is_singleton, gboolean use_additional_data, const GList *allowed_origins) { - g_return_val_if_fail(app_name != NULL, NULL); + g_return_val_if_fail(gdial_app_name_is_safe(app_name), NULL); g_return_val_if_fail(is_singleton, NULL); GDialAppRegistry *app_registry = (GDialAppRegistry *)malloc(sizeof(*app_registry)); diff --git a/server/plat/gdialappcache.cpp b/server/plat/gdialappcache.cpp index dfd08dde..859427ac 100644 --- a/server/plat/gdialappcache.cpp +++ b/server/plat/gdialappcache.cpp @@ -62,7 +62,6 @@ void GDialAppStatusCache :: setAppCacheId(std::string app_name,std::string id) AppCacheErrorCodes GDialAppStatusCache::UpdateAppStatusCache(AppInfo* appEntry) { GDIAL_LOGTRACE("Entering ..."); - AppCacheErrorCodes err; GDIAL_LOGINFO("APPCache: AppName[%s] AppID[%s] AppState[%s] Error[%s]", appEntry->appName.c_str(), appEntry->appId.c_str(), @@ -71,11 +70,7 @@ AppCacheErrorCodes GDialAppStatusCache::UpdateAppStatusCache(AppInfo* appEntry) std::string id = getAppCacheId(appEntry->appName.c_str()); - if(doIdExist(id)) { - GDIAL_LOGINFO("erasing old data"); - err = ObjectCache->erase(id); - } - err = ObjectCache->insert(std::move(id),appEntry); + AppCacheErrorCodes err = ObjectCache->replace(std::move(id), appEntry); GDIAL_LOGTRACE("Exiting ..."); return err; } @@ -86,15 +81,14 @@ std::string GDialAppStatusCache::SearchAppStatusInCache(const char* app_name) std::string state = "NOT_FOUND"; std::string id = getAppCacheId(app_name); - if(doIdExist(id)) + AppInfo appEntry("", "", "", ""); + if(ObjectCache->findObject(id, appEntry)) { - AppInfo* appEntry = ObjectCache->findObject(id); - - state = appEntry->appState; + state = appEntry.appState; GDIAL_LOGINFO("APPCache: App Name[%s] AppID[%s] Error[%s]", - appEntry->appName.c_str(), - appEntry->appId.c_str(), - appEntry->appError.c_str()); + appEntry.appName.c_str(), + appEntry.appId.c_str(), + appEntry.appError.c_str()); } GDIAL_LOGINFO("App State = %s ",state.c_str()); GDIAL_LOGTRACE("Exiting ..."); diff --git a/server/plat/gdialobjCacheHelper.hpp b/server/plat/gdialobjCacheHelper.hpp index f7286d54..fefc31e3 100644 --- a/server/plat/gdialobjCacheHelper.hpp +++ b/server/plat/gdialobjCacheHelper.hpp @@ -25,6 +25,7 @@ #include #include #include +#include #include "gdialservicelogging.h" enum AppCacheErrorCodes { @@ -56,6 +57,7 @@ class GDialObjectCacheHelper public: GDialObjectCacheHelper(){ } ~GDialObjectCacheHelper() { + std::lock_guard lock(objectsMutex); for (auto& entry : objects) { if (entry.second) { @@ -65,22 +67,25 @@ class GDialObjectCacheHelper } } - AppInfo* findObject(const std::string& appName) const + bool findObject(const std::string& appName, AppInfo& entry) const { GDIAL_LOGTRACE("Entering ..."); + std::lock_guard lock(objectsMutex); auto it = objects.find(appName); - if (it != objects.end()) + if (it != objects.end() && it->second) { + entry = *it->second; GDIAL_LOGTRACE("Exiting ..."); - return it->second; + return true; } GDIAL_LOGTRACE("Exiting ..."); - return nullptr; + return false; } AppCacheErrorCodes insert(std::string id, AppInfo* entry) { GDIAL_LOGTRACE("Entering ..."); + std::lock_guard lock(objectsMutex); AppCacheErrorCodes returnValue = AppCacheError_NULL_ENTRY; if (nullptr != entry) { @@ -98,10 +103,34 @@ class GDialObjectCacheHelper return returnValue; } + AppCacheErrorCodes replace(std::string id, AppInfo* entry) + { + GDIAL_LOGTRACE("Entering ..."); + if (nullptr == entry) + { + return AppCacheError_NULL_ENTRY; + } + + std::lock_guard lock(objectsMutex); + auto it = objects.find(id); + if (it != objects.end()) + { + delete it->second; + it->second = entry; + } + else + { + objects.emplace(std::move(id), entry); + } + GDIAL_LOGTRACE("Exiting ..."); + return AppCacheError_OK; + } + AppCacheErrorCodes touch(const std::string& appname) { AppCacheErrorCodes returnValue = AppCacheError_NOT_FOUND; GDIAL_LOGTRACE("Entering [%s]...",appname.c_str()); + std::lock_guard lock(objectsMutex); auto it = objects.find(appname); if (it != objects.end()) { @@ -117,6 +146,7 @@ class GDialObjectCacheHelper { AppCacheErrorCodes returnValue = AppCacheError_NOT_FOUND; GDIAL_LOGTRACE("Entering ..."); + std::lock_guard lock(objectsMutex); auto it = objects.find(appname); if (it != objects.end()) { @@ -129,5 +159,6 @@ class GDialObjectCacheHelper } private: std::unordered_map objects; + mutable std::mutex objectsMutex; }; #endif diff --git a/tests/test_security_cache.py b/tests/test_security_cache.py new file mode 100644 index 00000000..926eabed --- /dev/null +++ b/tests/test_security_cache.py @@ -0,0 +1,19 @@ +import pathlib +import unittest + + +class CacheSecurityGuardrail(unittest.TestCase): + def test_cache_operations_are_locked_and_reads_are_copied(self): + root = pathlib.Path(__file__).parents[1] + helper = (root / "server/plat/gdialobjCacheHelper.hpp").read_text() + caller = (root / "server/plat/gdialappcache.cpp").read_text() + self.assertGreaterEqual(helper.count("std::lock_guard lock(objectsMutex);"), 5) + self.assertIn("bool findObject(const std::string& appName, AppInfo& entry) const", helper) + self.assertIn("entry = *it->second;", helper) + self.assertIn("AppCacheErrorCodes replace(std::string id, AppInfo* entry)", helper) + self.assertIn("ObjectCache->replace(std::move(id), appEntry)", caller) + self.assertNotIn("AppInfo* appEntry = ObjectCache->findObject", caller) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_security_dial_data_paths.py b/tests/test_security_dial_data_paths.py new file mode 100644 index 00000000..00820442 --- /dev/null +++ b/tests/test_security_dial_data_paths.py @@ -0,0 +1,20 @@ +import pathlib +import unittest + + +class DialDataPathGuardrail(unittest.TestCase): + def test_registry_rejects_unsafe_application_names(self): + source = (pathlib.Path(__file__).parents[1] / "server/plat/gdial_app_registry.c").read_text() + self.assertIn("static gboolean gdial_app_name_is_safe", source) + self.assertIn("g_return_val_if_fail(gdial_app_name_is_safe(app_name), NULL);", source) + + def test_file_sinks_require_safe_application_names(self): + source = (pathlib.Path(__file__).parents[1] / "server/gdial-app.c").read_text() + self.assertIn("g_ascii_isalnum(*cursor)", source) + self.assertEqual(source.count("g_return_val_if_fail(gdial_app_name_is_safe(app_name), FALSE);"), 3) + for value in ('*cursor == \'/\'', '*cursor == \'\\\\\''): + self.assertNotIn(value, source[source.index("static gboolean gdial_app_name_is_safe"):source.index("static guint gdial_app_signals")]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_security_registration_dispatch.py b/tests/test_security_registration_dispatch.py new file mode 100644 index 00000000..79f019ef --- /dev/null +++ b/tests/test_security_registration_dispatch.py @@ -0,0 +1,18 @@ +import pathlib +import unittest + + +class RegistrationDispatchGuardrail(unittest.TestCase): + def test_registry_mutation_runs_on_main_context(self): + source = (pathlib.Path(__file__).parents[1] / "server/gdialservice.cpp").read_text() + callback = source.index("server_register_application_on_main") + unregister = source.index("gdial_rest_server_unregister_all_apps", callback) + attach = source.index("g_source_attach(source, server_main_context)", unregister) + wait = source.index("dispatch.condition.wait", attach) + self.assertLess(callback, unregister) + self.assertLess(attach, wait) + self.assertIn("g_main_context_is_owner(server_main_context)", source) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_security_system_actions.py b/tests/test_security_system_actions.py new file mode 100644 index 00000000..f379ccf4 --- /dev/null +++ b/tests/test_security_system_actions.py @@ -0,0 +1,15 @@ +import pathlib +import unittest + + +class SystemActionGuardrail(unittest.TestCase): + def test_power_actions_fail_closed(self): + source = (pathlib.Path(__file__).parents[1] / "server/plat/gdial.cpp").read_text() + self.assertIn("if (!system_key || system_key[0] == '\\0' || supplied == parsed_query.end())", source) + self.assertEqual(source.count("if (!is_authorized_system_action(parsed_query))"), 2) + self.assertNotIn("user provided '%s'", source) + self.assertIn("difference |= static_cast", source) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_security_uri_validation.py b/tests/test_security_uri_validation.py new file mode 100644 index 00000000..066dbaff --- /dev/null +++ b/tests/test_security_uri_validation.py @@ -0,0 +1,16 @@ +import pathlib +import unittest + + +class UriValidationGuardrail(unittest.TestCase): + def test_empty_path_components_are_rejected_before_copy(self): + source = (pathlib.Path(__file__).parents[1] / "server/gdial-rest.c").read_text() + loop = source.index("for (i = 0; elements[i] != NULL; i++)", source.rindex("g_strsplit(&path[1]")) + copy = source.index("g_strlcpy(base", loop) + rejection = source.index("if (elements[i][0] == '\\0')", loop) + self.assertLess(rejection, copy) + self.assertIn("while (elements[j] != NULL && i < element_num", source) + + +if __name__ == "__main__": + unittest.main()