Skip to content

Commit 2fb2480

Browse files
marek-trtikmarek-trtik
authored
Merge pull request diffblue#541 from diffblue/bugfix_apply_taint_summary_with_obtained_summary_inputs
SEC-633: Bugfix apply taint summary with obtained summary inputs
2 parents c632d0d + b174f06 commit 2fb2480

File tree

11 files changed

+269
-17
lines changed

11 files changed

+269
-17
lines changed

‎regression/end_to_end/xxe01/build.xml

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<project name="Main" basedir="." default="compile">
2+
3+
<property name="root.dir" value="./"/>
4+
<property name="src.dir" value="${root.dir}/src"/>
5+
<property name="classes.dir" value="${root.dir}/build"/>
6+
7+
<target name="compile">
8+
<antcall target="clean" />
9+
<mkdir dir="${classes.dir}"/>
10+
<javac srcdir="${src.dir}" destdir="${classes.dir}" includeantruntime="false" debug="on" />
11+
</target>
12+
13+
<target name="clean">
14+
<delete dir="${classes.dir}"/>
15+
</target>
16+
17+
</project>

‎regression/end_to_end/xxe01/rules.json

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
{
2+
"namespace": "com.diffblue.security",
3+
"rules":
4+
[
5+
{
6+
"comment": "Obtaining tainted XML text.",
7+
"class": "xxe01.Main",
8+
"method": "make_tainted:(Ljava/lang/String;)Ljava/lang/String;",
9+
"result": {
10+
"location": "returns",
11+
"taint": "Tainted XML text"
12+
}
13+
},
14+
{
15+
"comment": "Obtaining XML stream reader with external entities enabled for tainted XML text.",
16+
"class": "xxe01.XMLInputFactory",
17+
"method": "DIFFBLUE_createXMLStreamReader:(Ljava/lang/String;)Lxxe01/XMLStreamReader;",
18+
"input": {
19+
"location": "arg1",
20+
"taint": "Tainted XML text"
21+
},
22+
"result": {
23+
"location": "returns",
24+
"taint": "Reader of tainted XML with external entities enabled"
25+
}
26+
},
27+
{
28+
"comment": "Unmarshalling an object by reading tainted XML document with external entities enabled.",
29+
"class": "xxe01.Unmarshaller",
30+
"method": "unmarshal:(Lxxe01/XMLStreamReader;)Ljava/lang/Object;",
31+
"sinkTarget": {
32+
"location": "arg1",
33+
"vulnerability": "Reader of tainted XML with external entities enabled"
34+
},
35+
"message": "Unmarshalling an object by reading tainted XML document with external entities enabled."
36+
}
37+
]
38+
}
39+
40+

‎regression/end_to_end/xxe01/src/JAXBContext.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package xxe01;
2+
3+
4+
class JAXBContext {
5+
public static JAXBContext newInstance(String s) {
6+
return new JAXBContext();
7+
}
8+
9+
public Unmarshaller createUnmarshaller() {
10+
return new Unmarshaller();
11+
}
12+
}
13+

‎regression/end_to_end/xxe01/src/Main.java

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package xxe01;
2+
3+
4+
public class Main {
5+
6+
private static String make_tainted(String s) {
7+
return s;
8+
}
9+
10+
private static void test_no_xxe_explicit(String xml_from_attacker) {
11+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
12+
XMLInputFactory xif = XMLInputFactory.newFactory();
13+
xif.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
14+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
15+
Unmarshaller unmarshaller = jc.createUnmarshaller();
16+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
17+
}
18+
19+
private static void test_no_xxe_implicit(String xml_from_attacker) {
20+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
21+
XMLInputFactory xif = XMLInputFactory.newFactory();
22+
//xif.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
23+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
24+
Unmarshaller unmarshaller = jc.createUnmarshaller();
25+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
26+
}
27+
28+
private static void test_simple(String xml_from_attacker) {
29+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
30+
XMLInputFactory xif = XMLInputFactory.newFactory();
31+
xif.setProperty("javax.xml.stream.isSupportingExternalEntities", true);
32+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
33+
Unmarshaller unmarshaller = jc.createUnmarshaller();
34+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
35+
}
36+
37+
private static void test_guess_value(String xml_from_attacker, boolean property_value) {
38+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
39+
XMLInputFactory xif = XMLInputFactory.newFactory();
40+
xif.setProperty("javax.xml.stream.isSupportingExternalEntities", property_value);
41+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
42+
Unmarshaller unmarshaller = jc.createUnmarshaller();
43+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
44+
}
45+
46+
private static void test_guess_property(String xml_from_attacker, String property_name) {
47+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
48+
XMLInputFactory xif = XMLInputFactory.newFactory();
49+
xif.setProperty(property_name, true);
50+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
51+
Unmarshaller unmarshaller = jc.createUnmarshaller();
52+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
53+
}
54+
55+
private static void test_guess_property_and_value(String xml_from_attacker, String property_name, boolean property_value) {
56+
JAXBContext jc = JAXBContext.newInstance("xxe01.MyClass");
57+
XMLInputFactory xif = XMLInputFactory.newFactory();
58+
xif.setProperty(property_name, property_value);
59+
XMLStreamReader xsr = xif.createXMLStreamReader(xml_from_attacker);
60+
Unmarshaller unmarshaller = jc.createUnmarshaller();
61+
MyClass myClass = (MyClass)unmarshaller.unmarshal(xsr);
62+
}
63+
64+
public static void main(String[] args) {
65+
if (args.length < 3)
66+
return;
67+
68+
String xml_from_attacker = make_tainted(args[0]);
69+
String property_name = args[1];
70+
boolean property_value = false;
71+
if (args[2] == "TRUE")
72+
property_value = true;
73+
74+
test_no_xxe_explicit(xml_from_attacker);
75+
test_no_xxe_implicit(xml_from_attacker);
76+
test_simple(xml_from_attacker);
77+
test_guess_value(xml_from_attacker, property_value);
78+
test_guess_property(xml_from_attacker, property_name);
79+
test_guess_property_and_value(xml_from_attacker, property_name, property_value);
80+
}
81+
}
82+

‎regression/end_to_end/xxe01/src/MyClass.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package xxe01;
2+
3+
4+
// This is user's class to be unmarshaled from XML data.
5+
class MyClass {
6+
}
7+

‎regression/end_to_end/xxe01/src/Unmarshaller.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package xxe01;
2+
3+
4+
class Unmarshaller {
5+
public Object unmarshal(XMLStreamReader xsr) {
6+
return new MyClass();
7+
}
8+
}
9+

‎regression/end_to_end/xxe01/src/XMLInputFactory.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package xxe01;
2+
import java.util.HashMap;
3+
4+
5+
class XMLInputFactory {
6+
public static XMLInputFactory newFactory() {
7+
return new XMLInputFactory();
8+
}
9+
10+
public XMLInputFactory() {
11+
this.props = new HashMap<String, Object>();
12+
this.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
13+
}
14+
15+
public void setProperty(String key, Object value) {
16+
this.props.put(key, value);
17+
18+
// DIFFBLUE MODEL - BEGIN
19+
if (key == "javax.xml.stream.isSupportingExternalEntities") {
20+
if (value instanceof Boolean) {
21+
if ((Boolean)value == true)
22+
this.DIFFBLUE_isSupportingExternalEntities = true;
23+
else
24+
this.DIFFBLUE_isSupportingExternalEntities = false;
25+
}
26+
}
27+
// DIFFBLUE MODEL - END
28+
}
29+
30+
public Object getProperty(String key) {
31+
return this.props.get(key);
32+
}
33+
34+
public XMLStreamReader createXMLStreamReader(String xml) {
35+
// DIFFBLUE MODEL - BEGIN
36+
if (DIFFBLUE_isSupportingExternalEntities == true)
37+
return DIFFBLUE_createXMLStreamReader(xml);
38+
// DIFFBLUE MODEL - END
39+
return new XMLStreamReader();
40+
}
41+
42+
private HashMap<String, Object> props;
43+
44+
// DIFFBLUE MODEL - BEGIN
45+
private XMLStreamReader DIFFBLUE_createXMLStreamReader(String xml) {
46+
return new XMLStreamReader();
47+
}
48+
49+
private boolean DIFFBLUE_isSupportingExternalEntities;
50+
// DIFFBLUE MODEL - END
51+
}
52+

‎regression/end_to_end/xxe01/src/XMLStreamReader.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package xxe01;
2+
3+
4+
class XMLStreamReader {
5+
}
6+

‎regression/end_to_end/xxe01/test_xxe01.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import fasteners
2+
import os
3+
import subprocess
4+
import pytest
5+
6+
from regression.end_to_end.driver import run_security_analyser_pipeline
7+
import regression.utils as utils
8+
9+
10+
@pytest.mark.xfail(strict=True)
11+
@fasteners.interprocess_locked(os.path.join(os.path.dirname(__file__), ".build_lock"))
12+
def test_xxe01(load_strategy):
13+
with utils.working_dir(os.path.abspath(os.path.dirname(__file__))):
14+
subprocess.call(["ant"])
15+
with run_security_analyser_pipeline(
16+
"build",
17+
"rules.json",
18+
os.path.realpath(os.path.dirname(__file__)),
19+
"xxe01.Main.main",
20+
load_strategy) as traces:
21+
assert traces.count_traces() == 4
22+
assert traces.trace_exists(
23+
"java::xxe01.Main.test_simple:(Ljava/lang/String;)V", 34)
24+
assert traces.trace_exists(
25+
"java::xxe01.Main.test_guess_value:(Ljava/lang/String;B)V", 43)
26+
assert traces.trace_exists(
27+
"java::xxe01.Main.test_guess_property:(Ljava/lang/String;Ljava/lang/String;)V", 52)
28+
assert traces.trace_exists(
29+
"java::xxe01.Main.test_guess_property:(Ljava/lang/String;Ljava/lang/String;B)V", 61)

‎src/taint-analysis/taint_summary.cpp

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,7 @@ void taint_algorithm_computing_summary_of_functiont::
591591
std::map<taint_variablet,taint_sett> &symbols_substitution,
592592
const numbered_lvalue_to_taint_mapt &a,
593593
const std::shared_ptr<taint_summaryt> summary,
594+
const taint_summary_inputt &caller_summary_input,
594595
const code_function_callt &fn_call,
595596
const code_typet &fn_type,
596597
const local_value_set_analysist &lvsa,
@@ -656,6 +657,12 @@ void taint_algorithm_computing_summary_of_functiont::
656657
const auto it = a.find(lvalue);
657658
if(it != a.cend())
658659
argument_taint |= it->second;
660+
else
661+
{
662+
const auto input_it = caller_summary_input.find(lvalue);
663+
if(input_it != caller_summary_input.cend())
664+
argument_taint += input_it->second;
665+
}
659666
}
660667

661668
symbols_substitution.insert({ input.second, argument_taint });
@@ -759,25 +766,13 @@ void taint_algorithm_computing_summary_of_functiont::initialise_input(
759766
continue;
760767
irep_idt callee_id = to_symbol_expr(callee_expr).get_identifier();
761768

762-
if(!database.contains(callee_id) || transition_rules->has_rule(callee_id))
769+
for(const auto &arg : fn_call.arguments())
763770
{
764-
// This is either a recursive function or a function for which we have
765-
// a rule (any function not in the database has not been processed and
766-
// since we're following an inverted topological ordering it therefore
767-
// must recursively call us)
768-
// If we don't have a summary then we assume that the function could
769-
// use any of its arguments. If the function has a rule then it
770-
// probably will use some of its arguments.
771-
// In the future we could be more precise about exactly which arguments
772-
// are used in the rule.
773-
for(const auto &arg : fn_call.arguments())
774-
{
775-
collect_lvsa_access_paths_extended(
776-
arg, it, lvsa, environment, false);
777-
}
778-
if(!database.contains(callee_id))
779-
continue;
771+
collect_lvsa_access_paths_extended(
772+
arg, it, lvsa, environment, false);
780773
}
774+
if(!database.contains(callee_id))
775+
continue;
781776

782777
const std::shared_ptr<taint_summaryt> summary = database.at(callee_id);
783778
for(const std::pair<taint_lvalue_numbert, taint_sett> &lvalue_taint
@@ -1031,6 +1026,7 @@ taint_algorithm_computing_summary_of_functiont::transform(
10311026
symbols_substitution,
10321027
a,
10331028
summary,
1029+
input,
10341030
fn_call,
10351031
fn_type,
10361032
lvsa,

‎src/taint-analysis/taint_summary.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,7 @@ class taint_algorithm_computing_summary_of_functiont
361361
std::map<taint_variablet,taint_sett> &symbols_substitution,
362362
const numbered_lvalue_to_taint_mapt &a,
363363
const std::shared_ptr<taint_summaryt> summary,
364+
const taint_summary_inputt &caller_summary_input,
364365
const code_function_callt &fn_call,
365366
const code_typet &fn_type,
366367
const local_value_set_analysist &lvsa,

0 commit comments

Comments
 (0)