Skip to content

/ securitylab

Tainted data stucks at isNull() method #104

Unanswered
@testanull testanull
104d ago
· 10 replies
Unanswered
Tainted data stucks at isNull() method #104
@testanull testanull
104d ago
· 10 replies
Copy link
104d

@testanull 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
image
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!

Replies

edited
102d

@rvermeulen 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
102d

testanull
Author

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
102d

@aibaars aibaars
Collaborator

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?
101d

@aschackmull aschackmull
Collaborator

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
101d

testanull
Author

I saw that but its qualified name is not java.util.Objects
@aschackmull
101d

aschackmull
Collaborator

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.
101d

@aschackmull aschackmull
Collaborator

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)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
@testanull @rvermeulen @aibaars @aschackmull
Beta
You can’t perform that action at this time.