Bug 855117 - ccs: harmful handling of XML entities
ccs: 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: Chris Feist
Cluster QE
:
: 987765 (view as bug list)
Depends On: 855112 855121
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 14:10 EDT by Jan Pokorný
Modified: 2014-03-27 12:44 EDT (History)
3 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:44:42 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)

  None (edit)
Description Jan Pokorný 2012-09-06 14:10:37 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.

[1] https://www.redhat.com/archives/linux-cluster/2012-August/msg00135.html
Comment 1 Jan Pokorný 2012-09-07 19:24:32 EDT
I proposed a (roughly tested) patch in (temporary) branch upstream:
http://git.fedorahosted.org/cgit/conga.git/commit/?h=bz855117
Comment 3 Jan Pokorný 2012-09-11 09:54:42 EDT
Better patch pushed to the referenced branch (previous version branch
removed).

Sidenote: there is no need to preserve existing & etc. when
escaping the values as when such entity is present, it means just
this sequence of characters (so & should get escaped as well).
Stating this, as it used to be a behavior in ricci, but
it is to be removed as well [bug 855121].
Comment 4 Jan Pokorný 2012-09-13 12:10:15 EDT
Previous solutions were over-approximations of the correct fix.

The referenced branch should now contain the Right Thing.
Only the password needs to be escaped (postponed for when really
needed), other variables in the resulting request XML are set
directly in to the XML object which gets this right upon its
serialization (toxml()).
Comment 5 Chris Feist 2012-09-17 19:00:30 EDT
You should only be changing code in the check_authentication function all of the other changes could potentially affect other parts of the code, this patch should only fix issues from this bz.

Also, it looks like there is no check for '<', '>' and '&'.  Those need to be added to the escape.
Comment 6 Jan Pokorný 2012-09-18 06:57:21 EDT
re [comment 5]

> You should only be changing code in the check_authentication function
> all of the other changes could potentially affect other parts of
> the code, this patch should only fix issues from this bz.

Well, beside the main change, I made a (very conservative -- only around
the parts being changed) alignment of code closer to the better shape/less
debt (unused imports, more natural indentation).  It's unlikely this could
cause any issue and also unlikely such debt could be diminished by other
means.  Unfortunately, the unit test did not work for me, both before
and after the change.

> Also, it looks like there is no check for '<', '>' and '&'.  Those need
> to be added to the escape.

They indeed are handled in escape function, implicitly by its definition.
The explicit ones are added so the complete set is treated the same way,
just as what libxml2 does with attribute values automatically (so all
the communicating sides has unified approach).
Comment 7 Chris Feist 2012-09-18 09:48:27 EDT
Ahh, just double checked that escape function, ok we should be good to go with that.  Just remove all the extra unrelated changes and I'll pull the patch in.

Thanks,
Chris
Comment 8 Jan Pokorný 2012-09-26 17:34:23 EDT
Ok, unrelated stuff has to wait for other occasion (will post a separate
patch), but now when ricci protocol bump seems to happen, it makes sense
to read such version sent by ricci properly, adjust single/double escaping
of password respectively and also to issue a warning about recommended
update of ricci+modcluster (unfortunately, some refactoring was needed
so send_to_ricci depends on already initialized secure socket it gets
as a parameter).

I refreshed the referred branch with such updated changeset.
Does it make sense?
Comment 13 Jan Pokorný 2013-08-12 10:17:43 EDT
*** Bug 987765 has been marked as a duplicate of this bug. ***
Comment 15 Fabio Massimo Di Nitto 2014-03-27 12:44:42 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.