[jboss-user] [JBoss Seam] - Injection threat on EntityQuery order
While using framework EntityQuery we came up with some extensions that might be worth
sharing.. the 1st is a security addition: the 'order' parameter gets directly
concatenaded to the the query.. that would allow anything to get injected in the query,
possibly resulting in a security threat - yes, it is not SQL, its HQL, but still, you do
not want people messing up with your query, do you?
What we do is to limit the values that can be passed as 'order', like this:
| /**
| * Protect against SQL injection in the order parameter... it gets concatenated to
SQL query!
| *
| * @param order String w/ 1 property path + 'ASC'/'DESC'
| * @throws IllegalArgumentException
| * if property path is not among ACCEPTABLE_ORDERBY_PARAMETERS
| */
| @Override
| public void setOrder(String order) {
| // validate parameter
| if (StringUtils.isNotBlank(order)) {
| String[] parts = order.trim().split("\\s", 2);
| parts[0] = StringUtils.trim(parts[0]);
| parts[1] = StringUtils.trim(parts[1]);
| boolean valid = true;
| List<String> acceptable = getAcceptableOrderByParameters();
| valid &= acceptable.contains(parts[0]);
| valid &= ("ASC".equalsIgnoreCase(parts[1]) ||
"DESC".equalsIgnoreCase(parts[1]));
| if (!valid) {
| throw new IllegalArgumentException("order: " + order);
| }
| } else {
| // blank = null
| order = null;
| }
|
| super.setOrder(order);
| }
|
| protected abstract List<String> getAcceptableOrderByParameters();
|
and then in the implementing class, defining something like:
| public final static List<String> ACCEPTABLE_ORDERBY_PARAMETERS =
Arrays.asList(new String[] {
| "meeting.id",
| "meeting.type",
| "meeting.status",
| "meeting.timestamp",
| });
|
| @Override
| protected List<String> getAcceptableOrderByParameters() {
| return ACCEPTABLE_ORDERBY_PARAMETERS;
| }
|
In our case we felt it was a good thing to require the abstract method to be implemented.
A less radical approach would be to provide a default method returning null and thus
accepting any 'order', but letting people restrict it if they want to be
safer..Consider it for future version of EntityQuery.
Diego
View the original post :
http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4089154#...
Reply to the post :
http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&a...