Red Hat Bugzilla – Bug 855117
ccs: harmful handling of XML entities
Last modified: 2014-03-27 12:44:42 EDT
+++ This bug was initially created as a clone of Bug #855112 +++
In connection to case discussed in the public cluster ML , 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.
I proposed a (roughly tested) patch in (temporary) branch upstream:
Better patch pushed to the referenced branch (previous version branch
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].
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
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.
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).
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.
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?
*** Bug 987765 has been marked as a duplicate of this bug. ***
*** This bug has been marked as a duplicate of bug 855112 ***