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

Noetic: Fix XMLRPC endless loop #2185

Merged
merged 5 commits into from
Sep 21, 2021
Merged

Conversation

clalancette
Copy link
Contributor

We've recently had a report of an endless loop in ros_comm. The short of it is that the hand-written XML parser in the XMLRPC portion of ros_comm can get stuck in a (seemingly) infinite loop when certain XMLRPC calls are delivered.

Looking in detail at the XML parser, it actually accepts a lot of pieces of XML that it really shouldn't. While we could go in and heavily tighten what it accepts, that risks breaking legitimate users of the XMLRPC functionality in the stable ROS distributions.

Instead, this PR just fixes one of the issues, which is enough to fix this particular endless loop. To understand how it fixes it requires a bit of understanding of XMLRPC and how the XML parser works.

XMLRPC allows for "structures", which are heterogeneous collections of name -> value pairs. Each field of the structure should have a <name> tag, along with a <value> tag. That is, the XML should be structured something like:

<value>
  <struct>
    <member>
      <name>name</name>
      <value>
        <string>value</string>
      </value>
    </member>
  </struct>
</value>

However, <name> tag parsing is done in an extremely simplistic way. In particular, it starts at a particular offset within the string, and then goes searching throughout the rest of the string for any substring matching <name>. This means that when given XML that looks like:

<value>
  <struct>
    <member>
      <value>
        <string>value</string>
      </value>
    </member>
  </struct>
</value>
<name>name</name>

the parser finds the <name> tag, even though it is in a completely bogus part of the XML. Combining this with some of the other recursive parsing (since arrays can be embedded in structs can be embedded in arrays etc.) means that we can confuse the parser and end up in the endless loop.

What this PR does is to introduce a new parsing function, called nextTagData. Ignoring whitespace, this function expects that the very next XML tag is the specified one (<name> in this case). If that isn't the case, it ends up failing, which is enough to break the loop and cause parsing to completely fail.

In terms of backwards compatibility, so far I haven't been able to come up with too much that this should affect (other than the attack, of course). But the parsing here is pretty hairy, so I may be missing something.

Besides the fix, this PR also adds in quite a few unit tests for the XMLRPC parsing functionality. This was done in an effort to both characterize the current state of the parser, and to ensure that the fix did not break existing functionality.

If you'd like to test any of this out yourself, I found the most targeted way to do this is to:

$ source /opt/ros/noetic/setup.bash
$ git clone https://github.com/ros/ros_comm -b clalancette/fix-xmlrpc
$ mkdir -p ros_comm/utilities/xmlrpcpp/build
$ cd ros_comm/utilities/xmlrpcpp/build
$ cmake ..
$ make
$ make tests
$ LD_LIBRARY_PATH=./devel/lib CTEST_OUTPUT_ON_FAILURE=1 ctest

It claims to be "well-formed", but the closing tag was wrong.
Fix that here.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This includes parseTag, findTag, nextTagIs, and getNextTag.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@sloretz sloretz merged commit 41a956c into noetic-devel Sep 21, 2021
@sloretz sloretz deleted the clalancette/fix-xmlrpc branch September 21, 2021 17:26
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/new-packages-for-noetic-2021-09-27/22447/1

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.

None yet

4 participants