diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql index 068c4e442891..af3b9744cf64 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql @@ -14,6 +14,7 @@ */ import cpp import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.dataflow.DataFlow2 /** * A function call to SetSecurityDescriptorDacl to set the ACL, specified by (2nd argument) bDaclPresent = TRUE @@ -28,9 +29,9 @@ class SetSecurityDescriptorDaclFunctionCall extends FunctionCall { /** * Dataflow that detects a call to SetSecurityDescriptorDacl with a NULL DACL as the pDacl argument */ -class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configuration { - SetSecurityDescriptorDaclFunctionConfiguration() { - this = "SetSecurityDescriptorDaclFunctionConfiguration" +class NullDaclConfig extends DataFlow::Configuration { + NullDaclConfig() { + this = "NullDaclConfig" } override predicate isSource(DataFlow::Node source) { @@ -49,6 +50,43 @@ class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configura } } +/** + * Dataflow that detects a call to SetSecurityDescriptorDacl with a pDacl + * argument that's _not_ likely to be NULL. + */ +class NonNullDaclConfig extends DataFlow2::Configuration { + NonNullDaclConfig() { + this = "NonNullDaclConfig" + } + + override predicate isSource(DataFlow::Node source) { + source.getType().getUnspecifiedType().(PointerType).getBaseType() = + any(Type t | t.getName() = "ACL").getUnspecifiedType() and + ( + // If the value comes from a function whose body we can't see, assume + // it's not null. + exists(Call call | + not exists(call.getTarget().getBlock()) and + source.asExpr() = call + ) + or + // If the value is assigned by reference, assume it's not null. The data + // flow library cannot currently follow flow from the body of a function to + // an assignment by reference, so this rule applies whether we see the + // body or not. + exists(Call call | + call.getAnArgument() = source.asDefiningArgument() + ) + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(SetSecurityDescriptorDaclFunctionCall call | + sink.asExpr() = call.getArgument(2) + ) + } +} + from SetSecurityDescriptorDaclFunctionCall call, string message where exists ( @@ -59,9 +97,10 @@ where exists ) or exists ( Expr constassign, VariableAccess var, - SetSecurityDescriptorDaclFunctionConfiguration config | + NullDaclConfig nullDaclConfig, NonNullDaclConfig nonNullDaclConfig | message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable " + var + " that is set to NULL will result in an unprotected object." | var = call.getArgument(2) - and config.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var)) + and nullDaclConfig.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var)) + and not nonNullDaclConfig.hasFlow(_, DataFlow::exprNode(var)) ) select call, message \ No newline at end of file diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll index 60a7b7e7e352..191f754e2d99 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll @@ -93,9 +93,17 @@ module FlowVar_internal { * - Supporting fields, globals and statics like the Java SSA library does. * - Supporting all local variables, even if their address is taken by * address-of, reference assignments, or lambdas. + * - Understanding that assignment to a field of a local struct is a + * definition of the struct but not a complete overwrite. This is what the + * IR library uses chi nodes for. */ predicate fullySupportedSsaVariable(Variable v) { v = any(SsaDefinition def).getAVariable() and + // After `foo(&x.field)` we need for there to be two definitions of `x`: + // the one that existed before the call to `foo` and the def-by-ref from + // the call. It's fundamental in SSA that each use is only associated with + // one def, so we can't easily get this effect with SSA. + not definitionByReference(v.getAnAccess(), _) and // SSA variables do not exist before their first assignment, but one // feature of this data flow library is to track where uninitialized data // ends up. @@ -183,8 +191,7 @@ module FlowVar_internal { } override predicate definedByReference(Expr arg) { - definitionByReference(v.getAnAccess(), arg) and - arg = def.getDefinition() + none() // Not supported for SSA. See `fullySupportedSsaVariable`. } override predicate definedByInitialValue(LocalScopeVariable param) { @@ -204,8 +211,6 @@ module FlowVar_internal { this.definedByExpr(_, _) or this.definedByInitialValue(_) - or - this.definedByReference(_) } /** @@ -236,10 +241,7 @@ module FlowVar_internal { BlockVar() { this = TBlockVar(sbb, v) } override VariableAccess getAnAccess() { - exists(SubBasicBlock reached | - reached = getAReachedBlockVarSBB(this) and - variableAccessInSBB(v, reached, result) - ) + variableAccessInSBB(v, getAReachedBlockVarSBB(this), result) } override predicate definedByInitialValue(LocalScopeVariable lsv) { @@ -401,8 +403,7 @@ module FlowVar_internal { mid = getAReachedBlockVarSBB(start) and result = mid.getASuccessor() and not skipLoop(mid, result, sbbDef, v) and - not assignmentLikeOperation(result, v, _) and - not blockVarDefinedByReference(result, v, _) + not assignmentLikeOperation(result, v, _) ) } @@ -413,12 +414,6 @@ module FlowVar_internal { va.getTarget() = v and va = sbb.getANode() and not overwrite(va, _) - or - // Allow flow into a `VariableAccess` that is used as definition by - // reference. This flow is blocked by `getAReachedBlockVarSBB` because - // flow should not propagate past that. - va = sbb.getASuccessor().(VariableAccess) and - blockVarDefinedByReference(va, v, _) } /** @@ -516,9 +511,6 @@ module FlowVar_internal { */ predicate overwrite(VariableAccess va, ControlFlowNode node) { va = node.(AssignExpr).getLValue() - or - va = node and - definitionByReference(node, _) } /** diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected index 30fd13629d7d..da5001b0b575 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected @@ -1,11 +1,16 @@ | example.c:15:37:15:37 | b | example.c:19:6:19:6 | b | | example.c:15:44:15:46 | pos | example.c:24:24:24:26 | pos | | example.c:15:44:15:46 | pos | example.c:28:23:28:25 | pos | +| example.c:17:19:17:22 | {...} | example.c:24:2:24:7 | coords | | example.c:17:19:17:22 | {...} | example.c:24:13:24:18 | coords | +| example.c:17:19:17:22 | {...} | example.c:26:2:26:7 | coords | +| example.c:17:19:17:22 | {...} | example.c:26:19:26:24 | coords | | example.c:24:13:24:30 | ... = ... | example.c:24:2:24:30 | ... = ... | | example.c:24:24:24:30 | ... + ... | example.c:24:13:24:30 | ... = ... | | example.c:26:13:26:16 | call to getX | example.c:26:2:26:25 | ... = ... | | example.c:26:18:26:24 | ref arg & ... | example.c:26:2:26:7 | coords | +| example.c:26:18:26:24 | ref arg & ... | example.c:26:19:26:24 | coords | +| example.c:28:22:28:25 | ref arg & ... | example.c:28:23:28:25 | pos | | test.cpp:6:12:6:17 | call to source | test.cpp:7:8:7:9 | t1 | | test.cpp:6:12:6:17 | call to source | test.cpp:8:8:8:9 | t1 | | test.cpp:6:12:6:17 | call to source | test.cpp:9:8:9:9 | t1 | @@ -31,14 +36,22 @@ | test.cpp:24:10:24:11 | t2 | test.cpp:26:8:26:9 | t1 | | test.cpp:430:48:430:54 | source1 | test.cpp:432:17:432:23 | source1 | | test.cpp:431:12:431:13 | 0 | test.cpp:432:11:432:13 | tmp | +| test.cpp:431:12:431:13 | 0 | test.cpp:432:33:432:35 | tmp | +| test.cpp:431:12:431:13 | 0 | test.cpp:433:8:433:10 | tmp | | test.cpp:432:10:432:13 | & ... | test.cpp:432:3:432:8 | call to memcpy | +| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:11:432:13 | tmp | +| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:33:432:35 | tmp | | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:433:8:433:10 | tmp | | test.cpp:432:17:432:23 | source1 | test.cpp:432:10:432:13 | ref arg & ... | | test.cpp:436:53:436:59 | source1 | test.cpp:439:17:439:23 | source1 | | test.cpp:436:66:436:66 | b | test.cpp:441:7:441:7 | b | | test.cpp:437:12:437:13 | 0 | test.cpp:438:19:438:21 | tmp | | test.cpp:437:12:437:13 | 0 | test.cpp:439:11:439:13 | tmp | +| test.cpp:437:12:437:13 | 0 | test.cpp:439:33:439:35 | tmp | +| test.cpp:437:12:437:13 | 0 | test.cpp:440:8:440:10 | tmp | +| test.cpp:437:12:437:13 | 0 | test.cpp:442:10:442:12 | tmp | | test.cpp:439:10:439:13 | & ... | test.cpp:439:3:439:8 | call to memcpy | +| test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:11:439:13 | tmp | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:33:439:35 | tmp | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:440:8:440:10 | tmp | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:442:10:442:12 | tmp | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index 9fa3ad7f2f35..343789012793 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -443,17 +443,17 @@ void flowThroughMemcpy_blockvar_with_local_flow(int source1, int b) { } } -void cleanedByMemcpy_ssa(int clean1) { +void cleanedByMemcpy_ssa(int clean1) { // currently modeled with BlockVar, not SSA int tmp; memcpy(&tmp, &clean1, sizeof tmp); - sink(tmp); // clean + sink(tmp); // clean [FALSE POSITIVE] } void cleanedByMemcpy_blockvar(int clean1) { int tmp; int *capture = &tmp; memcpy(&tmp, &clean1, sizeof tmp); - sink(tmp); // clean + sink(tmp); // clean [FALSE POSITIVE] } void intRefSource(int &ref_source); diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected index 38cbbc1bc4bc..b10a13c02c01 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected @@ -30,10 +30,17 @@ | test.cpp:433:8:433:10 | tmp | test.cpp:430:48:430:54 | source1 | | test.cpp:440:8:440:10 | tmp | test.cpp:436:53:436:59 | source1 | | test.cpp:442:10:442:12 | tmp | test.cpp:436:53:436:59 | source1 | +| test.cpp:449:8:449:10 | tmp | test.cpp:447:7:447:9 | tmp | +| test.cpp:456:8:456:10 | tmp | test.cpp:453:7:453:9 | tmp | +| test.cpp:466:8:466:12 | local | test.cpp:464:7:464:11 | local | | test.cpp:466:8:466:12 | local | test.cpp:465:16:465:20 | ref arg local | +| test.cpp:472:8:472:12 | local | test.cpp:470:7:470:11 | local | | test.cpp:472:8:472:12 | local | test.cpp:471:20:471:25 | ref arg & ... | +| test.cpp:478:8:478:12 | local | test.cpp:476:7:476:11 | local | | test.cpp:478:8:478:12 | local | test.cpp:477:20:477:24 | ref arg local | +| test.cpp:485:8:485:12 | local | test.cpp:483:7:483:11 | local | | test.cpp:485:8:485:12 | local | test.cpp:484:18:484:23 | ref arg & ... | +| test.cpp:491:8:491:12 | local | test.cpp:489:7:489:11 | local | | test.cpp:491:8:491:12 | local | test.cpp:490:18:490:22 | ref arg local | | true_upon_entry.cpp:21:8:21:8 | x | true_upon_entry.cpp:17:11:17:16 | call to source | | true_upon_entry.cpp:29:8:29:8 | x | true_upon_entry.cpp:27:9:27:14 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index f0fc017b75cb..91876a9a0d03 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -11,10 +11,17 @@ | test.cpp:430:48:430:54 | test.cpp:433:8:433:10 | AST only | | test.cpp:436:53:436:59 | test.cpp:440:8:440:10 | AST only | | test.cpp:436:53:436:59 | test.cpp:442:10:442:12 | AST only | +| test.cpp:447:7:447:9 | test.cpp:449:8:449:10 | AST only | +| test.cpp:453:7:453:9 | test.cpp:456:8:456:10 | AST only | +| test.cpp:464:7:464:11 | test.cpp:466:8:466:12 | AST only | | test.cpp:465:16:465:20 | test.cpp:466:8:466:12 | AST only | +| test.cpp:470:7:470:11 | test.cpp:472:8:472:12 | AST only | | test.cpp:471:20:471:25 | test.cpp:472:8:472:12 | AST only | +| test.cpp:476:7:476:11 | test.cpp:478:8:478:12 | AST only | | test.cpp:477:20:477:24 | test.cpp:478:8:478:12 | AST only | +| test.cpp:483:7:483:11 | test.cpp:485:8:485:12 | AST only | | test.cpp:484:18:484:23 | test.cpp:485:8:485:12 | AST only | +| test.cpp:489:7:489:11 | test.cpp:491:8:491:12 | AST only | | test.cpp:490:18:490:22 | test.cpp:491:8:491:12 | AST only | | true_upon_entry.cpp:9:11:9:16 | true_upon_entry.cpp:13:8:13:8 | IR only | | true_upon_entry.cpp:62:11:62:16 | true_upon_entry.cpp:66:8:66:8 | IR only | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected index 82b70e6a8c42..6cfc73519dd3 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected @@ -1,4 +1,20 @@ | test.cpp:75:7:75:8 | u1 | test.cpp:76:8:76:9 | u1 | | test.cpp:83:7:83:8 | u2 | test.cpp:84:13:84:14 | u2 | | test.cpp:83:7:83:8 | u2 | test.cpp:85:8:85:9 | u2 | +| test.cpp:447:7:447:9 | tmp | test.cpp:448:11:448:13 | tmp | +| test.cpp:447:7:447:9 | tmp | test.cpp:449:8:449:10 | tmp | | test.cpp:453:7:453:9 | tmp | test.cpp:454:19:454:21 | tmp | +| test.cpp:453:7:453:9 | tmp | test.cpp:455:11:455:13 | tmp | +| test.cpp:453:7:453:9 | tmp | test.cpp:456:8:456:10 | tmp | +| test.cpp:464:7:464:11 | local | test.cpp:465:16:465:20 | local | +| test.cpp:464:7:464:11 | local | test.cpp:466:8:466:12 | local | +| test.cpp:470:7:470:11 | local | test.cpp:471:21:471:25 | local | +| test.cpp:470:7:470:11 | local | test.cpp:472:8:472:12 | local | +| test.cpp:476:7:476:11 | local | test.cpp:477:20:477:24 | local | +| test.cpp:476:7:476:11 | local | test.cpp:478:8:478:12 | local | +| test.cpp:476:7:476:11 | local | test.cpp:479:9:479:13 | local | +| test.cpp:483:7:483:11 | local | test.cpp:484:19:484:23 | local | +| test.cpp:483:7:483:11 | local | test.cpp:485:8:485:12 | local | +| test.cpp:489:7:489:11 | local | test.cpp:490:18:490:22 | local | +| test.cpp:489:7:489:11 | local | test.cpp:491:8:491:12 | local | +| test.cpp:489:7:489:11 | local | test.cpp:492:9:492:13 | local | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 601ad0a33528..6cf3e7bda227 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -128,13 +128,24 @@ | taint.cpp:164:19:164:24 | call to source | taint.cpp:168:8:168:14 | tainted | | | taint.cpp:164:19:164:24 | call to source | taint.cpp:172:18:172:24 | tainted | | | taint.cpp:165:22:165:25 | {...} | taint.cpp:170:10:170:15 | buffer | | +| taint.cpp:165:22:165:25 | {...} | taint.cpp:171:8:171:13 | buffer | | +| taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer | | +| taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT | | taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy | | +| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:10:170:15 | buffer | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | | +| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | +| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | +| taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat | | +| taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:173:8:173:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p | | | taint.cpp:181:9:181:9 | p | taint.cpp:181:8:181:9 | * ... | TAINT | | taint.cpp:185:11:185:16 | call to source | taint.cpp:186:11:186:11 | x | | +| taint.cpp:186:10:186:11 | ref arg & ... | taint.cpp:186:11:186:11 | x | | | taint.cpp:186:11:186:11 | x | taint.cpp:186:10:186:11 | & ... | TAINT | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp index 8c5e0e632d84..f2f7d80e44a2 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp @@ -89,4 +89,44 @@ void Test() NULL, // DACL is going to be removed from security descriptor. Default/inherited access ==> should not be flagged FALSE); -} \ No newline at end of file +} + +PACL returnUnknownAcl(); + +PACL returnNull() { + return NULL; +} + +PACL returnMaybeAcl(bool b) { + PACL pDacl = NULL; + if (b) { + SetEntriesInAcl(0, NULL, NULL, &pDacl); + } + return pDacl; +} + +void Test2() +{ + PSECURITY_DESCRIPTOR pSecurityDescriptor; + + PACL pDacl1 = returnUnknownAcl(); + SetSecurityDescriptorDacl( + pSecurityDescriptor, + TRUE, // Dacl Present + pDacl1, // Give `returnUnknownAcl` the benefit of the doubt ==> should not be flagged + FALSE); + + PACL pDacl2 = returnNull(); + SetSecurityDescriptorDacl( + pSecurityDescriptor, + TRUE, // Dacl Present + pDacl2, // NULL pointer to DACL == BUG + FALSE); + + PACL pDacl3 = returnMaybeAcl(true); + SetSecurityDescriptorDacl( + pSecurityDescriptor, + TRUE, // Dacl Present + pDacl3, // should not be flagged + FALSE); +} diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected index e596bae3a0f0..7881591c309f 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected @@ -1,2 +1,3 @@ | UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object. | | UnsafeDaclSecurityDescriptor.cpp:76:9:76:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable pDacl that is set to NULL will result in an unprotected object. | +| UnsafeDaclSecurityDescriptor.cpp:120:5:120:29 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable pDacl2 that is set to NULL will result in an unprotected object. |