Skip to content

Commit

Permalink
[clang][analyzer] Move security.cert.env.InvalidPtr out of alpha …
Browse files Browse the repository at this point in the history 
…(#71912)

Thanks to recent improvements in #67663, InvalidPtr checker does not
emit any false positives on the following OS projects: memcached, tmux,
curl, twin, vim, openssl, sqlite, ffmpeg, postgres, tinyxml2, libwebm,
xerces, bitcoin, protobuf, qtbase, contour, acid, openrct2. (Before the
changes mentioned above, there were 27 reports, catching the `getenv`
invalidates previous `getenv` results cases. That strict behaviour is
disabled by default)
  • Loading branch information
@gamesh411
gamesh411 authored Nov 24, 2023
1 parent dd0b3c2 commit b98a594
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 105 deletions.
138 changes: 69 additions & 69 deletions clang/docs/analyzer/checkers.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,75 @@ security
Security related checkers.
.. _security-cert-env-InvalidPtr:
security.cert.env.InvalidPtr
""""""""""""""""""""""""""""""""""
Corresponds to SEI CERT Rules `ENV31-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV31-C.+Do+not+rely+on+an+environment+pointer+following+an+operation+that+may+invalidate+it>`_ and `ENV34-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV34-C.+Do+not+store+pointers+returned+by+certain+functions>`_.
* **ENV31-C**:
Rule is about the possible problem with ``main`` function's third argument, environment pointer,
"envp". When environment array is modified using some modification function
such as ``putenv``, ``setenv`` or others, It may happen that memory is reallocated,
however "envp" is not updated to reflect the changes and points to old memory
region.
* **ENV34-C**:
Some functions return a pointer to a statically allocated buffer.
Consequently, subsequent call of these functions will invalidate previous
pointer. These functions include: ``getenv``, ``localeconv``, ``asctime``, ``setlocale``, ``strerror``
.. code-block:: c
int main(int argc, const char *argv[], const char *envp[]) {
if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
// setenv call may invalidate 'envp'
/* Handle error */
}
if (envp != NULL) {
for (size_t i = 0; envp[i] != NULL; ++i) {
puts(envp[i]);
// envp may no longer point to the current environment
// this program has unanticipated behavior, since envp
// does not reflect changes made by setenv function.
}
}
return 0;
}
void previous_call_invalidation() {
char *p, *pp;
p = getenv("VAR");
setenv("SOMEVAR", "VALUE", /*overwrite = */1);
// call to 'setenv' may invalidate p
*p;
// dereferencing invalid pointer
}
The ``InvalidatingGetEnv`` option is available for treating ``getenv`` calls as
invalidating. When enabled, the checker issues a warning if ``getenv`` is called
multiple times and their results are used without first creating a copy.
This level of strictness might be considered overly pedantic for the commonly
used ``getenv`` implementations.
To enable this option, use:
``-analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true``.
By default, this option is set to *false*.
When this option is enabled, warnings will be generated for scenarios like the
following:
.. code-block:: c
char* p = getenv("VAR");
char* pp = getenv("VAR2"); // assumes this call can invalidate `env`
strlen(p); // warns about accessing invalid ptr
.. _security-FloatLoopCounter:
security.FloatLoopCounter (C)
Expand Down Expand Up @@ -2549,75 +2618,6 @@ alpha.security.cert.env
SEI CERT checkers of `Environment C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_.
.. _alpha-security-cert-env-InvalidPtr:
alpha.security.cert.env.InvalidPtr
""""""""""""""""""""""""""""""""""
Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
ENV31-C:
Rule is about the possible problem with `main` function's third argument, environment pointer,
"envp". When environment array is modified using some modification function
such as putenv, setenv or others, It may happen that memory is reallocated,
however "envp" is not updated to reflect the changes and points to old memory
region.
ENV34-C:
Some functions return a pointer to a statically allocated buffer.
Consequently, subsequent call of these functions will invalidate previous
pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror
.. code-block:: c
int main(int argc, const char *argv[], const char *envp[]) {
if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
// setenv call may invalidate 'envp'
/* Handle error */
}
if (envp != NULL) {
for (size_t i = 0; envp[i] != NULL; ++i) {
puts(envp[i]);
// envp may no longer point to the current environment
// this program has unanticipated behavior, since envp
// does not reflect changes made by setenv function.
}
}
return 0;
}
void previous_call_invalidation() {
char *p, *pp;
p = getenv("VAR");
setenv("SOMEVAR", "VALUE", /*overwrite = */1);
// call to 'setenv' may invalidate p
*p;
// dereferencing invalid pointer
}
The ``InvalidatingGetEnv`` option is available for treating getenv calls as
invalidating. When enabled, the checker issues a warning if getenv is called
multiple times and their results are used without first creating a copy.
This level of strictness might be considered overly pedantic for the commonly
used getenv implementations.
To enable this option, use:
``-analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true``.
By default, this option is set to *false*.
When this option is enabled, warnings will be generated for scenarios like the
following:
.. code-block:: c
char* p = getenv("VAR");
char* pp = getenv("VAR2"); // assumes this call can invalidate `env`
strlen(p); // warns about accessing invalid ptr
alpha.security.taint
^^^^^^^^^^^^^^^^^^^^
Expand Down
28 changes: 15 additions & 13 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,12 @@ def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>;
def SecurityAlpha : Package<"security">, ParentPackage<Alpha>;
def Taint : Package<"taint">, ParentPackage<SecurityAlpha>;

def CERT : Package<"cert">, ParentPackage<SecurityAlpha>;
def POS : Package<"pos">, ParentPackage<CERT>;
def CERT : Package<"cert">, ParentPackage<Security>;
def ENV : Package<"env">, ParentPackage<CERT>;

def CERTAlpha : Package<"cert">, ParentPackage<SecurityAlpha>;
def POSAlpha : Package<"pos">, ParentPackage<CERTAlpha>;

def Unix : Package<"unix">;
def UnixAlpha : Package<"unix">, ParentPackage<Alpha>;
def CString : Package<"cstring">, ParentPackage<Unix>;
Expand Down Expand Up @@ -993,15 +995,6 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,

} // end "security"

let ParentPackage = POS in {

def PutenvWithAuto : Checker<"34c">,
HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
"an automatic variable as the argument.">,
Documentation<HasDocumentation>;

} // end "alpha.cert.pos"

let ParentPackage = ENV in {

def InvalidPtrChecker : Checker<"InvalidPtr">,
Expand All @@ -1013,11 +1006,20 @@ let ParentPackage = ENV in {
"standard), which can lead to false positives depending on "
"implementation.",
"false",
InAlpha>,
Released>,
]>,
Documentation<HasDocumentation>;

} // end "alpha.cert.env"
} // end "security.cert.env"

let ParentPackage = POSAlpha in {

def PutenvWithAuto : Checker<"34c">,
HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
"an automatic variable as the argument.">,
Documentation<HasDocumentation>;

} // end "alpha.cert.pos"

let ParentPackage = SecurityAlpha in {

Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/analyzer-config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
// CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
// CHECK-NEXT: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv = false
// CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
// CHECK-NEXT: apply-fixits = false
// CHECK-NEXT: assume-controlled-environment = false
Expand Down Expand Up @@ -116,6 +115,7 @@
// CHECK-NEXT: region-store-small-array-limit = 5
// CHECK-NEXT: region-store-small-struct-limit = 2
// CHECK-NEXT: report-in-main-source-file = false
// CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false
// CHECK-NEXT: serialize-stats = false
// CHECK-NEXT: silence-checkers = ""
// CHECK-NEXT: stable-report-filename = false
Expand Down
10 changes: 5 additions & 5 deletions clang/test/Analysis/cert/env31-c.c
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify=putenv,common \
// RUN: -DENV_INVALIDATING_CALL="putenv(\"X=Y\")"
//
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify=putenvs,common \
// RUN: -DENV_INVALIDATING_CALL="_putenv_s(\"X\", \"Y\")"
//
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify=wputenvs,common \
// RUN: -DENV_INVALIDATING_CALL="_wputenv_s(\"X\", \"Y\")"
//
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify=setenv,common \
// RUN: -DENV_INVALIDATING_CALL="setenv(\"X\", \"Y\", 0)"
//
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify=unsetenv,common \
// RUN: -DENV_INVALIDATING_CALL="unsetenv(\"X\")"

Expand Down
16 changes: 8 additions & 8 deletions clang/test/Analysis/cert/env34-c-cert-examples.c
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
// Default options.
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -verify -Wno-unused %s
//
// Test the laxer handling of getenv function (this is the default).
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
// RUN: -verify -Wno-unused %s
//
// Test the stricter handling of getenv function.
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: -verify=expected,pedantic -Wno-unused %s

#include "../Inputs/system-header-simulator.h"
Expand Down
4 changes: 2 additions & 2 deletions clang/test/Analysis/cert/env34-c.c
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr\
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: -analyzer-checker=security.cert.env.InvalidPtr\
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: -analyzer-output=text -verify -Wno-unused %s

#include "../Inputs/system-header-simulator.h"
Expand Down
14 changes: 7 additions & 7 deletions clang/test/Analysis/invalid-ptr-checker.c
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=security.cert.env.InvalidPtr \
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
// RUN: -analyzer-output=text -verify -Wno-unused %s
//
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \
// RUN: -analyzer-config \
// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=security.cert.env.InvalidPtr \
// RUN: -analyzer-config \
// RUN: security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
// RUN: -analyzer-output=text -verify=expected,pedantic -Wno-unused %s

#include "Inputs/system-header-simulator.h"
Expand Down

0 comments on commit b98a594

Please sign in to comment.