From 02a060fbfade5eefa9bb5876034474c36c4c7e1b Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 7 Dec 2018 16:16:19 +0000 Subject: [PATCH 1/4] CPP: Add a test. --- .../IncorrectPointerScaling.expected | 4 ++++ .../semmle/IncorrectPointerScaling/test_large.cpp | 10 ++++++++++ .../semmle/IncorrectPointerScaling/test_small.cpp | 13 +++++++++++++ 3 files changed, 27 insertions(+) create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected index f5ab81ebf057..dc77ca5d98cc 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected @@ -1,3 +1,7 @@ | test.cpp:50:19:50:19 | p | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type int * (size 4). | test.cpp:45:11:45:11 | test.cpp:45:11:45:11 | double | | test.cpp:94:18:94:18 | x | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type short * (size 2). | test.cpp:88:21:88:21 | test.cpp:88:21:88:21 | int | | test.cpp:130:27:130:29 | arr | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type short * (size 2). | test.cpp:128:16:128:18 | test.cpp:128:16:128:18 | int | +| test_large.cpp:9:22:9:24 | ptr | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type MyStruct * (size 16). | test_large.cpp:7:21:7:23 | test_large.cpp:7:21:7:23 | MyStruct | +| test_large.cpp:9:22:9:24 | ptr | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type MyStruct * (size 16). | test_small.cpp:10:21:10:23 | test_small.cpp:10:21:10:23 | MyStruct | +| test_small.cpp:12:22:12:24 | ptr | This pointer might have type $@ (size 16), but the pointer arithmetic here is done with type MyStruct * (size 8). | test_large.cpp:7:21:7:23 | test_large.cpp:7:21:7:23 | MyStruct | +| test_small.cpp:12:22:12:24 | ptr | This pointer might have type $@ (size 16), but the pointer arithmetic here is done with type MyStruct * (size 8). | test_small.cpp:10:21:10:23 | test_small.cpp:10:21:10:23 | MyStruct | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp new file mode 100644 index 000000000000..f63d32379d22 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp @@ -0,0 +1,10 @@ + +struct MyStruct +{ + int x, y, z, w; +}; + +void test(MyStruct *ptr) +{ + MyStruct *new_ptr = ptr + 1; // GOOD [FALSE POSITIVE] +} diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp new file mode 100644 index 000000000000..05e03626a174 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp @@ -0,0 +1,13 @@ +// note the two different `MyStruct` definitions, in test_small.cpp and test_large.cpp. These are +// in different translation units and we assume they are never linked into the same program (which +// would result in undefined behaviour). + +struct MyStruct +{ + int x, y; +}; + +void test(MyStruct *ptr) +{ + MyStruct *new_ptr = ptr + 1; // GOOD [FALSE POSITIVE] +} From 0f268cac40a7ed528607d8fa3b30a93c074c24ba Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 7 Dec 2018 17:40:12 +0000 Subject: [PATCH 2/4] CPP: Fix the issue. --- cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql | 2 +- .../IncorrectPointerScaling/IncorrectPointerScaling.expected | 4 ---- .../CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp | 2 +- .../CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql index 6417d025e6cf..fc542790e22f 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql @@ -34,7 +34,6 @@ private Type baseType(Type t) { ) // Make sure that the type has a size and that it isn't ambiguous. and strictcount(result.getSize()) = 1 - } /** @@ -98,6 +97,7 @@ predicate defSourceType(SsaDefinition def, LocalScopeVariable v, | p = v and def.definedByParameter(p) and sourceType = p.getType().getUnspecifiedType() and + strictcount(p.getType()) = 1 and isPointerType(sourceType) and sourceLoc = p.getLocation()) } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected index dc77ca5d98cc..f5ab81ebf057 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/IncorrectPointerScaling.expected @@ -1,7 +1,3 @@ | test.cpp:50:19:50:19 | p | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type int * (size 4). | test.cpp:45:11:45:11 | test.cpp:45:11:45:11 | double | | test.cpp:94:18:94:18 | x | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type short * (size 2). | test.cpp:88:21:88:21 | test.cpp:88:21:88:21 | int | | test.cpp:130:27:130:29 | arr | This pointer might have type $@ (size 4), but the pointer arithmetic here is done with type short * (size 2). | test.cpp:128:16:128:18 | test.cpp:128:16:128:18 | int | -| test_large.cpp:9:22:9:24 | ptr | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type MyStruct * (size 16). | test_large.cpp:7:21:7:23 | test_large.cpp:7:21:7:23 | MyStruct | -| test_large.cpp:9:22:9:24 | ptr | This pointer might have type $@ (size 8), but the pointer arithmetic here is done with type MyStruct * (size 16). | test_small.cpp:10:21:10:23 | test_small.cpp:10:21:10:23 | MyStruct | -| test_small.cpp:12:22:12:24 | ptr | This pointer might have type $@ (size 16), but the pointer arithmetic here is done with type MyStruct * (size 8). | test_large.cpp:7:21:7:23 | test_large.cpp:7:21:7:23 | MyStruct | -| test_small.cpp:12:22:12:24 | ptr | This pointer might have type $@ (size 16), but the pointer arithmetic here is done with type MyStruct * (size 8). | test_small.cpp:10:21:10:23 | test_small.cpp:10:21:10:23 | MyStruct | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp index f63d32379d22..6431f1ec1fca 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_large.cpp @@ -6,5 +6,5 @@ struct MyStruct void test(MyStruct *ptr) { - MyStruct *new_ptr = ptr + 1; // GOOD [FALSE POSITIVE] + MyStruct *new_ptr = ptr + 1; // GOOD } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp index 05e03626a174..4740fd6db2b0 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/IncorrectPointerScaling/test_small.cpp @@ -9,5 +9,5 @@ struct MyStruct void test(MyStruct *ptr) { - MyStruct *new_ptr = ptr + 1; // GOOD [FALSE POSITIVE] + MyStruct *new_ptr = ptr + 1; // GOOD } From d3c6d83786d90059fa4b6921ca39382f830bc0f8 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 7 Dec 2018 18:26:44 +0000 Subject: [PATCH 3/4] CPP: Change note. --- change-notes/1.20/analysis-cpp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.20/analysis-cpp.md b/change-notes/1.20/analysis-cpp.md index fa8ce23f5b71..3bf96b8b9fb4 100644 --- a/change-notes/1.20/analysis-cpp.md +++ b/change-notes/1.20/analysis-cpp.md @@ -11,6 +11,7 @@ | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| +| Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. | | Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. | ## Changes to QL libraries From 9857a8581742e594176e0af9a37161b54c392fc0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 7 Dec 2018 18:28:53 +0000 Subject: [PATCH 4/4] CPP: Fix similar queries. --- cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql | 1 + cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql | 1 + 2 files changed, 2 insertions(+) diff --git a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql index fa921e2beb42..c87f78c07eb4 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql @@ -89,6 +89,7 @@ predicate defSourceType(SsaDefinition def, LocalScopeVariable v, | p = v and def.definedByParameter(p) and sourceType = p.getType().getUnspecifiedType() and + strictcount(p.getType()) = 1 and isPointerType(sourceType) and sourceLoc = p.getLocation()) } diff --git a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql index c91c9a83e9f1..bcd76ca1790f 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingVoid.ql @@ -89,6 +89,7 @@ predicate defSourceType(SsaDefinition def, LocalScopeVariable v, | p = v and def.definedByParameter(p) and sourceType = p.getType().getUnspecifiedType() and + strictcount(p.getType()) = 1 and isPointerType(sourceType) and sourceLoc = p.getLocation()) }