From 3d779ddebb3b194cadaafcbd57ee15d45173d391 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Thu, 8 Nov 2018 18:04:31 +0000 Subject: [PATCH] Bring qhelp inline with current guidelines --- cpp/ql/src/Critical/DeadCodeCondition.qhelp | 25 +++++++++-------- cpp/ql/src/Critical/DeadCodeFunction.cpp | 2 +- cpp/ql/src/Critical/DeadCodeFunction.qhelp | 27 ++++++++++-------- cpp/ql/src/Critical/DeadCodeFunction.ql | 2 +- cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp | 18 ++++++------ cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 4 +-- .../InconsistentNullnessTesting.qhelp | 27 ++++++++++-------- .../Critical/InconsistentNullnessTesting.ql | 2 +- cpp/ql/src/Critical/OverflowCalculated.qhelp | 28 ++++++++++++------- cpp/ql/src/Critical/OverflowDestination.qhelp | 22 +++++++++------ cpp/ql/src/Critical/OverflowDestination.ql | 2 +- 11 files changed, 91 insertions(+), 68 deletions(-) diff --git a/cpp/ql/src/Critical/DeadCodeCondition.qhelp b/cpp/ql/src/Critical/DeadCodeCondition.qhelp index 10b1e3253a1..c0c65c9e017 100644 --- a/cpp/ql/src/Critical/DeadCodeCondition.qhelp +++ b/cpp/ql/src/Critical/DeadCodeCondition.qhelp @@ -5,23 +5,26 @@ -

This rule finds branching statements with conditions that always evaluate to the same value. -More likely than not these conditions indicate a defect in the branching condition or are an artifact left behind after debugging.

+

This query finds branching statements with conditions that always evaluate to the same value. +It is likely that these conditions indicate an error in the branching condition. +Alternatively, the conditions may have been left behind after debugging.

- + -

Check the branch condition for defects, and verify that it isn't a remnant from debugging.

- +

Check the branch condition for logic errors. Check whether it is still required.

 - - - - - - + +

This example shows two branch conditions that always evaluate to the same value. +The two conditions and their associated branches should be deleted. +This will simplify the code and make it easier to maintain.

+ + + +
  • SEI CERT C++ Coding Standard MSC12-C. Detect and remove code that has no effect or is never executed.
    • +     diff --git a/cpp/ql/src/Critical/DeadCodeFunction.cpp b/cpp/ql/src/Critical/DeadCodeFunction.cpp index 1cdcc652d97..553104e2b3e 100644 --- a/cpp/ql/src/Critical/DeadCodeFunction.cpp +++ b/cpp/ql/src/Critical/DeadCodeFunction.cpp @@ -2,7 +2,7 @@ class C { public: void g() { ... - //f() was previously used but is now commented, orphaning f() + //f() was previously used but is now commented-out, orphaning f() //f(); ... } diff --git a/cpp/ql/src/Critical/DeadCodeFunction.qhelp b/cpp/ql/src/Critical/DeadCodeFunction.qhelp index f43e6cefa91..f5c4b432836 100644 --- a/cpp/ql/src/Critical/DeadCodeFunction.qhelp +++ b/cpp/ql/src/Critical/DeadCodeFunction.qhelp @@ -3,28 +3,31 @@ "qhelp.dtd"> - -

    This rule finds functions that are non-public, non-virtual and are never called. Dead functions are often deprecated pieces of code, and should be removed -as they may increase object code size, decrease code comprehensibility, and create the possibility of misuse.

    +

    This query highlights functions that are non-public, non-virtual, and are never called. +Dead functions are often deprecated pieces of code, and should be removed. +If left in the code base they increase object code size, decrease code comprehensibility, and create the possibility of misuse.

    -public and protected functions are not considered by the check, as they could be part of the program's -API and could be used by external programs. +public and protected functions are ignored by this query. +This type of function may be part of the program's API and could be used by external programs.

    -     + -

    Consider removing the function.

    - +

    Verify that the function is genuinely unused and consider removing it.

     - - - - + +

    The example below includes a function f that is no longer used and should be deleted.

    +     + + +
  • SEI CERT C++ Coding Standard: MSC12-C. Detect and remove code that has no effect or is never executed.
    • +     +     diff --git a/cpp/ql/src/Critical/DeadCodeFunction.ql b/cpp/ql/src/Critical/DeadCodeFunction.ql index 8ddb30804ae..f3eee7135bd 100644 --- a/cpp/ql/src/Critical/DeadCodeFunction.ql +++ b/cpp/ql/src/Critical/DeadCodeFunction.ql @@ -1,6 +1,6 @@ /** * @name Function is never called - * @description A function is never called, and should be considered for removal. Unused functions may increase object size, decrease readability and create the possibility of misuse. + * @description Unused functions may increase object size, decrease readability, and create the possibility of misuse. * @kind problem * @id cpp/dead-code-function * @problem.severity warning diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp b/cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp index 26680a2598b..1c3037a7529 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp @@ -5,26 +5,26 @@ -

    This rule finds calls to functions that use a global variable which happen before the variable was initialized. +

    This rule finds calls to functions that use a global variable before the variable has been initialized. Not all compilers generate code that zero-out memory, especially when optimizations are enabled or the compiler is not compliant with the latest language standards. Accessing uninitialized memory will lead to undefined results.

    -     +

    Initialize the global variable. If no constant can be used for initialization, ensure that all accesses to the variable occur after the initialization code is executed.

    -     - - - - - - + +In the example below, callCtr is wrongly used before it has been initialized. + + + +
  • SEI CERT C++ Coding Standard: EXP53-CPP. Do not read uninitialized memory.
    • +     diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index efab4be7996..244e3939422 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -1,6 +1,6 @@ /** - * @name Global variable used before initialization - * @description A function that uses a global variable has been called before the variable has been initialized. Not all compilers zero-out memory for variables, especially when optimizations are enabled, or if the compiler is not compliant with the latest language standards. Using an uninitialized variable leads to undefined results. + * @name Global variable may be used before initialization + * @description Using an uninitialized variable leads to undefined results. * @kind problem * @id cpp/global-use-before-init * @problem.severity warning diff --git a/cpp/ql/src/Critical/InconsistentNullnessTesting.qhelp b/cpp/ql/src/Critical/InconsistentNullnessTesting.qhelp index ee1b2980727..9de3019bdb0 100644 --- a/cpp/ql/src/Critical/InconsistentNullnessTesting.qhelp +++ b/cpp/ql/src/Critical/InconsistentNullnessTesting.qhelp @@ -5,23 +5,28 @@ -

    This rule finds pointer dereferences that do not check the pointer for nullness, while the same pointer is checked for nullness in other -places in the code. It is most likely that the nullness check was omitted, and that a NULL pointer dereference can occur. -Dereferencing a null pointer and attempting to modify its contents can lead to anything from a segfault to corrupting -important system data (i.e. the interrupt table in some architectures). +

    This query finds pointer dereferences that do not first check the pointer for nullness, +even though the same pointer is checked for nullness in other +parts of the code. It is likely that the nullness check was accidentally omitted, and that a null pointer dereference can occur. +Dereferencing a null pointer and attempting to modify its contents can lead to anything from a segmentation fault to corrupting +important system data (including the interrupt table in some architectures).

    -     + -

    Make the nullness check on the pointer consistent across all dereferences.

    +

    Use a nullness check consistently in all cases where a pointer is dereferenced.

     - - - - - + +This code shows two examples where a pointer is dereferenced. +The first example checks that the pointer is not null before dereferencing it. +The second example fails to perform a nullnes check, leading to a potential vulnerability in the code. + + + +
  • SEI CERT C++ Coding Standard: MEM10-C. Define and use a pointer validation function.
    • +     diff --git a/cpp/ql/src/Critical/InconsistentNullnessTesting.ql b/cpp/ql/src/Critical/InconsistentNullnessTesting.ql index 1b17990f619..e54fe8f5497 100644 --- a/cpp/ql/src/Critical/InconsistentNullnessTesting.ql +++ b/cpp/ql/src/Critical/InconsistentNullnessTesting.ql @@ -1,6 +1,6 @@ /** * @name Inconsistent null check of pointer - * @description A dereferenced pointer is not checked for nullness in the given location, but is checked in other locations. Dereferencing a NULL pointer leads to undefined results. + * @description A dereferenced pointer is not checked for nullness in this location, but it is checked in other locations. Dereferencing a null pointer leads to undefined results. * @kind problem * @id cpp/inconsistent-nullness-testing * @problem.severity warning diff --git a/cpp/ql/src/Critical/OverflowCalculated.qhelp b/cpp/ql/src/Critical/OverflowCalculated.qhelp index 7cc1691d89b..ba441d4df4a 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.qhelp +++ b/cpp/ql/src/Critical/OverflowCalculated.qhelp @@ -6,14 +6,17 @@

    -This rule finds malloc that use a strlen for the size but to not take the -zero terminator into consideration, and strcat/strncat calls that are done on buffers that do -not have the sufficient size to contain the new string. +This query finds calls to:

    +
      +
    • malloc that use a strlen for the buffer size and do not take the +zero terminator into consideration.
      • +
    • strcat or strncat that use buffers that are too small to contain the new string.
      • +

    -The indicated expression will cause a buffer overflow due to a buffer that is of insufficient size to contain -the data being copied. Buffer overflows can result to anything from a segfault to a security vulnerability (particularly +The highlighted expression will cause a buffer overflow because the buffer is too small to contain +the data being copied. Buffer overflows can result to anything from a segmentation fault to a security vulnerability (particularly if the array is on stack-allocated memory).

    @@ -24,18 +27,23 @@ if the array is on stack-allocated memory).

    Increase the size of the buffer being allocated.

    - - - + +

    This example includes thre annotated calls that copy a string into a buffer. +The first call to malloc creates a buffer that's the +same size as the string, leaving no space for the zero terminator +and causing an overflow. The second call to malloc +correctly calculates the required buffer size. The call to +strcat appends an additional string to the same buffer +causing a second overflow.

    +     - +
  • CWE-131: Incorrect Calculation of Buffer Size
  • I. Gerg. An Overview and Example of the Buffer-Overflow Exploit. IANewsletter vol 7 no 4. 2005.
  • M. Donaldson. Inside the Buffer Overflow Attack: Mechanism, Method & Prevention. SANS Institute InfoSec Reading Room. 2002.
    • -     diff --git a/cpp/ql/src/Critical/OverflowDestination.qhelp b/cpp/ql/src/Critical/OverflowDestination.qhelp index e8a7e6ef322..c110bc4f6a6 100644 --- a/cpp/ql/src/Critical/OverflowDestination.qhelp +++ b/cpp/ql/src/Critical/OverflowDestination.qhelp @@ -3,27 +3,31 @@ "qhelp.dtd"> -

    The bounded copy functions memcpy, memmove, strncpy, strncat accept a size argument. You should call these functions with a size argument that is derived from the size of the destination buffer. Using a size argument that is derived from the source buffer may cause a buffer overflow. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.

    - - +

    The bounded copy functions memcpy, memmove, strncpy, strncat accept a size argument. +You should call these functions with a size argument that is derived from the size of the destination buffer. +Using a size argument that is derived from the source buffer may cause a buffer overflow. +Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.

     + -

    Check the highlighted function calls carefully, and ensure that the size parameter is derived from the size of the destination buffer, +

    Check the highlighted function calls carefully. +Ensure that the size parameter is derived from the size of the destination buffer, and not the source buffer.

    -     - + +

    +The code below shows an example where strncpy is called incorrectly, without checking the size of the destination buffer. +In the second example the call has been updated to include the size of the destination buffer.

    +     - -
  • CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
    • +
  • I. Gerg. An Overview and Example of the Buffer-Overflow Exploit. IANewsletter vol 7 no 4. 2005.
  • M. Donaldson. Inside the Buffer Overflow Attack: Mechanism, Method & Prevention. SANS Institute InfoSec Reading Room. 2002.
    • -     diff --git a/cpp/ql/src/Critical/OverflowDestination.ql b/cpp/ql/src/Critical/OverflowDestination.ql index 9f03146e366..fd2863573cf 100644 --- a/cpp/ql/src/Critical/OverflowDestination.ql +++ b/cpp/ql/src/Critical/OverflowDestination.ql @@ -1,7 +1,7 @@ /** * @name Copy function using source size * @description Calling a copy operation with a size derived from the source - * buffer instead of the destination buffer may result in a buffer overflow + * buffer instead of the destination buffer may result in a buffer overflow. * @kind problem * @id cpp/overflow-destination * @problem.severity warning