Bug 1104080

Summary:	Every SearchQuery is improperly evaluated as unsafe expression.
Product:	Red Hat Enterprise Virtualization Manager	Reporter:	Martin Mucha <mmucha>
Component:	ovirt-engine	Assignee:	Eli Mesika <emesika>
Status:	CLOSED CURRENTRELEASE	QA Contact:	Martin Mucha <mmucha>
Severity:	high	Docs Contact:
Priority:	unspecified
Version:	3.5.0	CC:	emesika, gklein, iheim, lpeer, mmucha, oourfali, pstehlik, rbalakri, Rhev-m-bugs, sherold, yeylon
Target Milestone:	---	Keywords:	CodeChange
Target Release:	3.5.0	Flags:	mmucha: needinfo-
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:	infra
Fixed In Version:	ovirt-engine-3.5.0_beta	Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2015-02-17 17:13:52 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	Infra	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	1142923, 1156165

Description Martin Mucha 2014-06-03 09:02:46 UTC

Description of problem:
*Every* SearchQuery is improperly evaluated as unsafe expression.

Version-Release number of selected component (if applicable):
3.5

How reproducible:
100%


Steps to Reproduce:
1. call GET on http://localhost:8080/api/vms or any other such 'getter' method and follow through mentioned methods.


Additional info:
in method
org.ovirt.engine.core.bll.SearchQuery#initQueryData
is this line:
isSafe = SearchObjects.isSafeExpression(searchText);
this line is currently always false, because internally it consists of searching through Map which is filled with <some_uppercase_constant> transformed to lowercase and then concatenated with colon. Example "vms:", "macpool:". The searching code looks like this:
<map>.contains(<input>.toLowerCase());

but that input is calculated here:
org.ovirt.engine.api.restapi.resource.AbstractBackendCollectionResource#getBackendCollection(org.ovirt.engine.core.common.interfaces.SearchType)
using code:
QueryHelper.getConstraint(getUriInfo(), "",  modelType)
which instead of previously mentioned examples produces "vms : ", "macpool : ". Reason is in incompatibilities among:
org.ovirt.engine.api.common.util.QueryHelper#RETURN_TYPE_SEPARTOR
and 
org/ovirt/engine/core/searchbackend/SearchObjects.java:63 //another definition of separator.

Comment: trying to solve sql injection is very improper, related code is extremely inferior and all of this should be thrown away as a whole.

Comment 1 Martin Mucha 2014-06-04 07:44:39 UTC

in
http://gerrit.ovirt.org/#/c/28343/
I've selected " : " to be the right suffix — it seemed to me, that this value is more important for the system, since ':' was only used in determining 'query safeness'. So I've changed ':' to " : ". If it should be the other case, please let me know.

Comment 2 Petr Beňas 2014-08-20 10:30:25 UTC

Is there any way how to test it? Increasing log level or something like that? I'm looking for a way how to confirm the change is in.

Comment 3 Martin Mucha 2014-08-28 13:53:02 UTC

I did a quick scan through code and I do not think this is testable in a black-box way. 
Internals seems too complex to me, so we cannot prove it's ok just upon changing logging level or some other externally observable change like this. Which is, on top of that, not present in code. I.e. I did not find such 'property', but even if it exist somewhere in code, based on it's complexity I do not think it's safe to pronounce it "ok" based just on such observation.
Bug was actually caused by two duplicate constants which weren't in sync, so fix was rather easy.

Comment 4 Eyal Edri 2015-02-17 17:13:52 UTC

rhev 3.5.0 was released. closing.