HQL injection vulnerabilities

[DJWalker42]

We have a number of 'GET' methods, returning a List, with RestQuery parameter string inputs that are not sanitised. These parameters are, generally speaking, passed to the 'getObjectIdentifiers()' by concatenating them into the query string with the 'like' operator. These input strings could be potentially used for HQL injection type attacks; not that I'm an expert and I don't know what the injection strings would look like in HQL, but its a potential vulnerability and is easily removed, or so the hibernate guides tell me.

The problem lies with the implementation of 'getObjectIdentifiers()'. We should be using named parameters (:<param_name>) in the QL string passed to 'createQuery()' then use 'setParameter()' to set their values, which mitigates the injection problem. Notice that the 'like' operator performs pattern matching, but the way we're using them they could be replaced with the '=' operator, and thus we could use named parameters.

To do this, we would need to pass the parameter name as used in the query string, and the value to which we want to set that parameter to the getObjectIdentifiers() function, for each parameter used in the string. A Java Map would do, with the key set to the parameter name, and the value to the parameter value.

--OR--

We just get rid of the getObjectIdentifiers() function, it is a convenience wrapper for the query itself, mainly to avoid writing out the same few lines per 'GET' call.

The Query could also be changed to a TypedQuery that would simplify the return statement to just 'query.getResultList()', I think, and lends itself to the argument of getting rid of 'getObjectIdentifiers()'.

[pahjbo]

A few points

• I don't think that it is that much as an injection attack possibility - the function is never called with completely unsanitised input from the api - it is always mediated by calls from other functions first that have naturally done some sanitisation - however, I agree that setting parameters on the query is better.
• the like comparisons on the person search is on purpose so that people can put in wild card searches if they want from the ui.
  *creating typed queries would be good, but because the return type is not one of the stored db types - i.e. only id and name that I do not think that it is possible
• rather than your map idea, it is possible to set parameters just by index value which would be simpler, then the signature would be something like
  protected List<ObjectIdentifier> getObjectIdentifiers(String queryStr, Object... params)

[alan-stokes]

Hi all. Just a tib bit for you. But get methods are generally only used for public accessible pages that the browser can cache easily. As everything in polaris should be behind a login screen, you really should replace those gets with posts. So that people can't use the caching process to bypass logins. At some point I'll send ya a copy if my Web security course which discusses this issue in detail. Regards Alan

…

On Thu, 21 Mar 2024, 13:19 Paul Harrison, ***@***.***> wrote: A few points - I don't think that it is that much as an injection attack possibility - the function is never called with completely unsanitised input from the api - it is always mediated by calls from other functions first that have naturally done some sanitisation - the like comparisons on the person search is on purpose so that people can put in wild card searches if they want from the ui. *creating typed queries would be good, but because the return type is not one of the stored db types - i.e. only id and name that I do not think that it is possible - rather than your map idea, it is possible to set parameters just by index value which would be simpler, then the signature would be something like protected List<ObjectIdentifier> getObjectIdentifiers(String queryStr, Object... params) — Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZMFSGWUMF6M2D2YOIBJNTYZLM5BAVCNFSM6AAAAABFBL275CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJSGI3TKMRTHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[DJWalker42]

TypedQuery is not possible here, it will throw a InstantiationException when trying to create an ObjectIdentifier.