[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=comments#action_12410934 ] Felix Ho?feld commented on JBSEAM-2931: --------------------------------------- Sorry, I could not reopen JBSEAM-2099 so I have cloned it. The fix for JBSEAM-2099 does not work as expected because the regular expression is too restrictive. When I suggested the regex for input filtering I did not think of funtion based order clauses which are quite common in the real world, e.g. <framework:entity-query name="qry_allPersons" ejbql="SELECT p FROM Person p" entity-manager="#{entityManager}" order="UPPER(p.lastname)"> </framework:entity-query> The query will raise an invalid argument exception "invalid order clause" because the order clause does not match the regular expression "^[\\w\\.,\\s]*$" defined in ORDER_CLAUSE_PATTERN). (http://fisheye.jboss.com/browse/Seam/trunk/src/main/org/jboss/seam/framew...). I suggest changing this to ^(\\w+\\([\\w\\.\\s]+\\)\\s*,\\s*|[\\w\\.]+\\s*,\\s*)*(\\w+\\([\\w\\.\\s]+\\)|[\\w\\.]+) which will alo match something like: UPPER(p.lastname), p.age, LOWER(p.country). -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=all ] Norman Richards closed JBSEAM-2931. ----------------------------------- Resolution: Rejected We aren't going to play regexp games with fields that we are supporting direct UI binding for. If you want a complex order clause that can be set from a UI binding, you'll need to subclass and accept responsibility for the SQL injection protection yourself. -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=all ] Felix Ho?feld reopened JBSEAM-2931: ----------------------------------- -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=comments#action_12410987 ] Felix Ho?feld commented on JBSEAM-2931: --------------------------------------- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=comments#action_12410996 ] Pete Muir commented on JBSEAM-2931: ----------------------------------- I tidied up the issue for you... -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=all ] Norman Richards updated JBSEAM-2931: ------------------------------------ Summary: Improve the exception thrown by an insecure order by clause (was: Document that the orderBy property of an EntityQuery is limited by a regex) -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=comments#action_12411089 ] Felix Ho?feld commented on JBSEAM-2931: --------------------------------------- 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. 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. -- 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

[ http://jira.jboss.com/jira/browse/JBSEAM-2931?page=all ] Pete Muir updated JBSEAM-2931: ------------------------------ Fix Version/s: 2.0.2.GA (was: 2.0.2.CR2) Assignee: Norman Richards (was: Pete Muir) I improved the exception message. Norman, if you want to improve the docs, we can do that before the GA. -- 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