Skip to content

Commit c2cd58e

Browse files
yoffyoff
committed
python: rewrite to separate configurations
source nodes get duplicated, so perhaps flow states are actually better for performance?
1 parent 7df5c70 commit c2cd58e

File tree

4 files changed

+133
-69
lines changed

4 files changed

+133
-69
lines changed

‎python/ql/lib/semmle/python/security/dataflow/LdapInjection.qll

Lines changed: 29 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -8,80 +8,42 @@ import semmle.python.dataflow.new.DataFlow
88
import semmle.python.dataflow.new.TaintTracking
99
import semmle.python.dataflow.new.RemoteFlowSources
1010

11-
/**
12-
* A taint-tracking configuration for detecting LDAP injections.
13-
*/
14-
class LDAPInjectionFlowConfig extends TaintTracking::Configuration {
15-
LDAPInjectionFlowConfig() { this = "LDAPInjectionFlowConfig" }
11+
module LdapInjection {
12+
import LdapInjectionCustomizations::LdapInjection
1613

17-
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
18-
source instanceof RemoteFlowSource and
19-
state instanceof Unsafe
20-
}
14+
class DnConfiguration extends TaintTracking::Configuration {
15+
DnConfiguration() { this = "LdapDnInjection" }
2116

22-
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
23-
sink = any(LdapExecution ldap).getBaseDn() and
24-
(state instanceof Unsafe or state instanceof UnsafeForDn)
25-
or
26-
sink = any(LdapExecution ldap).getFilter() and
27-
(state instanceof Unsafe or state instanceof UnsafeForFilter)
28-
}
17+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
2918

30-
override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
31-
// All additional flow steps signify a state change.
32-
// (n, `state`) --> (`node`, s)
33-
// Thus, if a node in `state` transitions to `node` and a new state,
34-
// then `state` should be blocked at `node`.
35-
isAdditionalFlowStep(_, state, node, _)
36-
}
19+
override predicate isSink(DataFlow::Node sink) { sink instanceof DnSink }
3720

38-
override predicate isAdditionalFlowStep(
39-
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
40-
DataFlow::FlowState state2
41-
) {
42-
exists(LdapDnEscaping ldapDnEsc |
43-
node1 = ldapDnEsc.getAnInput() and
44-
node2 = ldapDnEsc.getOutput()
45-
|
46-
state1 instanceof Unsafe and
47-
state2 instanceof UnsafeForFilter
48-
or
49-
state1 instanceof UnsafeForDn and
50-
state2 instanceof Safe
51-
)
52-
or
53-
exists(LdapFilterEscaping ldapFilterEsc |
54-
node1 = ldapFilterEsc.getAnInput() and
55-
node2 = ldapFilterEsc.getOutput()
56-
|
57-
state1 instanceof Unsafe and
58-
state2 instanceof UnsafeForDn
59-
or
60-
state1 instanceof UnsafeForFilter and
61-
state2 instanceof Safe
62-
)
21+
override predicate isSanitizer(DataFlow::Node node) { node instanceof DnSanitizer }
22+
23+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
24+
guard instanceof DnSanitizerGuard
25+
}
6326
}
64-
}
6527

66-
/** A flow satte signifying generally unsafe input. */
67-
class Unsafe extends DataFlow::FlowState {
68-
Unsafe() { this = "Unsafe" }
69-
}
28+
class FilterConfiguration extends TaintTracking::Configuration {
29+
FilterConfiguration() { this = "LdapFilterInjection" }
7030

71-
/** A flow state signifying input that is only unsafe for DNs. */
72-
class UnsafeForDn extends DataFlow::FlowState {
73-
UnsafeForDn() { this = "UnsafeForDn" }
74-
}
31+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
7532

76-
/** A flow state signifying input that is only unsafe for filter strings. */
77-
class UnsafeForFilter extends DataFlow::FlowState {
78-
UnsafeForFilter() { this = "UnsafeForFilter" }
79-
}
33+
override predicate isSink(DataFlow::Node sink) { sink instanceof FilterSink }
8034

81-
/**
82-
* A flow state that signifies safe input.
83-
* Including this makes `isAdditionalFlowStep` and `isBarrier` simpler.
84-
*/
85-
class Safe extends DataFlow::FlowState {
86-
Safe() { this = "Safe" }
35+
override predicate isSanitizer(DataFlow::Node node) { node instanceof FilterSanitizer }
36+
37+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
38+
guard instanceof FilterSanitizerGuard
39+
}
40+
}
41+
42+
import DataFlow::PathGraph
43+
44+
predicate ldapInjection(DataFlow::PathNode source, DataFlow::PathNode sink) {
45+
any(DnConfiguration dnConfig).hasFlowPath(source, sink)
46+
or
47+
any(FilterConfiguration filterConfig).hasFlowPath(source, sink)
48+
}
8749
}

‎python/ql/lib/semmle/python/security/dataflow/LdapInjectionCustomizations.qll

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting
3+
* "ldap injection"
4+
* vulnerabilities, as well as extension points for adding your own.
5+
*/
6+
7+
private import python
8+
private import semmle.python.dataflow.new.DataFlow
9+
private import semmle.python.Concepts
10+
private import semmle.python.dataflow.new.RemoteFlowSources
11+
private import semmle.python.dataflow.new.BarrierGuards
12+
13+
/**
14+
* Provides default sources, sinks and sanitizers for detecting
15+
* "ldap injection"
16+
* vulnerabilities, as well as extension points for adding your own.
17+
*/
18+
module LdapInjection {
19+
/**
20+
* A data flow source for "ldap injection" vulnerabilities.
21+
*/
22+
abstract class Source extends DataFlow::Node { }
23+
24+
/**
25+
* A data flow sink for "ldap injection" vulnerabilities.
26+
*/
27+
abstract class DnSink extends DataFlow::Node { }
28+
29+
/**
30+
* A data flow sink for "ldap injection" vulnerabilities.
31+
*/
32+
abstract class FilterSink extends DataFlow::Node { }
33+
34+
/**
35+
* A sanitizer for "ldap injection" vulnerabilities.
36+
*/
37+
abstract class DnSanitizer extends DataFlow::Node { }
38+
39+
/**
40+
* A sanitizer for "ldap injection" vulnerabilities.
41+
*/
42+
abstract class FilterSanitizer extends DataFlow::Node { }
43+
44+
/**
45+
* A sanitizer guard for "ldap injection" vulnerabilities.
46+
*/
47+
abstract class DnSanitizerGuard extends DataFlow::BarrierGuard { }
48+
49+
/**
50+
* A sanitizer guard for "ldap injection" vulnerabilities.
51+
*/
52+
abstract class FilterSanitizerGuard extends DataFlow::BarrierGuard { }
53+
54+
/**
55+
* A source of remote user input, considered as a flow source.
56+
*/
57+
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
58+
59+
/**
60+
* A logging operation, considered as a flow sink.
61+
*/
62+
class LdapExecutionAsDnSink extends DnSink {
63+
LdapExecutionAsDnSink() { this = any(LdapExecution ldap).getBaseDn() }
64+
}
65+
66+
/**
67+
* A logging operation, considered as a flow sink.
68+
*/
69+
class LdapExecutionAsFilterSink extends FilterSink {
70+
LdapExecutionAsFilterSink() { this = any(LdapExecution ldap).getFilter() }
71+
}
72+
73+
/**
74+
* A comparison with a constant string, considered as a sanitizer-guard.
75+
*/
76+
class StringConstCompareAsDnSanitizerGuard extends DnSanitizerGuard, StringConstCompare { }
77+
78+
/**
79+
* A comparison with a constant string, considered as a sanitizer-guard.
80+
*/
81+
class StringConstCompareAsFilterSanitizerGuard extends FilterSanitizerGuard, StringConstCompare {
82+
}
83+
84+
/**
85+
* A call to replace line breaks functions as a sanitizer.
86+
*/
87+
class LdapDnEscapingSanitizer extends DnSanitizer, DataFlow::CallCfgNode {
88+
LdapDnEscapingSanitizer() { this = any(LdapDnEscaping ldapDnEsc).getOutput() }
89+
}
90+
91+
/**
92+
* A call to replace line breaks functions as a sanitizer.
93+
*/
94+
class LdapFilterEscapingSanitizer extends FilterSanitizer, DataFlow::CallCfgNode {
95+
LdapFilterEscapingSanitizer() { this = any(LdapFilterEscaping ldapDnEsc).getOutput() }
96+
}
97+
}

‎python/ql/src/Security/CWE-090/LdapInjection.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import python
1616
import semmle.python.security.dataflow.LdapInjection
1717
import DataFlow::PathGraph
1818

19-
from LDAPInjectionFlowConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
20-
where config.hasFlowPath(source, sink)
19+
from DataFlow::PathNode source, DataFlow::PathNode sink
20+
where LdapInjection::ldapInjection(source, sink)
2121
select sink.getNode(), source, sink, "$@ LDAP query parameter comes from $@.", sink.getNode(),
2222
"This", source.getNode(), "a user-provided value"

‎python/ql/test/query-tests/Security/CWE-090-LdapInjection/LdapInjection.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ edges
4141
| ldap_bad.py:48:21:48:44 | ControlFlowNode for Subscript | ldap_bad.py:55:43:55:55 | ControlFlowNode for search_filter |
4242
nodes
4343
| ldap3_bad.py:13:17:13:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
44+
| ldap3_bad.py:13:17:13:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
4445
| ldap3_bad.py:13:17:13:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
4546
| ldap3_bad.py:13:17:13:34 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
4647
| ldap3_bad.py:14:21:14:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -49,6 +50,7 @@ nodes
4950
| ldap3_bad.py:21:17:21:18 | ControlFlowNode for dn | semmle.label | ControlFlowNode for dn |
5051
| ldap3_bad.py:21:21:21:33 | ControlFlowNode for search_filter | semmle.label | ControlFlowNode for search_filter |
5152
| ldap3_bad.py:30:17:30:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
53+
| ldap3_bad.py:30:17:30:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
5254
| ldap3_bad.py:30:17:30:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
5355
| ldap3_bad.py:30:17:30:34 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
5456
| ldap3_bad.py:31:21:31:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -57,6 +59,7 @@ nodes
5759
| ldap3_bad.py:38:9:38:10 | ControlFlowNode for dn | semmle.label | ControlFlowNode for dn |
5860
| ldap3_bad.py:38:13:38:25 | ControlFlowNode for search_filter | semmle.label | ControlFlowNode for search_filter |
5961
| ldap_bad.py:13:17:13:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
62+
| ldap_bad.py:13:17:13:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
6063
| ldap_bad.py:13:17:13:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
6164
| ldap_bad.py:13:17:13:34 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
6265
| ldap_bad.py:14:21:14:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -65,6 +68,7 @@ nodes
6568
| ldap_bad.py:21:9:21:10 | ControlFlowNode for dn | semmle.label | ControlFlowNode for dn |
6669
| ldap_bad.py:21:33:21:45 | ControlFlowNode for search_filter | semmle.label | ControlFlowNode for search_filter |
6770
| ldap_bad.py:30:17:30:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
71+
| ldap_bad.py:30:17:30:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
6872
| ldap_bad.py:30:17:30:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
6973
| ldap_bad.py:30:17:30:34 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
7074
| ldap_bad.py:31:21:31:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -73,6 +77,7 @@ nodes
7377
| ldap_bad.py:37:9:37:10 | ControlFlowNode for dn | semmle.label | ControlFlowNode for dn |
7478
| ldap_bad.py:37:33:37:45 | ControlFlowNode for search_filter | semmle.label | ControlFlowNode for search_filter |
7579
| ldap_bad.py:47:17:47:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
80+
| ldap_bad.py:47:17:47:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
7681
| ldap_bad.py:47:17:47:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
7782
| ldap_bad.py:47:17:47:34 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
7883
| ldap_bad.py:48:21:48:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |

0 commit comments

Comments
 (0)