caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' (#92424)

Browse Source 
The "cert" package looks not useful and the checker has not a meaningful name with the old naming scheme.
Additionally tests and documentation is updated.
This commit is contained in:
Balázs Kéri
2024-05-23 12:56:16 +02:00
committed by GitHub
parent 6552af1968
commit 3c23047413
7 changed files with 119 additions and 187 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
clang/docs/analyzer/checkers.rst
View File

@@ -2833,6 +2833,31 @@ Warn on mmap() calls that are both writable and executable.
// code
}
.. _alpha-security-putenv-stack-array:
alpha.security.PutenvStackArray (C)
"""""""""""""""""""""""""""""""""""
Finds calls to the ``putenv`` function which pass a pointer to a stack-allocated
(automatic) array as the argument. Function ``putenv`` does not copy the passed
string, only a pointer to the data is stored and this data can be read even by
other threads. Content of a stack-allocated array is likely to be overwritten
after returning from the parent function.
The problem can be solved by using a static array variable or dynamically
allocated memory. Even better is to avoid using ``putenv`` (it has other
problems related to memory leaks) and use ``setenv`` instead.
The check corresponds to CERT rule
`POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument
<https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument>`_.
.. code-block:: c
int f() {
char env[] = "NAME=value";
return putenv(env); // putenv function should not be called with stack-allocated string
}
.. _alpha-security-ReturnPtrRange:
alpha.security.ReturnPtrRange (C)
@@ -2859,55 +2884,6 @@ alpha.security.cert
SEI CERT checkers which tries to find errors based on their `C coding rules <https://wiki.sei.cmu.edu/confluence/display/c/2+Rules>`_.
.. _alpha-security-cert-pos-checkers:
alpha.security.cert.pos
^^^^^^^^^^^^^^^^^^^^^^^
SEI CERT checkers of `POSIX C coding rules <https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152405>`_.
.. _alpha-security-cert-pos-34c:
alpha.security.cert.pos.34c
"""""""""""""""""""""""""""
Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
.. code-block:: c
int func(const char *var) {
char env[1024];
int retval = snprintf(env, sizeof(env),"TEST=%s", var);
if (retval < 0 || (size_t)retval >= sizeof(env)) {
/* Handle error */
}
return putenv(env); // putenv function should not be called with auto variables
}
Limitations:
- Technically, one can pass automatic variables to ``putenv``,
but one needs to ensure that the given environment key stays
alive until it's removed or overwritten.
Since the analyzer cannot keep track of which envvars get overwritten
and when, it needs to be slightly more aggressive and warn for such
cases too, leading in some cases to false-positive reports like this:
.. code-block:: c
void baz() {
char env[] = "NAME=value";
putenv(env); // false-positive warning: putenv function should not be called...
// More code...
putenv((char *)"NAME=anothervalue");
// This putenv call overwrites the previous entry, thus that can no longer dangle.
} // 'env' array becomes dead only here.
alpha.security.cert.env
^^^^^^^^^^^^^^^^^^^^^^^
SEI CERT checkers of `Environment C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_.
alpha.security.taint
^^^^^^^^^^^^^^^^^^^^

22
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View File

@@ -1035,15 +1035,6 @@ let ParentPackage = ENV in {
} // 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 {
def ArrayBoundChecker : Checker<"ArrayBound">,
@@ -1054,10 +1045,6 @@ def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
HelpText<"Warn about buffer overflows (newer checker)">,
Documentation<HasDocumentation>;
def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">,
HelpText<"Check for an out-of-bound pointer being returned to callers">,
Documentation<HasDocumentation>;
def MallocOverflowSecurityChecker : Checker<"MallocOverflow">,
HelpText<"Check for overflows in the arguments to malloc()">,
Documentation<HasDocumentation>;
@@ -1078,6 +1065,15 @@ def MmapWriteExecChecker : Checker<"MmapWriteExec">,
]>,
Documentation<HasDocumentation>;
def PutenvStackArray : Checker<"PutenvStackArray">,
HelpText<"Finds calls to the function 'putenv' which pass a pointer to "
"an automatic (stack-allocated) array as the argument.">,
Documentation<HasDocumentation>;
def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">,
HelpText<"Check for an out-of-bound pointer being returned to callers">,
Documentation<HasDocumentation>;
} // end "alpha.security"
//===----------------------------------------------------------------------===//

2
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
View File

@@ -96,7 +96,7 @@ add_clang_library(clangStaticAnalyzerCheckers
PointerSortingChecker.cpp
PointerSubChecker.cpp
PthreadLockChecker.cpp
cert/PutenvWithAutoChecker.cpp
PutenvStackArrayChecker.cpp
RetainCountChecker/RetainCountChecker.cpp
RetainCountChecker/RetainCountDiagnostics.cpp
ReturnPointerRangeChecker.cpp

26
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp → clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp
View File

@@ -1,4 +1,4 @@
//== PutenvWithAutoChecker.cpp --------------------------------- -*- C++ -*--=//
//== PutenvStackArrayChecker.cpp ------------------------------- -*- C++ -*--=//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -6,13 +6,13 @@
//
//===----------------------------------------------------------------------===//
//
// This file defines PutenvWithAutoChecker which finds calls of ``putenv``
// function with automatic variable as the argument.
// This file defines PutenvStackArrayChecker which finds calls of ``putenv``
// function with automatic array variable as the argument.
// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
//
//===----------------------------------------------------------------------===//
#include "../AllocationState.h"
#include "AllocationState.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -26,9 +26,9 @@ using namespace clang;
using namespace ento;
namespace {
class PutenvWithAutoChecker : public Checker<check::PostCall> {
class PutenvStackArrayChecker : public Checker<check::PostCall> {
private:
BugType BT{this, "'putenv' function should not be called with auto variables",
BugType BT{this, "'putenv' called with stack-allocated string",
categories::SecurityError};
const CallDescription Putenv{CDM::CLibrary, {"putenv"}, 1};
@@ -37,8 +37,8 @@ public:
};
} // namespace
void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
void PutenvStackArrayChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
if (!Putenv.matches(Call))
return;
@@ -50,7 +50,7 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
return;
StringRef ErrorMsg = "The 'putenv' function should not be called with "
"arguments that have automatic storage";
"arrays that have automatic storage";
ExplodedNode *N = C.generateErrorNode();
auto Report = std::make_unique<PathSensitiveBugReport>(BT, ErrorMsg, N);
@@ -60,8 +60,10 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
C.emitReport(std::move(Report));
}
void ento::registerPutenvWithAuto(CheckerManager &Mgr) {
Mgr.registerChecker<PutenvWithAutoChecker>();
void ento::registerPutenvStackArray(CheckerManager &Mgr) {
Mgr.registerChecker<PutenvStackArrayChecker>();
}
bool ento::shouldRegisterPutenvWithAuto(const CheckerManager &) { return true; }
bool ento::shouldRegisterPutenvStackArray(const CheckerManager &) {
return true;
}

51
clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
View File

@@ -1,51 +0,0 @@
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.cert.pos.34c\
// RUN: -verify %s
#include "../Inputs/system-header-simulator.h"
void free(void *memblock);
void *malloc(size_t size);
int putenv(char *);
int rand();
namespace test_auto_var_used_good {
extern char *ex;
int test_extern() {
return putenv(ex); // no-warning: extern storage class.
}
void foo(void) {
char *buffer = (char *)"huttah!";
if (rand() % 2 == 0) {
buffer = (char *)malloc(5);
strcpy(buffer, "woot");
}
putenv(buffer);
}
void bar(void) {
char *buffer = (char *)malloc(5);
strcpy(buffer, "woot");
if (rand() % 2 == 0) {
free(buffer);
buffer = (char *)"blah blah blah";
}
putenv(buffer);
}
void baz() {
char env[] = "NAME=value";
// TODO: False Positive
putenv(env);
// expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
/*
DO SOMETHING
*/
putenv((char *)"NAME=anothervalue");
}
} // namespace test_auto_var_used_good

61
clang/test/Analysis/cert/pos34-c.cpp
View File

@@ -1,61 +0,0 @@
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.cert.pos.34c\
// RUN: -verify %s
// Examples from the CERT rule's page.
// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
#include "../Inputs/system-header-simulator.h"
void free(void *memblock);
void *malloc(size_t size);
int putenv(char *);
int snprintf(char *str, size_t size, const char *format, ...);
namespace test_auto_var_used_bad {
int volatile_memory1(const char *var) {
char env[1024];
int retval = snprintf(env, sizeof(env), "TEST=%s", var);
if (retval < 0 || (size_t)retval >= sizeof(env)) {
/* Handle error */
}
return putenv(env);
// expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
}
} // namespace test_auto_var_used_bad
namespace test_auto_var_used_good {
int test_static(const char *var) {
static char env[1024];
int retval = snprintf(env, sizeof(env), "TEST=%s", var);
if (retval < 0 || (size_t)retval >= sizeof(env)) {
/* Handle error */
}
return putenv(env);
}
int test_heap_memory(const char *var) {
static char *oldenv;
const char *env_format = "TEST=%s";
const size_t len = strlen(var) + strlen(env_format);
char *env = (char *)malloc(len);
if (env == NULL) {
return -1;
}
if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
free(env);
return -1;
}
if (oldenv != NULL) {
free(oldenv); /* avoid memory leak */
}
oldenv = env;
return 0;
}
} // namespace test_auto_var_used_good

70
clang/test/Analysis/putenv-stack-array.c Normal file
View File

@@ -0,0 +1,70 @@
// RUN: %clang_analyze_cc1 \
// RUN: -analyzer-checker=alpha.security.PutenvStackArray \
// RUN: -verify %s
#include "Inputs/system-header-simulator.h"
void free(void *);
void *malloc(size_t);
int putenv(char *);
int snprintf(char *, size_t, const char *, ...);
int test_auto_var(const char *var) {
char env[1024];
(void)snprintf(env, sizeof(env), "TEST=%s", var);
return putenv(env); // expected-warning{{The 'putenv' function should not be called with arrays that have automatic storage}}
}
int test_static_var(const char *var) {
static char env[1024];
(void)snprintf(env, sizeof(env), "TEST=%s", var);
return putenv(env); // no-warning: static array is used
}
void test_heap_memory(const char *var) {
const char *env_format = "TEST=%s";
const size_t len = strlen(var) + strlen(env_format);
char *env = (char *)malloc(len);
if (env == NULL)
return;
if (putenv(env) != 0) // no-warning: env was dynamically allocated.
free(env);
}
typedef struct {
int A;
char Env[1024];
} Mem;
int test_auto_var_struct() {
Mem mem;
return putenv(mem.Env); // expected-warning{{The 'putenv' function should not be called with}}
}
int test_auto_var_subarray() {
char env[1024];
return putenv(env + 100); // expected-warning{{The 'putenv' function should not be called with}}
}
int test_constant() {
char *env = "TEST";
return putenv(env); // no-warning: data is not on the stack
}
extern char *ext_env;
int test_extern() {
return putenv(ext_env); // no-warning: extern storage class.
}
void test_auto_var_reset() {
char env[] = "NAME=value";
putenv(env); // expected-warning{{The 'putenv' function should not be called with}}
// ... (do something)
// Even cases like this are likely a bug:
// It looks like that if one string was passed to putenv,
// it should not be deallocated at all, because when reading the
// environment variable a pointer into this string is returned.
// In this case, if another (or the same) thread reads variable "NAME"
// at this point and does not copy the returned string, the data may
// become invalid.
putenv((char *)"NAME=anothervalue");
}