Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UAF on listener arg on event during enable() #495

Merged

Conversation

eboasson
Copy link
Contributor

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.

Fixes #493

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]>
@ruoruoniao
Copy link
Contributor

ruoruoniao commented Jun 28, 2024

I'm very glad to see that fix works well~ Thx~O(∩_∩)O

@eboasson eboasson merged commit 15949d4 into eclipse-cyclonedds:master Jun 28, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random crash when create DataReader/DataWriter...
2 participants