Skip to content

Commit 800a78d

Browse files
author
Max Schaefer
committed
Treat unexploitable types more centrally.
The apparently missing test result is due to sampling.
1 parent 8614d7b commit 800a78d

8 files changed

Lines changed: 33 additions & 40 deletions

‎java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -239,13 +239,7 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
239239
// Sanitizers are currently not modeled in MaD. TODO: check if this has large negative impact.
240240
predicate isSanitizer(Endpoint e, EndpointType t) {
241241
exists(t) and
242-
(
243-
e.asNode().getType() instanceof BoxedType
244-
or
245-
e.asNode().getType() instanceof PrimitiveType
246-
or
247-
e.asNode().getType() instanceof NumberType
248-
)
242+
AutomodelJavaUtil::isUnexploitableType(e.asNode().getType())
249243
or
250244
t instanceof AutomodelEndpointTypes::PathInjectionSinkType and
251245
e.asNode() instanceof PathSanitizer::PathInjectionSanitizer
@@ -377,57 +371,39 @@ class ApplicationModeMetadataExtractor extends string {
377371
*/
378372

379373
/**
380-
* A negative characteristic that indicates that parameters of an is-style boolean method should not be considered sinks,
381-
* and its return value should not be considered a source.
374+
* A negative characteristic that indicates that parameters of an is-style boolean method should not be considered sinks.
382375
*
383376
* A sink is highly unlikely to be exploitable if its callable's name starts with `is` and the callable has a boolean return
384377
* type (e.g. `isDirectory`). These kinds of calls normally do only checks, and appear before the proper call that does
385378
* the dangerous/interesting thing, so we want the latter to be modeled as the sink.
386379
*
387380
* TODO: this might filter too much, it's possible that methods with more than one parameter contain interesting sinks
388381
*/
389-
private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic
390-
{
382+
private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
391383
UnexploitableIsCharacteristic() { this = "unexploitable (is-style boolean method)" }
392384

393385
override predicate appliesToEndpoint(Endpoint e) {
394386
e.getCallable().getName().matches("is%") and
395387
e.getCallable().getReturnType() instanceof BooleanType and
396-
(
397-
e.getExtensibleType() = "sinkModel" and
398-
not ApplicationCandidatesImpl::isSink(e, _, _)
399-
or
400-
e.getExtensibleType() = "sourceModel" and
401-
not ApplicationCandidatesImpl::isSource(e, _, _) and
402-
e.getMaDOutput() = "ReturnValue"
403-
)
388+
not ApplicationCandidatesImpl::isSink(e, _, _)
404389
}
405390
}
406391

407392
/**
408393
* A negative characteristic that indicates that parameters of an existence-checking boolean method should not be
409-
* considered sinks, and its return value should not be considered a source.
394+
* considered sinks.
410395
*
411396
* A sink is highly unlikely to be exploitable if its callable's name is `exists` or `notExists` and the callable has a
412397
* boolean return type. These kinds of calls normally do only checks, and appear before the proper call that does the
413398
* dangerous/interesting thing, so we want the latter to be modeled as the sink.
414399
*/
415-
private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic
416-
{
400+
private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
417401
UnexploitableExistsCharacteristic() { this = "unexploitable (existence-checking boolean method)" }
418402

419403
override predicate appliesToEndpoint(Endpoint e) {
420-
exists(Callable callable |
421-
callable = e.getCallable() and
404+
exists(Callable callable | callable = e.getCallable() |
422405
callable.getName().toLowerCase() = ["exists", "notexists"] and
423406
callable.getReturnType() instanceof BooleanType
424-
|
425-
e.getExtensibleType() = "sinkModel" and
426-
not ApplicationCandidatesImpl::isSink(e, _, _)
427-
or
428-
e.getExtensibleType() = "sourceModel" and
429-
not ApplicationCandidatesImpl::isSource(e, _, _) and
430-
e.getMaDOutput() = "ReturnValue"
431407
)
432408
}
433409
}

‎java/ql/automodel/src/AutomodelFrameworkModeCharacteristics.qll‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@ newtype JavaRelatedLocationType =
2525
newtype TFrameworkModeEndpoint =
2626
TExplicitParameter(Parameter p) {
2727
AutomodelJavaUtil::isFromSource(p) and
28-
not p.getType() instanceof PrimitiveType and
29-
not p.getType() instanceof BoxedType and
30-
not p.getType() instanceof NumberType
28+
not AutomodelJavaUtil::isUnexploitableType(p.getType())
3129
} or
3230
TQualifier(Callable c) { AutomodelJavaUtil::isFromSource(c) and not c instanceof Constructor } or
3331
TReturnValue(Callable c) {
@@ -36,10 +34,7 @@ newtype TFrameworkModeEndpoint =
3634
or
3735
AutomodelJavaUtil::isFromSource(c) and
3836
c instanceof Method and
39-
(
40-
not c.getReturnType() instanceof VoidType and
41-
not c.getReturnType() instanceof PrimitiveType
42-
)
37+
not AutomodelJavaUtil::isUnexploitableType(c.getReturnType())
4338
} or
4439
TOverridableParameter(Method m, Parameter p) {
4540
AutomodelJavaUtil::isFromSource(p) and

‎java/ql/automodel/src/AutomodelJavaUtil.qll‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,3 +100,14 @@ predicate isFromSource(Element e) {
100100
// does not have a dummy location
101101
not e.hasLocationInfo(_, 0, 0, 0, 0)
102102
}
103+
104+
/**
105+
* Holds if taint cannot flow through the given type (because it is a numeric
106+
* type or some other type with a fixed set of values).
107+
*/
108+
predicate isUnexploitableType(Type tp) {
109+
tp instanceof PrimitiveType or
110+
tp instanceof BoxedType or
111+
tp instanceof NumberType or
112+
tp instanceof VoidType
113+
}

‎java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,5 @@
1313
| Test.java:68:30:68:47 | writer | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:68:30:68:47 | writer | CallContext | Test.java:68:30:68:47 | writer | MethodDoc | Test.java:68:30:68:47 | writer | ClassDoc | file://java.lang:1:1:1:1 | java.lang | package | file://Throwable:1:1:1:1 | Throwable | type | file://true:1:1:1:1 | true | subtypes | file://printStackTrace:1:1:1:1 | printStackTrace | name | file://(PrintWriter):1:1:1:1 | (PrintWriter) | signature | file://:1:1:1:1 | | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
1414
| Test.java:86:3:88:3 | list(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:86:3:88:3 | list(...) | CallContext | Test.java:86:3:88:3 | list(...) | MethodDoc | Test.java:86:3:88:3 | list(...) | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://list:1:1:1:1 | list | name | file://(Path):1:1:1:1 | (Path) | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
1515
| Test.java:87:4:87:29 | createDirectories(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:87:4:87:29 | createDirectories(...) | CallContext | Test.java:87:4:87:29 | createDirectories(...) | MethodDoc | Test.java:87:4:87:29 | createDirectories(...) | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://createDirectories:1:1:1:1 | createDirectories | name | file://(Path,FileAttribute[]):1:1:1:1 | (Path,FileAttribute[]) | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
16+
| Test.java:91:4:91:4 | p | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:90:3:92:3 | delete(...) | CallContext | Test.java:91:4:91:4 | p | MethodDoc | Test.java:91:4:91:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://delete:1:1:1:1 | delete | name | file://(Path):1:1:1:1 | (Path) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://ai-manual:1:1:1:1 | ai-manual | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
17+
| Test.java:95:4:95:4 | p | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:94:3:96:3 | deleteIfExists(...) | CallContext | Test.java:95:4:95:4 | p | MethodDoc | Test.java:95:4:95:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://deleteIfExists:1:1:1:1 | deleteIfExists | name | file://(Path):1:1:1:1 | (Path) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://ai-manual:1:1:1:1 | ai-manual | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |

‎java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractNegativeExamples.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
| Test.java:48:10:50:3 | compareTo(...) | known sanitizer\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:48:10:50:3 | compareTo(...) | MethodDoc | Test.java:48:10:50:3 | compareTo(...) | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
21
| Test.java:49:4:49:5 | f2 | known non-sink\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:49:4:49:5 | f2 | MethodDoc | Test.java:49:4:49:5 | f2 | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
32
| Test.java:55:4:55:4 | p | taint step\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:54:3:59:3 | walk(...) | CallContext | Test.java:55:4:55:4 | p | MethodDoc | Test.java:55:4:55:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
3+
| Test.java:94:3:96:3 | deleteIfExists(...) | known sanitizer\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:94:3:96:3 | deleteIfExists(...) | CallContext | Test.java:94:3:96:3 | deleteIfExists(...) | MethodDoc | Test.java:94:3:96:3 | deleteIfExists(...) | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://deleteIfExists:1:1:1:1 | deleteIfExists | name | file://(Path):1:1:1:1 | (Path) | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |

‎java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractPositiveExamples.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,5 @@
33
| Test.java:37:4:37:11 | openPath | path-injection\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:36:10:38:3 | newInputStream(...) | CallContext | Test.java:37:4:37:11 | openPath | MethodDoc | Test.java:37:4:37:11 | openPath | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://newInputStream:1:1:1:1 | newInputStream | name | file://(Path,OpenOption[]):1:1:1:1 | (Path,OpenOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
44
| Test.java:63:3:63:20 | getInputStream(...) | remote\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:63:3:63:20 | getInputStream(...) | CallContext | Test.java:63:3:63:20 | getInputStream(...) | MethodDoc | Test.java:63:3:63:20 | getInputStream(...) | ClassDoc | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
55
| Test.java:87:28:87:28 | p | path-injection\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:87:4:87:29 | createDirectories(...) | CallContext | Test.java:87:28:87:28 | p | MethodDoc | Test.java:87:28:87:28 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://createDirectories:1:1:1:1 | createDirectories | name | file://(Path,FileAttribute[]):1:1:1:1 | (Path,FileAttribute[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
6+
| Test.java:91:4:91:4 | p | path-injection\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:90:3:92:3 | delete(...) | CallContext | Test.java:91:4:91:4 | p | MethodDoc | Test.java:91:4:91:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://delete:1:1:1:1 | delete | name | file://(Path):1:1:1:1 | (Path) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
7+
| Test.java:95:4:95:4 | p | path-injection\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:94:3:96:3 | deleteIfExists(...) | CallContext | Test.java:95:4:95:4 | p | MethodDoc | Test.java:95:4:95:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://deleteIfExists:1:1:1:1 | deleteIfExists | name | file://(Path):1:1:1:1 | (Path) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |

‎java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,5 +86,13 @@ public static void FilesListExample(Path p) throws Exception {
8686
Files.list( // the call is a source candidate
8787
Files.createDirectories(p) // the call is a source candidate, but not a sink candidate (modeled as a taint step)
8888
);
89+
90+
Files.delete( // not a source candidate (return type is void)
91+
p // sink candidate
92+
);
93+
94+
Files.deleteIfExists( // not a source candidate (return type is boolean)
95+
p // sink candidate
96+
);
8997
}
9098
}

‎java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractNegativeExamples.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
| com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 | | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://:1:1:1:1 | | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
21
| com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 | | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
32
| com/github/codeql/test/PublicClass.java:26:28:26:39 | input | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | MethodDoc | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 | | output | file://input:1:1:1:1 | input | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
43
| java/io/File.java:4:16:4:24 | compareTo | known non-sink\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 | | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |

0 commit comments

Comments
 (0)