HQL injection trough orderBy

[CSIRTTrizna]

Observed vs. expected behavior

When using JPAQuery.orderBy with user provided input there is a possibility to inject HQL query, which is then executed in database.

Steps to reproduce

Full POC code is available in repository:
https://github.com/CSIRTTrizna/CVE-2024-49203/

1. Create JPAQuery object instance:

JPAQuery<Test> query = new JPAQuery<Test>(entityManager).from(test);

2. Create OrderSpecifier object instance:

PathBuilder<Test> pathBuilder = new PathBuilder<>(Test.class, "test");
OrderSpecifier order = new OrderSpecifier(Order.ASC, pathBuilder.get(orderBy));

Where orderBy variable is user provided input.

3. order and run the query

JPAQuery<Test> orderedQuery = query.orderBy(order);
orderedQuery.fetch();

Environment

Library versions used in proof of concept to reproduce the vulnerability:

querydsl-jpa: 5.1.0
querydsl-apt: 5.1.0
hibernate-core: 6.1.1.Final
jakarta.persistence-api: 3.1.0
postgresql: 42.7.4

Querydsl version: 5.1.0

Querydsl module: querydsl-jpa

Database: postgresql

JDK: 23

Additional details

Article detailing the vulnerability : https://www.csirt.sk/querydsl-java-library-vulnerability-permits-sql-hql-injection.html

[romanhuba]

This can be easily fixed by using one of the provided PathBuilderValidator's when a path is being built from user input.

For example:

PathBuilder<Test> pathBuilder = new PathBuilder<>(Test.class, "test", PathBuilderValidator.FIELDS);

The default validator returns the property without any validation, which makes it unsuitable for this use case.

From http://querydsl.com/static/querydsl/3.6.0/reference/html/ch03.html 3.1.3. Dynamic paths:

PathBuilderValidator.FIELDS will verify field existence, PathBuilderValidator.PROPERTIES validates Bean properties and JPAPathBuilderValidator validates using a JPA metamodel.

[CSIRTTrizna]

Hello,
Thank you for your comment.
You are right, that fixed the issue. However, I don't understand why the default setting is deliberately insecure. Is there any reason why the FIELDS validator is not enabled by default? In my opinion, it would likely be much more secure if the validator was enabled by default, and only disabled in specific edge cases when necessary.

[romanhuba]

Having the functionality enabled by default would certainly make it safer, but using other validators implies some computational overhead, which could be significant in cases where this construct is used extensively. I would guess that the authors of this library did not intend for the path builder to be used with unverified user input, and this should definitely be better documented. Having no validator could make sense when the developer is the one providing the property names.

[CSIRTTrizna]

Yes, I get your point. I guess the ideal solution would be to add a visible warning in the documentation stating that, by default, the PathBuilder is unsafe and allows any HQL/SQL code execution, and that it should be used with caution.

[CSIRTTrizna]

I am conflicted, whether to close this issue or leave it open so that it can serve as a warning for developers in the future.

[romanhuba]

A more useful outcome would be updating the documentation and Javadoc for the relevant components.

[CSIRTTrizna]

Perhaps yes. However, I still think it would make more sense to have the default setting check the columns. There shouldn't be much overhead, as the columns should be defined in the Entity class, making the lookup complexity something like O(1).

[koisurunippon]

Hi, is there a release plan for a new querydsl version that fixes this issue? Thanks.

[CSIRTTrizna]

Hi, from what I’ve observed, this repository is more or less inactive. I would recommend switching to the new fork of QueryDSL from OpenFeign (https://github.com/OpenFeign/querydsl). Although they face the same issue, the authors are actively communicating with me and working to resolve the problem as quickly as possible.

[koisurunippon]

Thanks CSIRTTrizna

[DengChen2020]

I think the problem mainly lies in the fact that you allow users to input, and you should validate their input. It's not a bug that orderBy might be an expression. It's appropriate not to validate the default case.

[CSIRTTrizna]

So, do you think it is appropriate for a framework that calls itself safe to have unsafe / insecure default settings in the common use of essential query functionality?

[DengChen2020]

Perhaps yes. However, I still think it would make more sense to have the default setting check the columns. There shouldn't be much overhead, as the columns should be defined in the Entity class, making the lookup complexity something like O(1).

The sorted columns may not necessarily be defined in the Entity class, such as functions like rand()

[DengChen2020]

I don't think the author ever thought that someone would leave SQL to user input without any validation, and I don't think that's a big problem. This project seems to no longer be maintained

[CSIRTTrizna]

Yes, this project is no longer active; however, there are still people using it, so it is worth warning them about this possibility. Additionally, as mentioned in this thread, there is a new fork that faces the same issue.

[CSIRTTrizna]

Perhaps yes. However, I still think it would make more sense to have the default setting check the columns. There shouldn't be much overhead, as the columns should be defined in the Entity class, making the lookup complexity something like O(1).

The sorted columns may not necessarily be defined in the Entity class, such as functions like rand()

Yes, we can name plenty of cases for column ordering that do not involve simple column names. However, as mentioned before, the default should be secure. If you want to use some obscure method of ordering your data you have to check the user input. It is really not that hard to understand.

[velo]

Hi @CSIRTTrizna,

Thank you for reporting this issue and for taking the time to bring it to my attention.

I have patched the issue in both the 6.x and 5.x releases of my fork.

For anyone concerned about this security issue, the fix is available in my fork. To migrate, simply change the group ID from com.querydsl to io.github.openfeign.querydsl.

The fork is fully backward-compatible, so the migration should be seamless.

https://github.com/OpenFeign/querydsl/releases/tag/6.10.1
https://github.com/OpenFeign/querydsl/releases/tag/5.6.1

[jwgmeligmeyling]

Closing as invalid.

Reason being: Querydsl is a query builder, it is inherently possible to create unsafe queries. It's not the responsibility of a query builder to sanitise user input, particularly input that is explicitly put in an expression (like path is) by the developer. Just as it's not the responsibility of the JDBC driver to sanitise the query string either. All it does is instead is provide a safe mechanism for avoiding SQL injection (in the form of Prepared Statements), for which Querydsl provides the same in the form of parameter/constant expressions.

Any attempt at fixing this CVE is giving a misleading sense of security as now developers will expect it is safe to put request parameters directly into expressions. For example: fixing PathBuilder doesn't address the exact same issue Expressions.path or TemplateExpression, which users may just as well use to pass user input into the query string. Request parameters should only be passed as constants (which are passed as query parameters so inherently not vulnerable to SQL injection) or be validated by the application.

It's also not trivial at all to sanitise user input against SQL injection, because these types of checks are database vendor specific. For example: some databases may accept different whitespace characters like tab or escape characters in their syntax. See for example: https://security.elarlang.eu/cve-2016-10007-and-cve-2016-10008-2-sql-injection-vulnerabilities-in-dotcms-blacklist-defence-bypass.html

[noren95]

Hi @jwgmeligmeyling
So since this is not a real issue, are there any plans to dispute this CVE? And regardless of it, do you consider it as a FP report?
Thank you!

[jwgmeligmeyling]

I think the CVE needs to be disputed (no idea how).

If we do want to fix the CVE properly, it would have to be done through making Path / PathBuilder / Template internal API's, and removing all methods that take an expression string as parameter so that all path expressions and template expressions are only created by the framework itself. (I.e. JPA's Criteria API only accepts Expression types that are returned from the JPA metamodel).

Alternatively we can explicitly rename any method/type that is unsafe to indicated it as such. I.e. PathBuilder -> PathBuilderUnsafe .

Either solution would be a really breaking to most Querydsl codebases and integrations, so I think the most logical solution is to dispute the CVE.

[CSIRTTrizna]

Yes, it is possible to dispute the CVE. However, you should also update the documentation and website, which currently states that "Querydsl is compact, safe and easy to learn." to "Querydsl is compact, willingly unsafe and easy to learn.". If the website had not claimed that it was safe, no CVE would have been reported.

Yes, there are plenty of frameworks that are unsafe but are upfront about it. However, claiming that the library is safe and then blaming the user or programmer when an security issue arises is rather scummy.

[jwgmeligmeyling]

Could be, but jOOQ doesn't advertise itself as unsafe either, and its safety guarantees regarding security are also bound to the generated code and DSL exclusively.

Their documentation mentions:

Important things to note about plain SQL!

There are some important things to keep in mind when using plain SQL:

• jOOQ doesn't know what you're doing. You're on your own again!
• You have to provide something that will be syntactically correct. If it's not, then jOOQ won't know. Only your JDBC driver or your RDBMS will detect the syntax error.
• You have to provide consistency when you use variable binding. The number of ? must match the number of variables
• Your SQL is inserted into jOOQ queries without further checks. Hence, jOOQ can't prevent SQL injection.

It does provide a build plugin to statically check if these unsafe API's are used (or are only used in specifically annotated places), which is something we could consider as well.