C++: Block flow out of sinks that are qualifiers. This removes the new result duplication and keeps the new result.

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

@@ -52,35 +52,25 @@ predicate isUnboundedWrite(BufferWrite bw) {
  * Holds if `e` is a source buffer going into an unbounded write `bw` or a
  * qualifier of (a qualifier of ...) such a source.
  */
-predicate unboundedWriteSource(Expr e, BufferWrite bw) {
-  isUnboundedWrite(bw) and e = bw.getASource()
+predicate unboundedWriteSource(Expr e, BufferWrite bw, boolean qualifier) {
+  isUnboundedWrite(bw) and e = bw.getASource() and qualifier = false
   or
-  exists(FieldAccess fa | unboundedWriteSource(fa, bw) and e = fa.getQualifier())
+  exists(FieldAccess fa | unboundedWriteSource(fa, bw, _) and e = fa.getQualifier()) and
+  qualifier = true
 }
 
 predicate isSource(FS::FlowSource source, string sourceType) { source.getSourceType() = sourceType }
 
-/**
- * Holds if `bw` is a `BufferWrite` that reads from `stdin`. In such cases the
- * sink is the outgoing argument that is written to.
- *
- * By factoring these cases out into this predicate we can place an out barrier
- * on exactly these sinks in `Config`.
- */
-predicate isSinkFromStdIn(DataFlow::Node sink, BufferWrite bw) {
+predicate isSink(DataFlow::Node sink, BufferWrite bw, boolean qualifier) {
+  unboundedWriteSource(sink.asIndirectExpr(), bw, qualifier)
+  or
   // `gets` and `scanf` reads from stdin so there's no real input.
   // The `BufferWrite` library models this as the call itself being
   // the source. In this case we mark the output argument as being
   // the sink so that we report a path where source = sink (because
   // the same output argument is also included in `isSource`).
   bw.getASource() = bw and
-  unboundedWriteSource(sink.asDefiningArgument(), bw)
-}
-
-predicate isSink(DataFlow::Node sink, BufferWrite bw) {
-  unboundedWriteSource(sink.asIndirectExpr(), bw)
-  or
-  isSinkFromStdIn(sink, bw)
+  unboundedWriteSource(sink.asDefiningArgument(), bw, qualifier)
 }
 
 predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
@@ -95,9 +85,9 @@ predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
 module Config implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { isSource(source, _) }
 
-  predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
+  predicate isSink(DataFlow::Node sink) { isSink(sink, _, _) }
 
-  predicate isBarrierOut(DataFlow::Node node) { isSinkFromStdIn(node, _) }
+  predicate isBarrierOut(DataFlow::Node node) { isSink(node, _, false) }
 
   predicate isBarrier(DataFlow::Node node) {
     // Block flow if the node is guarded by any <, <= or = operations.
@@ -127,7 +117,7 @@ from BufferWrite bw, Flow::PathNode source, Flow::PathNode sink, string sourceTy
 where
   Flow::flowPath(source, sink) and
   isSource(source.getNode(), sourceType) and
-  isSink(sink.getNode(), bw)
+  isSink(sink.getNode(), bw, _)
 select bw, source, sink,
   "This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.",
   source.getNode(), sourceType

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected

@@ -1,24 +1,18 @@
 edges
 | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection |
 | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection |
-| tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection |
-| tests.c:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection |
 | tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection |
 nodes
 | tests.c:16:26:16:29 | argv indirection | semmle.label | argv indirection |
 | tests.c:28:22:28:28 | access to array indirection | semmle.label | access to array indirection |
 | tests.c:29:28:29:34 | access to array indirection | semmle.label | access to array indirection |
-| tests.c:31:15:31:23 | buffer100 indirection | semmle.label | buffer100 indirection |
 | tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument |
-| tests.c:33:21:33:29 | buffer100 indirection | semmle.label | buffer100 indirection |
 | tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument |
 | tests.c:34:10:34:16 | access to array indirection | semmle.label | access to array indirection |
 subpaths
 #select
 | tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
 | tests.c:29:3:29:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
-| tests.c:31:15:31:23 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
 | tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | scanf output argument | tests.c:31:15:31:23 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | scanf output argument | value read by scanf |
-| tests.c:33:21:33:29 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
 | tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | scanf output argument | tests.c:33:21:33:29 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | scanf output argument | value read by scanf |
 | tests.c:34:25:34:33 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |