Tainted data stucks at isNull() method

[testanull]

Hi there,
I 'm writing QL for java and got some problem
In my QL, tainted data is “pollerRequestString”, it keeps stucking at some method like “Validator.isXxx”
Other methods after that method is not being tracked anymore

in above example, it stucks at “Validator.isNotNull”, method “Validator.isNull” and parsePollerRequestParameters after that is not tracked
have anyone got this before?
any solutions for this?
Thanks!

[rvermeulen]

Hi @testanull,

You are correct that the taint is not propagated beyond the Validator.isNotNull and parsePollerRequestParameters. You can verify this using the PartialPathGraph module.

How to use the module and how to ensure taint is propagated using additional taint steps is discussed in the latest Security Lab CTF Code & Chill starting from step 1.4.

If it remains unclear how to proceed don't hesitate to ask.

Cheers,
Remco

[testanull]

Yeah I know that, I have used PartialPathGraph to find out it stopped at Validator.isNull()
And AdditionalTaintStep doesn't help because it 's got filtered by Barrier

[aibaars]

I think pollerRequestString is indeed not tainted after the validator checks. If the validator tests do what their names suggest, the code after the validator tests look dead to me. The first validator tests that the value is not null and returns. Therefore, the variable must be null in the remainder of the method. I think we treat null values as untainted.

@aschackmull Could you confirm?

[aschackmull]

It appears that the flow is stopped by TaintTracking::defaultTaintBarrier. The fact that indeed the subsequent code appears dead may just be a coincidence, but it is hard to know for sure without knowing the exact contents of the validation methods.

[testanull]

Here is the Validator.isNull() method: https://github.com/liferay/liferay-portal/blob/5f6981fe8d818e5348d4a8688210fc17022986b3/portal-kernel/src/com/liferay/portal/kernel/util/Validator.java#L922

[aibaars]

This looks relevant too https://github.com/github/codeql/blob/master/java/ql/src/semmle/code/java/dataflow/NullGuards.qll#L124

[testanull]

I saw that but its qualified name is not java.util.Objects

[rvermeulen]

If it is stopped by the TaintTracking::defaultTaintBarrier then this query should identify why.
https://github.com/github/codeql/blob/083b8ef8e532968bbcd400d81b700230642403bf/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.ql

[aschackmull]

Alright, this is what happens: The null check is not just a null check when the argument is a string - it also checks for whitespace and the literal string "null". The checks on the string use charAt here https://github.com/liferay/liferay-portal/blob/5f6981fe8d818e5348d4a8688210fc17022986b3/portal-kernel/src/com/liferay/portal/kernel/util/Validator.java#L954 and the heuristic in defaultTaintBarrier therefore picks this up as being string validation call. Admittedly, this part of the defaultTaintBarrier probably ought to be moved into the various queries that want this and out of the default taint tracking configuration to avoid confusing cases like this one.

[aschackmull]

I have raised github/codeql#3590 to track the relevant library changes. In the meantime you can use a DataFlow::Configuration instead of a TaintTracking::Configuration if you want to avoid the barrier. Then you just need to add the default taint steps using

override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
  defaultAdditionalTaintStep(node1, node2)
}