The 'order' parameter gets directly concatenaded to the query.. that would allow anything to get injected in the query, possibly resulting in a security threat. This patch gives the developer extending framework Query the chance to limit the acceptable order properties.
Description
From http://www.jboss.com/index.html?module=bb&op=viewtopic&t=119810
The 'order' parameter gets directly concatenaded to the query.. that would allow anything to get injected in the query, possibly resulting in a security threat. This patch gives the developer extending framework Query the chance to limit the acceptable order properties.
Matt Drees [13/Oct/07 01:41 PM]
It looks like the patch requires order clauses to contain either ASC or DESC; I think it'd be good to make the direction optional.
Diego Ballve [13/Oct/07 03:19 PM]
I did not realize it could be omitted. The check for parts[1] becomes:
valid &= ("".equals(parts[1]) || "ASC".equalsIgnoreCase(parts[1]) || "DESC".equalsIgnoreCase(parts[1]));
IMHO this will not do. I may want to have more complex order clauses containing more than one property or even properties of child objects like:
SELECT e FROM Employee e ORDER BY e.department.name, e.lastname, e.firstname
I think this will be a common use case. If the order by statement is not defined by a single property name the patch will not work. Besides it requires that the query object is extended, so for any users using an xml declaration (and I think this is the vast majority) it will be useless. Finally it breaks backward compatibility: It would simply blow up all existing applications that use the order by parameter even if it is hardcoded into the xml and not an el expression (and that are therefore not even threatened).
As an alternative I suggest running the order against a simple regex, e.g.
if (! order.matches("^[\\w\\s\\.,]$"))
throw new IllegalArgumentException("Invalid order by clause in hql statement: " + order);
I don't know HQL syntax well enough: In SQL this would still allow appending queries using UNION but AFAIK UNION is not supported by HQL or JPQL.
BTW I think this bug definitely warants a backport of the final patch and an "official" advisory.
Felix Ho?feld [14/Oct/07 07:07 AM]
IMHO this will not do. I may want to have more complex order clauses containing more than one property or even properties of child objects like:
SELECT e FROM Employee e ORDER BY e.department.name, e.lastname, e.firstname
I think this will be a common use case. If the order by statement is not defined by a single property name the patch will not work. Besides it requires that the query object is extended, so for any users using an xml declaration (and I think this is the vast majority) it will be useless. Finally it breaks backward compatibility: It would simply blow up all existing applications that use the order by parameter even if it is hardcoded into the xml and not an el expression (and that are therefore not even threatened).
As an alternative I suggest running the order against a simple regex, e.g.
if (! order.matches("^[\\w\\s\\.,]$"))
throw new IllegalArgumentException("Invalid order by clause in hql statement: " + order);
I don't know HQL syntax well enough: In SQL this would still allow appending queries using UNION but AFAIK UNION is not supported by HQL or JPQL.
BTW I think this bug definitely warants a backport of the final patch and an "official" advisory.
Felix, I agree this is getting more complex than ideal, but If you want more complex order you have more to change than just validating the order param. But lets go through the comments:
First, backward compatibility is not broken. If list of valid params is not set, any is still acceptable.
Second, I have not been using xml to set up queries, i needed more than what xml could offer.. but I agree It would be desirable to be able to set valid props from xml.
Third:
- more than one property: still simple, the check could verify that all props are in the valid list. And actually, I'm splitting the string on spaces, not commas.. 'e.lastname,e.firstname' would be seen as 1 prop.. if defined as valid, it would pass. ;)
- order by child object: we actually use that, the trick is the property must appear in the select.. the way we solved it, your query would become:
SELECT e, e.department.name as depName FROM Employee e ORDER BY e.department.name
Besides, you need to say e.department.name is a valid order parameter and you need getResultList() to process the resulting.. if it is List<Object[]> then return a new List containing item[0].. Not pretty but did the trick.
Not to loose focus, the root of the problem is not to allow anything coming from a mapped requestParam.order to make it to the HQL query, unchecked. If you can restrict what fields can be exposed to web user, even better.
Diego Ballve [14/Oct/07 08:31 AM]
Felix, I agree this is getting more complex than ideal, but If you want more complex order you have more to change than just validating the order param. But lets go through the comments:
First, backward compatibility is not broken. If list of valid params is not set, any is still acceptable.
Second, I have not been using xml to set up queries, i needed more than what xml could offer.. but I agree It would be desirable to be able to set valid props from xml.
Third:
- more than one property: still simple, the check could verify that all props are in the valid list. And actually, I'm splitting the string on spaces, not commas.. 'e.lastname,e.firstname' would be seen as 1 prop.. if defined as valid, it would pass. ;)
- order by child object: we actually use that, the trick is the property must appear in the select.. the way we solved it, your query would become:
SELECT e, e.department.name as depName FROM Employee e ORDER BY e.department.name
Besides, you need to say e.department.name is a valid order parameter and you need getResultList() to process the resulting.. if it is List<Object[]> then return a new List containing item[0].. Not pretty but did the trick.
Not to loose focus, the root of the problem is not to allow anything coming from a mapped requestParam.order to make it to the HQL query, unchecked. If you can restrict what fields can be exposed to web user, even better.
Thanks for catching this. After examining the issue, I implemented something very similar to Felix's suggestion to filter out sql injection hacks without imposing any other constraints on the parameter.
The use of the order attribute as a direct request parameter is inherently flawed. We should change seam-gen, and perhaps the Query class itself, to support a better nothing of sortable columns than passing text strings that get appended to the query. What we are doing now is bad, and we should NOT be encouraging people to do it like that.
Norman Richards [15/Oct/07 03:41 PM]
Thanks for catching this. After examining the issue, I implemented something very similar to Felix's suggestion to filter out sql injection hacks without imposing any other constraints on the parameter.
The use of the order attribute as a direct request parameter is inherently flawed. We should change seam-gen, and perhaps the Query class itself, to support a better nothing of sortable columns than passing text strings that get appended to the query. What we are doing now is bad, and we should NOT be encouraging people to do it like that.
Patch
Closed
Critical
Support protection against SQL injection in Query order parameter
