Possible HQL injection trough orderBy in QueryDSL

[clementdenis]

CVE-2024-49203 was reported on QueryDSL, involving HQL injection trough a user-specified orderBy clause: querydsl/querydsl#3757

I was wondering if Spring Data JPA was vulnerable through its QueryDSL support.

From what I see in the code, OrderSpecifiers are always built using toOrderSpecifier, which calls buildOrderPropertyPathFrom, that seem to validate that a property exists in the entity being queried.

spring-data-jpa/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java

Lines 177 to 188 in ea0bd8c

	/**
	* Transforms a plain {@link Order} into a QueryDsl specific {@link OrderSpecifier}.
	*
	* @param order must not be {@literal null}.
	*/
	@SuppressWarnings({ "rawtypes", "unchecked" })
	private OrderSpecifier<?> toOrderSpecifier(Order order) {
	return new OrderSpecifier(
	order.isAscending() ? com.querydsl.core.types.Order.ASC : com.querydsl.core.types.Order.DESC,
	buildOrderPropertyPathFrom(order), toQueryDslNullHandling(order.getNullHandling()));
	}

A validation of my analysis from the maintainers of this project would be great :-)

[mp911de]

Generally, asking for exploitability in a publicly visible ticket puts us, our users, and folks looking for zero-day vulnerabilities in a problematic situation. Even worse that there is no fix available for the Querydsl CVE. Not reacting timely to a ticket can be seen as a sign of vulnerability since there's no insight into what happens behind the scenes. A public ticket talking about attack vectors easily leads to concerns on the user side and gives potential attackers some guidance on how things could be exploited.

In case of an existing vulnerability, confirming it publicly would prevent us from analyzing the issue, developing and testing a fix propertly, and finally shipping a release. As you might have noticed, discussing security concerns publicly isn't a great approach unless we can guarantee no vulnerability.

If you have questions or concerns in the future, I'm asking you to follow our responsible disclosure process outlined in our Security Policy.

That being said, your analysis only covers a part of applying sorting to Querydsl queries.
Querydsl generates String-based JPQL queries and so each expression can contribute to inclusion of undesired JPQL fragments. I do not see how the mentioned CVE can be fixed because that is the way how Querydsl for JPA is designed and implemented. Instead, that report leads to fear instead of contributing to more security (otherwise this ticket wouldn't exist, right?). For me, that report is about attention-seeking.

For Querydsl sorting, we utilize two approaches: QSort and Sort.

QSort is based on Querydsl OrderSpecifier. If the underlying expression in OrderSpecifier is contributed from an untrusted source, it can evaluate to contain undesired JPQL text. That is what PathBuilder demonstrates. Clearly, String concatenation is prone to exploitation and therefore, no expression should be built from unsanitized input.

The Sort approach uses domain class metadata to resolve the given sort expression, specifically Order into a property path. If the domain type doesn't define a path segement then path resolution fails. On the other side, our JpaSort.unsafe(…) cannot be used with Querydsl queries as any functions or non-path expressions would lead to PropertyReferenceException.

[clementdenis]

Sorry about this, I'll be more careful next time!

[OrangeDog]

I do not see how the mentioned CVE can be fixed

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

[velo]

Hello,

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

[OrangeDog]

@velo see also #3335

[velo]

@velo see also #3335

Well, if you guys want to do it as part of 4.x release... There is a 7.x branch in the works to be jpa 3.2 ready. Can start publishing snapshots if that helps somehow

[velo]

@OrangeDog the discussion seems to quote plenty of comments that have been deleted....

Wondering why