github / github/codeql

Java: CodeQL and chill missing result

Abierto
#17,045 2 comentarios 0 reacciones 0 asignados Ver en GitHub
Java question
Lenguaje dominante
CodeQL
Estrellas
10.1k
Forks
2.1k
Merge medio
2 d 15 h
PR fusionados (30 d)
141

Descripción

**Description of the issue**
I'm practicing securitylab's Codeql-and-chill, https://securitylab.github.com/ctf/codeql-and-chill/
and I found four data flows using the following codeql rules, with one missing,However, in my test demo, the missing data stream can be completely followed up. I don't know what causes this. The following is the specific codeql rules and test demo

Code database download link https://github.com/github/securitylab/releases/download/ctf-codeql-and-chill-java-edition/titus-control-plane-db.zip

Here's what my codeql rules say
```ql
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.FlowSources
import DataFlow::PartialPathGraph

class BuildConstraintViolationWithTemplateMethod extends Method{
BuildConstraintViolationWithTemplateMethod(){
hasQualifiedName("javax.validation", ["ConstraintValidatorContext"], "buildConstraintViolationWithTemplate")
}
}

class BuildConstraintViolationWithTemplateSink extends DataFlow::ExprNode{
BuildConstraintViolationWithTemplateSink(){
exists( MethodAccess ma | ma.getCallee() instanceof BuildConstraintViolationWithTemplateMethod | ma.getArgument(0)=asExpr())
}
}

class CollectionType extends RefType{
CollectionType(){
hasQualifiedName("java.util", "Collection")
}
}

class BeanValidationConfig extends TaintTracking::Configuration {
BeanValidationConfig() { this = "BeanValidationConfig" }

override predicate isSource(DataFlow::Node source) {
source instanceof RemoteFlowSource
}

override predicate isSink(DataFlow::Node sink) {
sink instanceof BuildConstraintViolationWithTemplateSink
}

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
pred.asExpr() = succ.asExpr().(AddExpr).getAnOperand()
or
exists(MethodAccess ac | pred.asExpr() = ac.getAnArgument() and succ.asExpr() = ac)
}

override predicate isSanitizer(DataFlow::Node n) {
exists(Method m | m.getParameter(0).getType() instanceof CollectionType|n.asParameter()=m.getParameter(0))
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
super.allowImplicitRead(node, c)
or
this.isAdditionalTaintStep(node, _) and
(
c instanceof DataFlow::ArrayContent
or
c instanceof DataFlow::CollectionContent
or
c instanceof DataFlow::MapValueContent
)
}
}

from BeanValidationConfig bvc, DataFlow::PathNode source, DataFlow::PathNode sink
where bvc.hasFlowPath(source, sink)
select source.getNode(), "-->", sink
```
The missing code location is in the `/Users/xavier/src/github/titus-control-plane/titus-common/src/main/java/com/netflix/titus/common/model/sanitizer/internal/CollectionValidator.java` file

Debugging through `hasPartialFlow` shows that the last position is in `filter(e -> e.getValue() == null)`
```java
if (!constraintAnnotation.allowNullValues()) {
Set badEntryKeys = value.entrySet().stream()
.filter(e -> e.getValue() == null)
.map(e -> e.getKey() instanceof String ? (String) e.getKey() : "")
.collect(Collectors.toSet());
if (!badEntryKeys.isEmpty()) {
attachMessage(context, "null values found for keys: " + new TreeSet<>(badEntryKeys));
return false;
}
}
```

When I tested it with the following code, I found that this rule follows through into the final system.out.println() function
MapToSetTest.java
```java
package com.example.demo;

import java.util.*;
import java.util.stream.Collectors;

public class MapToSetTest {
public static void main(String[] args) {
Map value = new HashMap<>();
value.put("key1", null);
value.put(2, null);
value.put("key3", "value3");
value.put(4, "value4");

Set badEntryKeys = value.entrySet().stream()
.filter(e -> e.getValue() == null)
.map(e -> e.getKey() instanceof String ? (String) e.getKey() : "")
.collect(Collectors.toSet());
x("null values found for keys: " +new TreeSet<>( badEntryKeys));
}

private static void x(String string){
System.out.println(string);
}
}
```
The test rules are as follows
```ql
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.FlowSources
import DataFlow::PartialPathGraph

class BeanValidationConfig extends TaintTracking::Configuration {
BeanValidationConfig() { this = "BeanValidationConfig" }

override predicate isSource(DataFlow::Node source) {
exists( Variable v | v.getName()="value" | v.getInitializer()= source.asExpr())
}

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
pred.asExpr() = succ.asExpr().(AddExpr).getAnOperand()
or
exists(MethodAccess ac | pred.asExpr() = ac.getAnArgument() and succ.asExpr() = ac)
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
super.allowImplicitRead(node, c)
or
this.isAdditionalTaintStep(node, _) and
(
c instanceof DataFlow::ArrayContent
or
c instanceof DataFlow::CollectionContent
or
c instanceof DataFlow::MapValueContent
)
}
override int explorationLimit() { result = 3}
}

predicate partial_flow(DataFlow::PartialPathNode n, DataFlow::Node src, int dist) {
exists(BeanValidationConfig conf, DataFlow::PartialPathNode source |
conf.hasPartialFlow(source, n, dist) and
src = source.getNode() and source.getNode().getLocation().getFile().getBaseName()="MapToSetTest.java"
)
}
```

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.