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)
6.3
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
Environment:
Last Closed: 2014-03-27 12:43:21 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
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:
http://git.fedorahosted.org/cgit/conga.git/commit/?h=bz855121

The (sort of) unit test is committed separately into the master:
http://git.fedorahosted.org/cgit/conga.git/commit/?id=35c7af4

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

BEFORE:

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"
foo&amp;bar
foo&amp;bar
foo&bar

Original information got changed within escape-unescape cycle.

AFTER:

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

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-COMPILE: INVERT=1 KEENESCAPE=1
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-COMPILE: INVERT=1 KEENESCAPE=0
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-COMPILE: INVERT=0 KEENESCAPE=1
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-COMPILE: INVERT=0 KEENESCAPE=0
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-
   -formedness")

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
bz855121.

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.