Bug 855121 - ricci: harmful handling of XML entities
ricci: harmful handling of XML entities
Status: CLOSED DUPLICATE of bug 855112
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: ricci (Show other bugs)
Unspecified Unspecified
medium Severity medium
: rc
: ---
Assigned To: Jan Pokorný
Cluster QE
: 949566 (view as bug list)
Depends On: 858386
Blocks: 855112 855117 855120
  Show dependency treegraph
Reported: 2012-09-06 14:19 EDT by Jan Pokorný
Modified: 2014-03-27 12:43 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 855112
Last Closed: 2014-03-27 12:43:21 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Knowledge Base (Solution) 198233 None None None Never

  None (edit)
Description Jan Pokorný 2012-09-06 14:19:45 EDT
+++ This bug was initially created as a clone of Bug #855112 +++

In connection to case discussed in the public cluster ML [1], we need
to make sure the user input that makes its way into XML (either as
cluster.conf or XML-formatted requests towards ricci) is XML-ready.
That is, the "unsafe" characters are encoded as XML entities.

Otherwise, such requests have no effect and unfortunately, neither
ricci provides a sensible diagnostics about this.  Example of such
input is the password for the initial authentization against ricci.


Specifically on the ricci's side, we need to be prepared for this
escaped input in order to convert it back where suitable (e.g.,
the mentioned case of authentication password).


[1] https://www.redhat.com/archives/linux-cluster/2012-August/msg00135.html
Comment 1 Jan Pokorný 2012-09-07 14:20:36 EDT
In fact, this seems to be an non-issue for ricci side as lon as
only &, <, >, ' and " are considered.

Then, only client-side work would be needed.
Comment 2 Jan Pokorný 2012-09-10 09:43:45 EDT
FWIW, a sort of unit test for the discussed XML entities un-/escaping
in ricci si now in upstream [2].  It was used to check that those
5 characters from [comment 1] are treated correctly.

[2] http://git.fedorahosted.org/cgit/conga.git/commit/?id=35c7af4
Comment 3 Jan Pokorný 2012-09-11 06:27:36 EDT
In fact, there is an issue with proactive skipping of already
present XML entities when performing the escaping as a part of
cooking the externalized XML.  Taking the circumstances into
account, we need to make sure there is not such extra treatment
and initial '&' of these entities is escaped as usual.

I proposed a (roughly tested) patch in (temporary) branch upstream:

The (sort of) unit test is committed separately into the master:

Using this test (see also the commit message for the fix):


ricci/test/xml_escape$ make check
./test-xmlescape | sed "s|\(.*\)|\1\n|g" | tee -a /dev/stderr | \
    ./test-xmlescape -i | sed "s|\(.*\)|\1\n|g"

Original information got changed within escape-unescape cycle.


$ make check
./test-xmlescape | sed "s|\(.*\)|\1\n|g" | tee -a /dev/stderr | \
    ./test-xmlescape -i | sed "s|\(.*\)|\1\n|g"

Information preserved.
Comment 5 Jan Pokorný 2012-09-11 11:50:11 EDT
Re [comment 3]:
This change may not be so important from common user-defined values
standpoint, but it becomes actual when the ricci password is generated
by some kind of PRNG and the resulting string, possibly containg
sequences like "&lt;", is just copied around whenever needed, without
any attention to its content.
This is the situation the patch helps to prevent.
Comment 6 Jan Pokorný 2012-09-18 16:27:52 EDT
In fact, the things are not got right on multiple levels,
"keen escaping" discussed in [comment 3] being one them.

I did a little experiment taking ricci codebase and playing with
common/XML.cpp and copying resulting ricci, ricci-worker, and modcluster
binaries in place of the ones from ricci-0.16.2-55.el6.x86_64.
Whenever un-/escaped string differed from its original, this was logged
into syslog for debugging purposes (see bellow).

Following conditional macros were used to enable/disable particular
parts of code:

INVERT: whether to use unescaping of the entities (i.e., use invert_chars
function); studying the behavior of xmllib2 tells that this is completely
unnecessary as this is done automatically (at least as far as attribute
values are concerned)

KEENESCAPE: whether to use original code or mimic the fix from [comment 3]

For this purpose, in relation to [comment 3], I put following XML snippet
into cluster.conf on the examined node:
> <fencedevices>
>   <fencedevice agent="fence_apc" ipaddr="localhost" login="root"
>                name="apcpowerswitch" passwd="foo&amp;amp;bar"/>
> </fencedevices>
then from the machine outside the cluster, I ran:
> ccs-overly -d -h mynode1 --getconf
which I used to verify for correctness of password form corresponding
to the snippet above (marked as CCS-RESULT below).


ricci: ricci-xml: foo&amp;bar -> foo&bar
ricci[660]: Executing '/usr/libexec/ricci/ricci-worker
            -f /var/lib/ricci/queue/1168596506'
modcluster: ricci-xml: foo&amp;bar -> foo&bar
modcluster: ricci-xml: foo&bar => foo&amp;bar
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar


ricci: ricci-xml: foo&amp;bar -> foo&bar
ricci[1926]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/1778838431'
modcluster: ricci-xml: foo&amp;bar -> foo&bar
modcluster: ricci-xml: foo&bar => foo&amp;bar
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar


ricci[3163]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/521582756'
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar


ricci[4309]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/1490636628'
modcluster: ricci-xml: foo&amp;bar => foo&amp;amp;bar
ricci-worker: ricci-xml: foo&amp;bar => foo&amp;amp;bar
ricci: ricci-xml: foo&amp;bar => foo&amp;amp;bar

CCS-RESULT: foo&amp;amp;bar


Based on this, the conclusion is:

1/ patch proposed in [comment 3] is half-way to a desired behavior

2/ there is no need for and it is even harmful to unescape entities
   (call invert_chars) as the attribute values provided by xmllib2
   are already unescaped ("raw" as opposed to "mangled for XML well-

3/ modcluster package (clustermon BZ component) is also affected
   -> [bug 858386]

Action items: 2/ and 3/ (will solve these shortly)
Comment 7 Jan Pokorný 2012-09-24 14:17:55 EDT
Based on [comment 6], I committed another changeset into referred branch

In addition, the version of top-level protocol was bumped to 1.0 -> 1.1.
Provided that it is likely more doable to update client components then
server one in usual deployments (n.b. very often different machines),
this allows for backward-compatible workaround of buggy versions of
ricci.  These are to be implemented as part of already opened bugs:

- ccs:      [bug 855117]
- ccs_sync: [bug 855120]
- luci:     [bug 855112]
Comment 11 Chris Feist 2012-10-12 14:31:11 EDT
Bumping to 6.5 since we're still waiting on an acked patch.
Comment 14 Shane Bradley 2013-09-17 10:09:01 EDT
*** Bug 949566 has been marked as a duplicate of this bug. ***
Comment 17 Fabio Massimo Di Nitto 2014-03-27 12:43:21 EDT

*** This bug has been marked as a duplicate of bug 855112 ***

Note You need to log in before you can comment on or make changes to this bug.