Bug 855117
| Summary: | ccs: harmful handling of XML entities | ||
|---|---|---|---|
| Product: | Red Hat Enterprise Linux 6 | Reporter: | Jan Pokorný [poki] <jpokorny> |
| Component: | ricci | Assignee: | Chris Feist <cfeist> |
| Status: | CLOSED DUPLICATE | QA Contact: | Cluster QE <mspqa-list> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | 6.3 | CC: | cluster-maint, fdinitto, mlazar |
| Target Milestone: | rc | ||
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | 855112 | Environment: | |
| Last Closed: | 2014-03-27 16:44:42 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | 855112, 855121 | ||
| Bug Blocks: | |||
|
Description
Jan Pokorný [poki]
2012-09-06 18:10:37 UTC
I proposed a (roughly tested) patch in (temporary) branch upstream: http://git.fedorahosted.org/cgit/conga.git/commit/?h=bz855117 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]. 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()). 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. Thanks, Chris 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 *** |