Skip to content

Commit 59d4547

Browse files
Jami CogswellJami Cogswell
Jami Cogswell
authored and
Jami Cogswell
committed
Java: add FileConstructorSanitizer and tests
1 parent 0447628 commit 59d4547

File tree

2 files changed

+186
-2
lines changed

2 files changed

+186
-2
lines changed

‎java/ql/lib/semmle/code/java/security/PathSanitizer.qll

+42-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ private import semmle.code.java.dataflow.FlowSources
66
private import semmle.code.java.dataflow.SSA
77
private import semmle.code.java.frameworks.kotlin.IO
88
private import semmle.code.java.frameworks.kotlin.Text
9+
private import semmle.code.java.dataflow.Nullness
910

1011
/** A sanitizer that protects against path injection vulnerabilities. */
1112
abstract class PathInjectionSanitizer extends DataFlow::Node { }
@@ -143,8 +144,7 @@ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
143144
// String strPath = path.toString();
144145
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
145146
// sink(path);
146-
branch = g.(PathTraversalGuard).getBranch() and
147-
localTaintFlowToPathGuard(e, g) and
147+
pathTraversalGuard(g, e, branch) and
148148
exists(Guard previousGuard |
149149
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
150150
or
@@ -352,3 +352,43 @@ private class FileGetNameSanitizer extends PathInjectionSanitizer {
352352
)
353353
}
354354
}
355+
356+
/** Holds if `expr` may be null. */
357+
private predicate maybeNull(Expr expr) {
358+
exists(DataFlow::Node src, DataFlow::Node sink |
359+
src.asExpr() = nullExpr() and
360+
sink.asExpr() = expr
361+
|
362+
DataFlow::localFlow(src, sink)
363+
)
364+
}
365+
366+
/** Holds if `g` is a guard that checks for `..` components. */
367+
private predicate pathTraversalGuard(Guard g, Expr e, boolean branch) {
368+
branch = g.(PathTraversalGuard).getBranch() and
369+
localTaintFlowToPathGuard(e, g)
370+
}
371+
372+
/**
373+
* A sanitizer that considers the second argument to a `File` constructor safe
374+
* if it is checked for `..` components (`PathTraversalGuard`) or if any internal
375+
* `..` components are removed from it (`PathNormalizeSanitizer`).
376+
*/
377+
private class FileConstructorSanitizer extends PathInjectionSanitizer {
378+
FileConstructorSanitizer() {
379+
exists(ConstructorCall constrCall, Argument arg |
380+
constrCall.getConstructedType() instanceof TypeFile and
381+
// Exclude cases where the parent argument is null since the
382+
// `java.io.File` documentation states that such cases are
383+
// treated as if invoking the single-argument `File` constructor.
384+
not maybeNull(constrCall.getArgument(0)) and
385+
arg = constrCall.getArgument(1) and
386+
(
387+
arg = DataFlow::BarrierGuard<pathTraversalGuard/3>::getABarrierNode().asExpr() or
388+
arg = ValidationMethod<pathTraversalGuard/3>::getAValidatedNode().asExpr() or
389+
TaintTracking::localExprTaint(any(PathNormalizeSanitizer p), arg)
390+
) and
391+
this.asExpr() = arg
392+
)
393+
}
394+
}

‎java/ql/test/library-tests/pathsanitizer/Test.java

+144
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
import java.nio.file.Path;
44
import java.nio.file.Paths;
55
import android.net.Uri;
6+
import java.io.BufferedReader;
7+
import java.io.InputStreamReader;
8+
import java.net.Socket;
69

710
public class Test {
811

@@ -463,4 +466,145 @@ public void blockListGuard() throws Exception {
463466
}
464467
}
465468
}
469+
470+
private void fileConstructorValidation(String path) throws Exception {
471+
// Use `indexOf` instead of `contains` for this test since a `contains`
472+
// call in this scenario will already be sanitized due to the inclusion
473+
// of `ValidatedVariableAccess` nodes in `defaultTaintSanitizer`.
474+
if (path.indexOf("..") != -1)
475+
throw new Exception();
476+
}
477+
478+
public void fileConstructorSanitizer() throws Exception {
479+
// PathTraversalGuard
480+
{
481+
String source = (String) source();
482+
File f1 = new File("safe/file.txt");
483+
if (!source.contains("..")) {
484+
File f2 = new File(f1, source);
485+
sink(f2); // Safe
486+
sink(source); // $ MISSING: hasTaintFlow
487+
} else {
488+
File f3 = new File(f1, source);
489+
sink(f3); // $ hasTaintFlow
490+
sink(source); // $ hasTaintFlow
491+
}
492+
}
493+
{
494+
String source = (String) source();
495+
File f1Tainted = (File) source();
496+
if (!source.contains("..")) {
497+
// `f2` is unsafe if `f1` is tainted
498+
File f2 = new File(f1Tainted, source);
499+
sink(f2); // $ hasTaintFlow
500+
sink(source); // $ MISSING: hasTaintFlow
501+
} else {
502+
File f3 = new File(f1Tainted, source);
503+
sink(f3); // $ hasTaintFlow
504+
sink(source); // $ hasTaintFlow
505+
}
506+
}
507+
{
508+
String source = (String) source();
509+
File f1Null = null;
510+
if (!source.contains("..")) {
511+
// `f2` is unsafe if `f1` is null
512+
File f2 = new File(f1Null, source);
513+
sink(f2); // $ hasTaintFlow
514+
sink(source); // $ hasTaintFlow
515+
} else {
516+
File f3 = new File(f1Null, source);
517+
sink(f3); // $ hasTaintFlow
518+
sink(source); // $ hasTaintFlow
519+
}
520+
}
521+
{
522+
String source = (String) source();
523+
File f1 = new File("safe/file.txt");
524+
if (source.indexOf("..") == -1) {
525+
File f2 = new File(f1, source);
526+
sink(f2); // Safe
527+
sink(source); // $ MISSING: hasTaintFlow
528+
} else {
529+
File f3 = new File(f1, source);
530+
sink(f3); // $ hasTaintFlow
531+
sink(source); // $ hasTaintFlow
532+
}
533+
}
534+
{
535+
String source = (String) source();
536+
File f1 = new File("safe/file.txt");
537+
if (source.indexOf("..") != -1) {
538+
File f2 = new File(f1, source);
539+
sink(f2); // $ hasTaintFlow
540+
sink(source); // $ hasTaintFlow
541+
} else {
542+
File f3 = new File(f1, source);
543+
sink(f3); // Safe
544+
sink(source); // $ MISSING: hasTaintFlow
545+
}
546+
}
547+
{
548+
String source = (String) source();
549+
File f1 = new File("safe/file.txt");
550+
if (source.lastIndexOf("..") == -1) {
551+
File f2 = new File(f1, source);
552+
sink(f2); // Safe
553+
sink(source); // $ MISSING: hasTaintFlow
554+
} else {
555+
File f3 = new File(f1, source);
556+
sink(f3); // $ hasTaintFlow
557+
sink(source); // $ hasTaintFlow
558+
}
559+
}
560+
// validation method
561+
{
562+
String source = (String) source();
563+
File f1 = new File("safe/file.txt");
564+
fileConstructorValidation(source);
565+
File f2 = new File(f1, source);
566+
sink(f2); // Safe
567+
sink(source); // $ MISSING: hasTaintFlow
568+
}
569+
{
570+
String source = (String) source();
571+
File f1 = new File("safe/file.txt");
572+
573+
if (source.contains("..")) {
574+
throw new Exception();
575+
} else {
576+
File f2 = new File(f1, source);
577+
sink(f2); // Safe
578+
sink(source); // $ MISSING: hasTaintFlow
579+
}
580+
}
581+
// PathNormalizeSanitizer
582+
{
583+
File source = (File) source();
584+
String normalized = source.getCanonicalPath();
585+
File f1 = new File("safe/file.txt");
586+
File f2 = new File(f1, normalized);
587+
sink(f2); // Safe
588+
sink(source); // $ hasTaintFlow
589+
sink(normalized); // $ MISSING: hasTaintFlow
590+
}
591+
{
592+
File source = (File) source();
593+
String normalized = source.getCanonicalFile().toString();
594+
File f1 = new File("safe/file.txt");
595+
File f2 = new File(f1, normalized);
596+
sink(f2); // Safe
597+
sink(source); // $ hasTaintFlow
598+
sink(normalized); // $ MISSING: hasTaintFlow
599+
}
600+
{
601+
String source = (String) source();
602+
String normalized = Paths.get(source).normalize().toString();
603+
File f1 = new File("safe/file.txt");
604+
File f2 = new File(f1, normalized);
605+
sink(f2); // Safe
606+
sink(source); // $ hasTaintFlow
607+
sink(normalized); // $ MISSING: hasTaintFlow
608+
}
609+
}
466610
}

0 commit comments

Comments
 (0)