C++: Emulate old security library's use of predictable more accurately.

[geoffw0]

Changes to the defaulttainttracking library, to make it emulate use of the old security taint tracking's predictable predicate more accurately. This resolves at least one of the issues listed in https://jira.semmle.com/browse/CPP-491:

• new FPs in CWE-807/semmle/TaintedCondition/TaintedCondition.expected; in fact only one of them was an FP, the other is a TP (with the comment BAD, but it requires pointer analysis to catch), and this PR removes the FP but not the TP.

---

Test changes examined:

• ql/cpp/ql/test/query-tests/Security and semmlecode-cpp-tests/DO_NOT_DISTRIBUTE/security-tests
• from: defaulttainttracking enabled, before this PR
• to: defaulttainttracking enabled, with this PR

b/cpp/ql/test/library-tests/dataflow/security-taint/tainted.expected
@@ -14,9 +14,6 @@
 | test.cpp:38:23:38:28 | call to getenv | test.cpp:38:14:38:19 | envStr |  |
 | test.cpp:38:23:38:28 | call to getenv | test.cpp:38:23:38:28 | call to getenv |  |
 | test.cpp:38:23:38:28 | call to getenv | test.cpp:38:23:38:40 | (const char *)... |  |
-| test.cpp:38:23:38:28 | call to getenv | test.cpp:40:6:40:33 | ! ... |  |
-| test.cpp:38:23:38:28 | call to getenv | test.cpp:40:7:40:12 | call to strcmp |  |
-| test.cpp:38:23:38:28 | call to getenv | test.cpp:40:7:40:33 | (bool)... |  |
 | test.cpp:38:23:38:28 | call to getenv | test.cpp:40:14:40:19 | envStr |  |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:45:13:45:24 | envStrGlobal |  |
@@ -32,10 +29,6 @@
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:18:60:25 | userName |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:34 | call to getenv |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:47 | (const char *)... |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:10:64:14 | bytes |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:23 | call to strlen |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | (int)... |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | ... + ... |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:64:25:64:32 | userName |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:68:17:68:24 | userName |  |

^^^ these things weren't tainted with the old security taint tracking lib, and I don't think we want them, so these changes are desired.

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
@@ -4,6 +4,3 @@
 | test5.cpp:10:9:10:15 | call to strtoul | $@ flows to here and is used in an expression which might overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
 | test.c:44:7:44:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:51:17:51:20 | argv | User-provided value |
-| test.c:74:7:74:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:71:19:71:22 | argv | User-provided value |
-| test.c:84:7:84:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:81:19:81:22 | argv | User-provided value |
-| test.c:94:7:94:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:91:19:91:22 | argv | User-provided value |

^^^ these things weren't tainted with the old security taint tracking lib, and are labelled 'GOOD' in the source code, so these changes are desired.

cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected
@@ -1,3 +1,2 @@
 | test.cpp:24:10:24:35 | ! ... | Reliance on untrusted input $@ to raise privilege at $@ | test.cpp:20:29:20:34 | call to getenv | call to getenv | test.cpp:25:9:25:27 | ... = ... | ... = ... |
-| test.cpp:30:10:30:37 | ! ... | Reliance on untrusted input $@ to raise privilege at $@ | test.cpp:29:27:29:32 | call to getenv | call to getenv | test.cpp:31:9:31:27 | ... = ... | ... = ... |
 | test.cpp:41:10:41:38 | ! ... | Reliance on untrusted input $@ to raise privilege at $@ | test.cpp:20:29:20:34 | call to getenv | call to getenv | test.cpp:42:8:42:26 | ... = ... | ... = ... |

^^^ this wasn't reported with the old security taint tracking lib, and is labelled 'OK' in the source code, so this change is desired.
^^^ the result on line 41 wasn't reported with the old security taint tracking lib either, but appears to be a TP result so I'm happy we leave it in.

[geoffw0]

Rebased, fixed merge conflict.

[jbj]

Otherwise LGTM. I think you've struck the right balance here between legacy compatibility and future maintainability.

[jbj]

Two tests are failing.

[geoffw0]

Fixed the failing tests (they were new tests, and I believe the changes are correct).

[jbj]

In the PR description, should IntegerOverflowTainted.expected be ArithmeticTainted.expected? If so, I'm seeing the same results diff that you're seeing on the security queries.

[geoffw0]

In the PR description, should IntegerOverflowTainted.expected be ArithmeticTainted.expected?

I don't think so, I only saw changes for IntegerOverflowTainted. Something else could've changed since the 30th of January though - when I got the version of master I tested this on.