Replace eldbus with gdbus #150
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the eldbus library with gdbus for D-Bus communication within the accessibility channel, updating build configurations and refactoring the C++ implementation. However, it introduces critical and high-severity command injection vulnerabilities. A critical issue exists in tools/generate_sysroot.py due to improper handling of filenames in shell commands, and a high-severity vulnerability is present in flutter/shell/platform/tizen/channels/accessibility_channel.cc where untrusted input from the Flutter engine is used in a D-Bus method call without sufficient sanitization. Additionally, the new gdbus code could be improved by leveraging g_autoptr for automatic resource management, which is a GLib best practice that will enhance code safety and maintainability.
| EncodableValueHolder<std::string> msg(data.value, "message"); | ||
| if (msg && accessibility_bus_) { | ||
| Eldbus_Message* eldbus_message = eldbus_message_method_call_new( | ||
| kAtspiDirectReadBus, kAtspiDirectReadPath, | ||
| kAtspiDirectReadInterface, "ReadCommand"); | ||
| Eldbus_Message_Iter* iter = eldbus_message_iter_get(eldbus_message); | ||
| eldbus_message_iter_arguments_append(iter, "sb", msg->c_str(), | ||
| true); | ||
| eldbus_connection_send(accessibility_bus_, eldbus_message, nullptr, | ||
| nullptr, -1); | ||
| GVariant* params = g_variant_new("(sb)", msg->c_str(), TRUE); | ||
| g_dbus_connection_call( | ||
| accessibility_bus_, kAtspiDirectReadBus, kAtspiDirectReadPath, | ||
| kAtspiDirectReadInterface, "ReadCommand", params, nullptr, | ||
| G_DBUS_CALL_FLAGS_NONE, -1, nullptr, nullptr, nullptr); |
There was a problem hiding this comment.
The msg variable, which can contain untrusted input from the Flutter engine, is directly used in a D-Bus method call to ReadCommand. This could lead to command injection if the accessibility service interprets the input as commands. Implement robust input validation and sanitization for msg before it is used in the D-Bus method call. Ensure that the ReadCommand method on the org.tizen.DirectReading interface is designed to handle untrusted input securely, or consider implementing an allow-list for acceptable commands or data patterns. If possible, avoid directly passing unsanitized user input to system-level commands or interfaces.
| static void _accessibilityBusAddressGet(GObject* source_object, | ||
| GAsyncResult* res, | ||
| gpointer user_data) { | ||
| GError* error = nullptr; | ||
| GVariant* result = nullptr; | ||
| GDBusConnection** accessibility_bus = | ||
| static_cast<GDBusConnection**>(user_data); | ||
|
|
||
| GDBusProxy* proxy = G_DBUS_PROXY(source_object); | ||
| result = g_dbus_proxy_call_finish(proxy, res, &error); | ||
|
|
||
| if (error) { | ||
| FT_LOG(Error) << "GDBus message error. (" << error->message << ")"; | ||
| g_error_free(error); | ||
| return; | ||
| } | ||
|
|
||
| if (!eldbus_message_arguments_get(message, "s", &socket_address) || | ||
| !socket_address) { | ||
| const gchar* socket_address = nullptr; | ||
| if (g_variant_is_of_type(result, G_VARIANT_TYPE("(s)"))) { | ||
| g_variant_get(result, "(&s)", &socket_address); | ||
| } | ||
|
|
||
| if (!socket_address) { | ||
| FT_LOG(Error) << "Could not get A11Y Bus socket address."; | ||
| g_variant_unref(result); | ||
| return; | ||
| } | ||
| *accessibility_bus = eldbus_private_address_connection_get(socket_address); | ||
|
|
||
| *accessibility_bus = g_dbus_connection_new_for_address_sync( | ||
| socket_address, G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, nullptr, | ||
| nullptr, &error); | ||
|
|
||
| if (error) { | ||
| FT_LOG(Error) << "Failed to connect to A11Y Bus: " << error->message; | ||
| g_error_free(error); | ||
| } | ||
|
|
||
| g_variant_unref(result); | ||
| } |
There was a problem hiding this comment.
This function can be greatly simplified by using g_autoptr for automatic resource management of GLib objects. This avoids manual cleanup calls (g_error_free, g_variant_unref), making the code cleaner, safer, and more maintainable. The logic can also be structured to have clearer return paths.
static void _accessibilityBusAddressGet(GObject* source_object,
GAsyncResult* res,
gpointer user_data) {
g_autoptr(GError) error = nullptr;
g_autoptr(GVariant) result = nullptr;
GDBusConnection** accessibility_bus =
static_cast<GDBusConnection**>(user_data);
GDBusProxy* proxy = G_DBUS_PROXY(source_object);
result = g_dbus_proxy_call_finish(proxy, res, &error);
if (error) {
FT_LOG(Error) << "GDBus message error. (" << error->message << ")";
return;
}
const gchar* socket_address = nullptr;
if (g_variant_is_of_type(result, G_VARIANT_TYPE("(s)"))) {
g_variant_get(result, "(&s)", &socket_address);
}
if (!socket_address) {
FT_LOG(Error) << "Could not get A11Y Bus socket address.";
return;
}
*accessibility_bus = g_dbus_connection_new_for_address_sync(
socket_address, G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, nullptr,
nullptr, &error);
if (error) {
FT_LOG(Error) << "Failed to connect to A11Y Bus: " << error->message;
}
}| kChannelName, | ||
| &StandardMessageCodec::GetInstance())) { | ||
| eldbus_init(); | ||
| GError* error = nullptr; |
There was a problem hiding this comment.
Consider using g_autoptr(GError) error = nullptr; to simplify error handling. This will automatically free the GError object when it goes out of scope, removing the need for the manual g_error_free calls at lines 74 and 84.
g_autoptr(GError) error = nullptr;1 participant
#148