[jboss-user] [JBoss Seam] - Injection threat on EntityQuery order

[doballve]

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#4089154

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4089154