Question: False positive in Path traversal - Java

[chmodxxx]

Hello, I'm facing a false positive in path traversal with the following code :

 public String doSomething(ArgType arg1, SinkType sink1) {
        if (!Util.isValidFilename(sink1.getFilename())) {
            throw new ServiceException();
        }
        Path testPath = Paths.get(sink1.getFilename());

isValidFilename definition :

    static boolean isValidFilename(String filename) {
        if (filename.contains("/") || filename.contains("\\")) {
            return false;
        }
        return true;
    }
}

basically the isValidFilename method doesn't cut the flow when it reaches Util.isValidFilename(sink1.getFilename()). Do you know how I can add some customizations to PathSanitizer so that it takes into consideration this case.
I have tried things like

class ContainsSanitizer extends PathInjectionSanitizer {
  ContainsSanitizer() {
     exists(MethodCall ma| 
      
      ma.getMethod().getName() = "contains" and ma = this.asExpr() and isDisallowedWord(ma.getArgument(0)) )

  }
}

this doesn't work as well.

Appreciate any pointers

[chmodxxx]

From my quick understanding looks like this predicate is not working here in this case

private predicate blockListGuard(Guard g, Expr e, boolean branch) {
  branch = false and
  // Local taint-flow is used here to handle cases where the validated expression comes from the
  // expression reaching the sink, but it's not the same one, e.g.:
  //  File file = source();
  //  String strPath = file.getCanonicalPath();
  //  if (!strPath.contains("..") && !strPath.startsWith("/dangerous/dir"))
  //    sink(file);
  g instanceof BlockListGuard and
  localTaintFlowToPathGuard(e, g) and
  exists(Expr previousGuard |
    localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g)
    or
    previousGuard
        .(PathTraversalGuard)
        .controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch())
  )
}

since it's not a localtaintflow, please correct me if i'm wrong

[jketema]

Hi @chmodxxx,

Thanks for your question. I've asked someone who is in the known aboyt this to weigh in on your question.

[atorralba]

Hi @chmodxxx.

There are many moving parts in your example that add complexity to this use case.

First of all, I'm making the assumption that SinkType is somehow a source, and that getFilename() propagates taint from the qualifier, so I added the following custom models in Customizations.qll to start working on this (note that they're by no means precise, but ought to be enough for this example):

import java
private import semmle.code.java.dataflow.FlowSources

class MySource extends RemoteFlowSource {
  MySource() { this.asParameter().getType().(RefType).hasName("SinkType") }

  override string getSourceType() { result = "test" }
}

class MyStep extends AdditionalTaintStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
    exists(MethodCall mc | mc.getMethod().hasName("getFilename") |
      n1.asExpr() = mc.getQualifier() and
      n2.asExpr() = mc
    )
  }
}

Now, there are things to consider in how we model path sanitizers in PathSanitizer.qll:

• We require at least two sanitization/validation mechanisms to be applied to a value to consider it safe against path injection attacks, which normally are path normalization + some kind of allow/disallow check. This is to not have FNs in case of incomplete validations like input.contains("..") (which doesn't protect against absolute paths).
• In your case, contains("/") || contains("\\") is an exception to this, because it protects both against path traversal and absolute paths. But we don't have a guard for this specific check by itself.
• Our ValidationMethod logic currently only supports validation methods that throw an exception, not that return true or false. So we aren't able to detect isValidFilename as a path injection guard out of the box.
• See how there's no actual flow from what's being validated (the result of the first call to getFilename()) to the sink (the second call to getFilename()). The only thing that connects those two pieces of data is the qualifier, which we don't track in our sanitizers.

Having said all that (which is a lot to justify why we don't cover this out of the box, sorry 😄), you can easily add your own sanitizer to support this case by extending PathInjectionSanitizer.

You want a BarrierGuard that checks either the argument of isValidFilename if it's a variable, or its qualifier if it's a MethodAccess (which BTW has been renamed to MethodCall recently). So you could write something like this:

private import semmle.code.java.controlflow.Guards
private import semmle.code.java.security.PathSanitizer

private predicate isValidFilenameGuard(Guard g, Expr e, boolean branch) {
  branch = true and
  exists(MethodCall mc | mc.getMethod().hasName("isValidFilename") |
    g = mc and
    e = [mc.getArgument(0), mc.getArgument(0).(MethodCall).getQualifier()]
  )
}

class IsValidFilenameSanitizer extends PathInjectionSanitizer {
  IsValidFilenameSanitizer() {
    this = DataFlow::BarrierGuard<isValidFilenameGuard/3>::getABarrierNode()
  }
}

Note that this would still not catch more complicated patterns like:

String filename = sink1.getFilename();
if (!Utils.isValidFilename(filename))
  throw new Exception();

Paths.get(sink1.getFilename());

In which case local flow (or even global flow if you complicate it even more) would be needed in the definition of e in isValidFilenameGuard.

Also, of course this solution relies on your knowledge that isValidFilename successfully validates the filename, instead of trying to analyze its contents to determine whether it does (which as I explained above, is quite tricky if you want to catch all edge cases). Depending on your use case, it might be easier to model the custom sanitizers/validators used in your codebase (particularly if they are defined in generally-used libraries) instead of trying to find them dynamically by analyzing what they do with CodeQL.

I hope this helps you! This is a lot to take in, so don't hesitate to ask if something isn't clear enough.

[chmodxxx]

Ok just to understand what I need here,

the first issue is that I need to propagate taint from my remoteSource Qualifier (e.g SourceType.getFilename()) , then I need to do a global dataflow to the filenameguard method or more easily to "contains" method.

Does that work ?

[atorralba]

The part about getFilename() was my assumption as to how the out-of-the-box path injection query could be finding results in this codebase. Not needed for the sanitization part.

If you don't follow my advice about writing a barrier guard around isValidFilename, and decide to go the route of trying to dynamically figure out that it's a validation method by tracking flow into contains, you'll run into several problems, as I explained in my comment above. If you do it, let us know how it goes! :)

[chmodxxx]

Thanks @atorralba