Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Java] Clarification Needed on DataFlow::FeatureHasSourceCallContext Behavior#21097

Answeredbyaschackmull
Hug0Vincent asked this question inQ&A
Discussion options

Hi,
I'm encountering unexpected behavior that I thought would be resolved by usingDataFlow::FeatureHasSourceCallContext, but it persists. I'd appreciate some clarification on how the three different Feature settings interact.

To illustrate, I've reproduced the issue in Java: I'm tracking ataint parameter from mysource method. However, when the taint flows intorandomMethod, it incorrectly appears to continue after the return statement inotherMethod even though source never callsotherMethod. Why is this path being shown as possible?

publicvoidsource(Stringtaint) {returnrandomMethod(taint)}publicObjectrandomMethod(Stringtaint) {returntaint}publicvoidotherMethod(Stringparam) {Objectobj =randomMethod(taint)// continue the tracking / flow}

I initially thought thatDataFlow::FeatureHasSourceCallContext would solve this issue so I'm confused. There is theDataFlow::FeatureEqualSourceSinkCallContext value but I don't understand it's purpose, could you provide a simple code snippet to explain bothFeatureHasSinkCallContext andFeatureEqualSourceSinkCallContext ?

I feel I'm not the only one struggling with this issue:#17241 I'm surprised by this kind of result.

I would like a way to ensure that my path goes from source to sink and that it can be called in my program.

Thank you

You must be logged in to vote

That looks completely correct to me. If flow reaches a static variable then surely that same static variable can be read from other unrelated calls totoUpperEnglish - storing into and reading from a static variable effectively throws away the call context. So even though this may be considered a false positive, it's working as intended. Data flow cannot distinguish between individual array indices here, so as soon as one entry in the cache is tainted, then they're all considered tainted.

Replies: 2 comments 11 replies

Comment options

Hi 👋

Can you share the query that you used to track flow? I'm surprised that you see flow going fromsource intorandomMethod and then out tootherMethod; that certainly shouldn't happen.

As for flow features, they are used to restrict which kind of flow is allowed at sources/sinks:

  • FeatureHasSourceCallContext: This means that a source is not allowed to flowout of the method containing the source.
  • FeatureHasSinkCallContext: This means that a sink is not allowed to have flowin to the method containing the sink.
  • FeatureEqualSourceSinkCallContext: This means, basically, that the source and sink must be in the same method.

Flow features are rarely needed in practice, and if the underlying problem is the erroneous flow mentioned above, then it sounds more like a bug in the data flow library.

You must be logged in to vote
1 reply
@Hug0Vincent
Comment options

Hi, yes so here it is:

When isFeatureHasSinkCallContext useful ?

Comment options

@hvitved I have extracted the relevant code from my codebase and created a query to show you.

So here is my Java code, I have basically copy/paste the weird function which istoUpperEnglish. I want to find flow from parameter ofsourceMethod to arguments ofsinkMethod. From my understanding their should be only one path in my example. I've omittedFeatureHasSourceCallContext here because it does not change anything but I'm using it to prevent flow to get out of my source method.

packagecom.test;importjava.util.Locale;publicclassStringUtils {privatestaticfinalintTO_UPPER_CACHE_LENGTH =2048;privatestaticfinalintTO_UPPER_CACHE_MAX_ENTRY_LENGTH =64;privatestaticfinalString[][]TO_UPPER_CACHE =newString[TO_UPPER_CACHE_LENGTH][];privateStringUtils() {   }publicstaticStringtoUpperEnglish(Stringvar0) {if (var0 ==null)returnnull;if (var0.length() >TO_UPPER_CACHE_MAX_ENTRY_LENGTH) {returnvar0.toUpperCase(Locale.ENGLISH);      }else {intvar1 =var0.hashCode() & (TO_UPPER_CACHE_LENGTH -1);String[]var2 =TO_UPPER_CACHE[var1];if (var2 !=null &&var2[0].equals(var0)) {returnvar2[1];         }else {Stringvar3 =var0.toUpperCase(Locale.ENGLISH);var2 =newString[]{var0,var3};TO_UPPER_CACHE[var1] =var2;returnvar3;         }      }   }}classSourceclass {publicObjectsourceMethod(Objectsource) {return (String)Sinkclass.sinkMethod(StringUtils.toUpperEnglish((String)source));   }}classOther {privatestaticStringrandomMethod(Objectvar1) {return (String)Sinkclass.sinkMethod(StringUtils.toUpperEnglish((String)var1));   }}classSinkclass {publicstaticObjectsinkMethod(Objectsource) {returnsource;   }}

Note therandomMethod that is also callingtoUpperEnglish butsourceMethod never call it.

Here is my CodeQL query:

/** * @id test * @description test * @name test * @kind path-problem * @problem.severity warning * @tags security */import javaimport semmle.code.java.dataflow.FlowSourcesimport semmle.code.java.security.CommandLineQueryclassSourceMethodextendsMethod{SourceMethod(){this.getName()="sourceMethod"}}module MyConfigimplements DataFlow::ConfigSig{predicateisSource(DataFlow::Nodesource){source.asParameter().getCallable()instanceofSourceMethod}predicateisSink(DataFlow::Nodesink){exists(Callc|c.getCallee().hasQualifiedName("com.test","Sinkclass","sinkMethod")andsink.asExpr()=c.getAnArgument())}}module MyFlow= TaintTracking::Global<MyConfig>;import MyFlow::PathGraphfrom MyFlow::PathNodesource, MyFlow::PathNodesinkwhere MyFlow::flowPath(source,sink)selectsink.getNode(),source,sink,"sink: $@",source.getNode(),sink.toStringWithContext()

For this I get 2 paths, the one I'm looking for:

image

but also this one:

image
You must be logged in to vote
10 replies
@aschackmull
Comment options

That looks completely correct to me. If flow reaches a static variable then surely that same static variable can be read from other unrelated calls totoUpperEnglish - storing into and reading from a static variable effectively throws away the call context. So even though this may be considered a false positive, it's working as intended. Data flow cannot distinguish between individual array indices here, so as soon as one entry in the cache is tainted, then they're all considered tainted.

Answer selected byHug0Vincent
@Hug0Vincent
Comment options

Alright so Is there a way to keep the call context from source to sink to prevent those "false positive" ?

@aschackmull
Comment options

If we're analysing the code as-is then I'd argue that we do in fact want this path: The call fromsourceMethod taints the cache, which ought to be reflected as taint reaching other call sites.
An alternative approach is to supply a model giving a high-level description of the taint flow intoUpperEnglish, which can then supersede the source code analysis. You'd want ayml file with something like

extensions:  - addsTo:      pack: codeql/java-all      extensible: summaryModel    data:      - ["com.test", "StringUtils", True, "toUpperEnglish", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
@Hug0Vincent
Comment options

The problem is that I'm running it on multiple codebase and I don't know in advance which function will need such summaryModel

@aschackmull
Comment options

I guess alternatively you can introduce a barrier for theFieldValueNode. That should block the call-context-dropping flow path.

  predicate isBarrier(DataFlow::Node node) {    node.(DataFlow::FieldValueNode).getField() instanceof MyStaticCacheFieldThatShouldntPropagateTaint  }
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Category
Q&A
Labels
None yet
3 participants
@Hug0Vincent@hvitved@aschackmull
[8]ページ先頭
©2009-2026 Movatter.jp