-
Notifications
You must be signed in to change notification settings - Fork 75
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix UAF on listener arg on event during enable()
The C++ code first installs a no-op listener in the entity, then completes construction of the C++ object and only then sets the application listener. The dds_set_listener operation then invokes the listeners for any events that happened before the application listener was installed. This is different from not installing a no-op listener: if there is no listener, an event is propagates up the entity hierarchy, checking whether any of those has a listener installed. If there is one, it is invoked and the event is considered handled, and the status flag cleared (leaving no trace of the event). If the application is trying to install a listener for the event, then this results in invoking the wrong listener. Cyclone offers a mode in which the status flag is not cleared. This means one can define a set of listener functions that do absolutely nothing and without clearing the status flags. The code re-used its normal callback functions. Those inspect the listener argument, and during construction of the entity there is a window where the argument for the no-op listener is freed but the callback functions remain installed. This fixes that by using separate functions that do nothing so that we don't have to touch the listener callbacks/argument data maintained in the C++ binding. The normal functions used to check the "reset on invoke" flag that is used to enable/disable clearing the status flags for deciding whether or not to actually do anything. The standard API doesn't allow this mode of not clearing the status flag on listener invocation, so it could be abused in this manner. With the introduction of a separate set of no-op functions, the need for this abuse disappeared. That, too, has been fixed. Details of the UAF: 1. When creating a data writer, it will first use EntityDelegate::listener_set to set the listener_callbacks to the special "no-op" mode of the normal listener C++ functions. 2. It then updates the listener_callbacks to those of the "real" listener (again using EntityDelegate::listener_set). The entity is not yet "enabled" and the call to the C API dds_set_listener is skipped. 3. It then frees the previous listener_callbacks with its argument. 4. If an event causing a listener invocation happens now, the callback function is called with the freed argument pointer because the C API still references it. Thanks to @ruoruoniao for the discovery and root-cause analysis. Signed-off-by: Erik Boasson <[email protected]>
- Loading branch information
Showing
9 changed files
with
104 additions
and
20 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
src/ddscxx/include/org/eclipse/cyclonedds/core/NoopListener.hpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // Copyright(c) 2024 ZettaScale Technology and others | ||
| // | ||
| // This program and the accompanying materials are made available under the | ||
| // terms of the Eclipse Public License v. 2.0 which is available at | ||
| // http://www.eclipse.org/legal/epl-2.0, or the Eclipse Distribution License | ||
| // v. 1.0 which is available at | ||
| // http://www.eclipse.org/org/documents/edl-v10.php. | ||
| // | ||
| // SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause | ||
|
|
||
| #ifndef CYCLONEDDS_CORE_NOOPLISTENER_H_ | ||
| #define CYCLONEDDS_CORE_NOOPLISTENER_H_ | ||
|
|
||
| #include <memory> | ||
| #include <functional> | ||
| #include "dds/dds.h" | ||
|
|
||
| #include "dds/core/macros.hpp" | ||
|
|
||
| namespace org { namespace eclipse { namespace cyclonedds { namespace core { | ||
|
|
||
| extern OMG_DDS_API std::unique_ptr<dds_listener_t, std::function<void(dds_listener_t *)>> make_noop_listener(); | ||
|
|
||
| } } } } | ||
|
|
||
| #endif /* CYCLONEDDS_CORE_NOOPLISTENER_H_ */ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
54 changes: 54 additions & 0 deletions
54
src/ddscxx/src/org/eclipse/cyclonedds/core/NoopListener.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // Copyright(c) 2024 ZettaScale Technology and others | ||
| // | ||
| // This program and the accompanying materials are made available under the | ||
| // terms of the Eclipse Public License v. 2.0 which is available at | ||
| // http://www.eclipse.org/legal/epl-2.0, or the Eclipse Distribution License | ||
| // v. 1.0 which is available at | ||
| // http://www.eclipse.org/org/documents/edl-v10.php. | ||
| // | ||
| // SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause | ||
|
|
||
| #include "org/eclipse/cyclonedds/core/NoopListener.hpp" | ||
|
|
||
| #ifdef _WIN32_DLL_ | ||
| #define DDS_FN_EXPORT __declspec (dllexport) | ||
| #else | ||
| #define DDS_FN_EXPORT | ||
| #endif | ||
|
|
||
| namespace org { namespace eclipse { namespace cyclonedds { namespace core { | ||
|
|
||
| static void callback_on_inconsistent_topic_noop (dds_entity_t, dds_inconsistent_topic_status_t, void *) { } | ||
| static void callback_on_offered_deadline_missed_noop (dds_entity_t, dds_offered_deadline_missed_status_t, void *) { } | ||
| static void callback_on_offered_incompatible_qos_noop (dds_entity_t, dds_offered_incompatible_qos_status_t, void *) { } | ||
| static void callback_on_liveliness_lost_noop (dds_entity_t, dds_liveliness_lost_status_t, void *) { } | ||
| static void callback_on_publication_matched_noop (dds_entity_t, dds_publication_matched_status_t, void *) { } | ||
| static void callback_on_requested_deadline_missed_noop (dds_entity_t, dds_requested_deadline_missed_status_t, void *) { } | ||
| static void callback_on_requested_incompatible_qos_noop (dds_entity_t, dds_requested_incompatible_qos_status_t, void *) { } | ||
| static void callback_on_sample_rejected_noop (dds_entity_t, dds_sample_rejected_status_t, void *) { } | ||
| static void callback_on_liveliness_changed_noop (dds_entity_t, dds_liveliness_changed_status_t, void *) { } | ||
| static void callback_on_data_available_noop (dds_entity_t, void *) { } | ||
| static void callback_on_subscription_matched_noop (dds_entity_t, dds_subscription_matched_status_t, void *) { } | ||
| static void callback_on_sample_lost_noop (dds_entity_t, dds_sample_lost_status_t, void *) { } | ||
| static void callback_on_data_readers_noop (dds_entity_t, void *) { } | ||
|
|
||
| DDS_FN_EXPORT std::unique_ptr<dds_listener_t, std::function<void(dds_listener_t *)>> make_noop_listener() | ||
| { | ||
| dds_listener_t *callbacks = dds_create_listener(nullptr); | ||
| dds_lset_inconsistent_topic_arg(callbacks, callback_on_inconsistent_topic_noop, nullptr, false); | ||
| dds_lset_offered_deadline_missed_arg(callbacks, callback_on_offered_deadline_missed_noop, nullptr, false); | ||
| dds_lset_offered_incompatible_qos_arg(callbacks, callback_on_offered_incompatible_qos_noop, nullptr, false); | ||
| dds_lset_liveliness_lost_arg(callbacks, callback_on_liveliness_lost_noop, nullptr, false); | ||
| dds_lset_publication_matched_arg(callbacks, callback_on_publication_matched_noop, nullptr, false); | ||
| dds_lset_requested_deadline_missed_arg(callbacks, callback_on_requested_deadline_missed_noop, nullptr, false); | ||
| dds_lset_requested_incompatible_qos_arg(callbacks, callback_on_requested_incompatible_qos_noop, nullptr, false); | ||
| dds_lset_sample_rejected_arg(callbacks, callback_on_sample_rejected_noop, nullptr, false); | ||
| dds_lset_liveliness_changed_arg(callbacks, callback_on_liveliness_changed_noop, nullptr, false); | ||
| dds_lset_data_available_arg(callbacks, callback_on_data_available_noop, nullptr, false); | ||
| dds_lset_subscription_matched_arg(callbacks, callback_on_subscription_matched_noop, nullptr, false); | ||
| dds_lset_sample_lost_arg(callbacks, callback_on_sample_lost_noop, nullptr, false); | ||
| dds_lset_data_on_readers_arg(callbacks, callback_on_data_readers_noop, nullptr, false); | ||
| return std::unique_ptr<dds_listener_t, std::function<void(dds_listener_t *)>>(callbacks, &dds_delete_listener); | ||
| } | ||
|
|
||
| } } } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters