3:19 a.m.
[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=comments#action_12411089 ]
Felix Ho?feld commented on JBSEAM-2931:
---------------------------------------
we should do more to document the huge hole that was in Seam and
strongly urge people not to try and do this kind of thing.
I perfectly agree. In the opriginal issue I recommended myself this warrants an official
advisory and a backport of the patch to 1.2. Since most people think EJBQL is not SQL they
assume that they are safe from any SQL injection.
Note - unless you are trying to change the order parameter from a UI
binding, the santizing code should not be called and there should be no error regardless
of how you craft the order by clause.
The check against the regex is in the order-acessor so it is called every time regardless
whether the order clause was created by an EL expression or is hardcoded into the query
object. The example I already posted above will fail:
<framework:entity-query name="qry_allPersons" ejbql="SELECT p FROM
Person p" entity-manager="#{entityManager}"
order="UPPER(p.lastname)" />
Now in this case it would be trivial to fix this by rewriting this query as
<framework:entity-query name="qry_allPersons" ejbql="SELECT p FROM
Person p ORDER BY UPPER(p.lastname)" entity-manager="#{entityManager}"
/>
but I assume this does not work if I wish to add restrictions. Is this correct?
IMHO the best solutions would be to add an additional element to the entity-query element
called "order" with two attributes "by" and "valid". If this
alement is missing the behaviour is exactly as it is now. But if you feel lucky you can do
something like this:
<framework:entity-query name="qry_allPersons" ejbql="SELECT p FROM
Person p" entity-manager="#{entityManager}" />
<framework:order by="UPPER(#{TestAction.orderBy})"
valid="^UPPER(\w+(\.\w+)*)" />
</framework:entity-query>
Then Seam should for check if the by attribute contains an EL expression and if it does
but the valid attribute was not set it would fail with an exception. This would force
people to think about security but still allow people to make (more or less) informed
choices about what they wish to allow. However, it would still allow stupid people to do
stupid things like:
<framework:order
by="#{facesContext.externalContext.requestParameterMap['order']}"
valid=".*" />
I have to solve the issue anyway today so I will post what I will come up with.
Improve the exception thrown by an insecure order by clause
-----------------------------------------------------------
Key: JBSEAM-2931
URL: http://jira.jboss.com/jira/browse/JBSEAM-2931
Project: Seam
Issue Type: Task
Components: Framework
Affects Versions: 2.0.1.GA
Reporter: Felix Ho?feld
Assigned To: Norman Richards
Priority: Minor
Fix For: 2.1.0.BETA1, 2.0.2.CR2
We have an existing application running Seam 1.2. Today I tried upgrading to Seam
2.0.1.GA. In the process I discovered that the fix for JBSEAM-2099 breaks the application
because the application uses lots of query objects with an order clause that sorts on the
result of an function, namely UPPER(): order="UPPER(p.lastname)".
This used to work under 1.2. So this is a regression that probably does affect a lot of
real world applications. I have suggested the original fix and have to say it is not done
probably. Even my latest version is not the proper way to fix this as it will not allow
functions with multiple arguments, nor concatenations of properties, nor computing the
order by-value... To fix this properly it definitly takes an EJBQL-Expert greater than me
:-) I'm not even sure if there is an SQL-Injection threat here.
I don't mind implementing an insufficient fix for my special problem myself by
extending the Query object and binding that to a custom namespace but I would appreciate
if
a.) the regression would be properly documented, and
b.) the error message would tell the user what happened and what is necessary to fix it.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://jira.jboss.com/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira